New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Object URL references, named paths and pseudo-slugs #1674
Conversation
… DefaultCharacter, Account and DefaultRoom objects.
…tic get_*_url() fields to DefaultAccount.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very clever and looks like it would be powerful a tool for users to expand web functionality down the line.
- I think these web-specific methods need to be clearly marked somehow. I suggest naming them
web_*
to make it clearer what their function is for. Any better suggestions? - There is an API inconsistency here - if Accounts and Objects have these methods, the user will expect other typeclasses like Scripts and Messages to have them too. I think it would be an idea to explore if these methods could be moved up and generalized into the main Typeclass parent instead.
It's a good idea. I moved the methods to TypedObject and was able to eliminate some duplicate code. I initially couldn't think of any reasons why one would want to render a ScriptDetailView, but given that the docs encourage the use of scripts for global settings (like economic engines) I'm sure someone might find it useful to be able to display the current price of pork futures on their website. |
I think I've addressed all the feedback on this one; ready for review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, only a minor change to do, for consistency. And unit tests must pass..
evennia/typeclasses/models.py
Outdated
path (str): URI path to object detail page, if defined. | ||
|
||
""" | ||
return self.web_detail_url() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name this method web_get_absolute_url
for consistency. Since Django requires it on a specific form, you add that as an alias below:
get_absolute_url = web_get_absolute_url # django requires this name
@Griatch This should be good to go now... |
@strikaco Thanks for your work with this! Looks good now! Merged. |
except: return '#' | ||
|
||
# Used by Django Sites/Admin | ||
get_absolute_url = web_get_detail_url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is web_get_detail_url
not named web_absolute_url
if get_absolute_url
is what django expects - would not that name (just adding web_
) be more expected?
I get where you're coming from and agree; Django's use of the term
'absolute' is a bit nonsensical given that what's expected on the other end
of it is in fact a DetailView (or equivalent function). I'm not aware of
anywhere else they use the term 'absolute'.
One could also argue it should be get_read_* to adhere to the CRUD idea but
Django deviates there too-- their idea of a Read view is again the
DetailView.
So Detail felt like a more natural term to use. Where should the
get_update_url function go? An UpdateView. Where should get_detail_url go?
A DetailView. And so on.
…On Oct 22, 2018 3:44 PM, "Griatch" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In evennia/typeclasses/models.py
<#1674 (comment)>:
> + If no View has been created and defined in urls.py, returns an
+ HTML anchor.
+
+ This method is naive and simply returns a path. Securing access to
+ the actual view and limiting who can delete this object is the developer's
+ responsibility.
+
+ Returns:
+ path (str): URI path to object deletion page, if defined.
+
+ """
+ try: return reverse('%s-delete' % self._meta.verbose_name.lower(), kwargs={'pk': self.pk, 'slug': slugify(self.name)})
+ except: return '#'
+
+ # Used by Django Sites/Admin
+ get_absolute_url = web_get_detail_url
Why is web_get_detail_url not named web_absolute_url if get_absolute_url
is what django expects - would not that name (just adding web_) be more
expected?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1674 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHyB9KrWrQ3icknofCdWpRzRs27qzbodks5unko-gaJpZM4XLE8v>
.
|
Brief overview of PR changes/additions
This PR is designed to let one generate paths to an object's web interface(s) from within a Django template.
It adds a set of methods to retrieve HTTP paths for DefaultAccount objects and all children of DefaultObject:
get_admin_url()
: Returns a path that, if followed, will display the object in the Admin backend.get_absolute_url()
: Django construct; returns a path that should display the object in a DetailView.get_update_url()
: Returns a path that should display the object in an UpdateView.get_delete_url()
: Returns a path that should display the object in a DeleteView.Note that this does not magically expose all your objects, create DeleteViews for everything, let random users into Admin or enable any sort of new or scary functionality.
The admin function works out of the box. It lets you do things in templates like:
...so when looking at an object (say, on a DetailView) staff can quickly pivot to the Admin backend to change things.
For the rest to work, you need to have defined named URLs that map typeclasses to actions. It must follow a particular naming convention (
object-action
, soaccount-update
,character-delete
orroom-detail
would yield the link to an appropriate view).This enables template functionality like:
...to let developers quickly generate links to the appropriate views governing disposition of an object. Securing those views and restricting access is entirely the responsibility of the developer; this PR does not facilitate anything that isn't already possible, just makes things more convenient for frontend development.
This PR also makes it more difficult to enumerate predictable endpoints (
/accounts/1/
,/accounts/2/
, etc.). Evennia doesn't do slugs natively, though Django heavily encourages their use. When the link is generated, it passes the object pk as well as the slugified object name as kwargs. This means that the corresponding view must also be configured to receive and check an arbitraryslug
kwarg, which is not the case by default. The receiving view should retrieve the object by pk but confirm the slug relates to the object name. This will inhibit scrapers and crawlers from probing random object IDs by requiring knowledge of both the object ID and the slugified version of the object name to be supplied to the view. It also makes the URL more intelligible (/accounts/johnny/1/
,/accounts/frank/2/
, etc.).