Skip to content
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

Merged
merged 6 commits into from Oct 22, 2018
Merged

Object URL references, named paths and pseudo-slugs #1674

merged 6 commits into from Oct 22, 2018

Conversation

strikaco
Copy link
Contributor

@strikaco strikaco commented Oct 5, 2018

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:

{% if request.user.is_staff %}
<a href="{{ object.get_admin_url }}">Admin</a>
{% endif %}

...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, so account-update, character-delete or room-detail would yield the link to an appropriate view).

    # Character management
    url(r'^characters/create/$', website_views.CharacterCreateView.as_view(), name="character-create"),
    url(r'^characters/manage/$', website_views.CharacterManageView.as_view(), name="character-manage"),
    url(r'^characters/update/(?P<slug>[\w\d\-]+)/(?P<pk>[0-9]+)/$', website_views.CharacterUpdateView.as_view(), name="character-update"),
    url(r'^characters/delete/(?P<slug>[\w\d\-]+)/(?P<pk>[0-9]+)/$', website_views.CharacterDeleteView.as_view(), name="character-delete"),

This enables template functionality like:

{% for character in characters %}
<li>{{ character }} 
    <a href="{{ character.get_absolute_url }}">View</a> 
    <a href="{{ character.get_update_url }}">Update</a> 
    <a href="{{ character.get_delete_url }}">Delete</a>
</li>
{% endfor %}

...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 arbitrary slug 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.).

Copy link
Member

@Griatch Griatch left a 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.

evennia/accounts/accounts.py Outdated Show resolved Hide resolved
evennia/accounts/accounts.py Outdated Show resolved Hide resolved
@strikaco
Copy link
Contributor Author

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.

@strikaco
Copy link
Contributor Author

I think I've addressed all the feedback on this one; ready for review!

Copy link
Member

@Griatch Griatch left a 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..

path (str): URI path to object detail page, if defined.

"""
return self.web_detail_url()
Copy link
Member

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

@strikaco
Copy link
Contributor Author

@Griatch This should be good to go now...

@Griatch Griatch merged commit cfc8179 into evennia:develop Oct 22, 2018
@Griatch
Copy link
Member

Griatch commented Oct 22, 2018

@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
Copy link
Member

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?

@strikaco
Copy link
Contributor Author

strikaco commented Oct 22, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants