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

Allow customising the base URL and URL namespace for snippet views through SnippetViewSet #10235

Closed
wants to merge 9 commits into from

Conversation

laymonage
Copy link
Member

As part of #10206. Incorporates #10178.

Similar to ModelAdmin.base_url_path, you can customise the base URL path for the snippets views by setting SnippetViewSet.base_url_path. You can also customise the URL namespace, something that couldn't be done in ModelAdmin without a custom URL helper class, by setting SnippetViewSet.admin_url_namespace. If left unset, they default to snippets/{app_label}/{model_name} and wagtailsnippets_{app_label}_{model_name}, respectively. If further customisation is needed (e.g. you need to compute the string based on the model), the SnippetViewSet.get_admin_base_path() and SnippetViewSet.get_admin_url_namespace() methods can be overridden.

In addition, you can also do the same with the chooser views through chooser_base_url_path, chooser_admin_url_namespace, SnippetViewSet.get_chooser_admin_base_path() and SnippetViewSet.get_chooser_admin_url_namespace().

Note:
I'm not sure whether we should support the customisations for the chooser through SnippetViewSet or shall we just tell developers to override the SnippetViewSet.chooser_viewset property.


I also found a bug where the permissions for snippets registered through wagtail_hooks.py might not be initialised properly if Wagtail has not scanned for hooks.

Please check the following:

  • Do the tests still pass?1
  • Does the code comply with the style guide?
    • Run make lint from the Wagtail root.
  • For Python changes: Have you added tests to cover the new/fixed behaviour?
  • For front-end changes: Did you test on all of Wagtail’s supported environments?2
    • Please list the exact browser and operating system versions you tested:
    • Please list which assistive technologies 3 you tested:
  • For new features: Has the documentation been updated accordingly?

Please describe additional details for testing this change.

Continuing from the test example in #10178.

from wagtail.snippets.models import register_snippet
from wagtail.snippets.views.snippets import SnippetViewSet


class PersonViewSet(SnippetViewSet):
    icon = "user"
    admin_url_namespace = "some_namespace"
    base_url_path = "deep/within/the/admin"
    chooser_admin_url_namespace = "my_chooser_namespace"
    chooser_base_url_path = "choose/wisely"


register_snippet(Person, viewset=PersonViewSet)

You should be able to:

from django.urls import reverse

assert reverse("some_namespace:add") == "/admin/deep/within/the/admin/add/"
assert reverse("my_chooser_namespace:create") == "/admin/choose/wisely/create/"

Footnotes

  1. Development Testing

  2. Browser and device support

  3. Accessibility Target

Daniel Kirkham and others added 9 commits March 15, 2023 10:28
…idget()

This effectively sets the chooser widget icon to use SnippetViewSet.icon
Snippets can be registered in wagtail_hooks.py by calling register_snippet as a function instead of a decorator. If there is code that looks for snippet models and is executed before hooks are loaded, it may lead to incorrect results.

For example, the create_extra_permissions function is connected to the post_migrate signal, which gets called very early when testing. Without this change, running wagtail.snippets.tests.test_locking separately will result to a failing test because the permissions for FullFeaturedSnippet (that is registered in wagtail_hooks) have not been set up, as the testapp's hooks have not been called by the time the post_migrate signal is sent.
Comment on lines +1166 to +1176
def register_model_methods(self):
@classmethod
def get_admin_url_namespace(cls):
return self.get_admin_url_namespace()

@classmethod
def get_admin_base_path(cls):
return self.get_admin_base_path()

self.model.get_admin_url_namespace = get_admin_url_namespace
self.model.get_admin_base_path = get_admin_base_path
Copy link
Member Author

@laymonage laymonage Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to keep doing this? Or should we advocate for model.snippet_viewset.get_admin_url_namespace() etc.?

It is quite handy, but I'm not really a fan of doing too much patching on the model. Even more so now that we have a reference to the viewset through model.snippet_viewset, we can access all the configuration from there. (Speaking of which, I'm thinking whether it would be better to do the model.snippet_viewset patching from here instead.)

We don't have to remove this outright (in case people depend on it, even though these aren't documented), but I'd like us to have a preferred way to get these values.

The same goes to these as well:

model.get_usage = lambda obj: ReferenceIndex.get_references_to(
obj
).group_by_source_object()
model.usage_url = get_snippet_usage_url

If we remove (or at least move) those, then what register_snippet does is pretty much just registering a snippet model with a viewset, putting the model into a global list, and that's it, which sounds very nice.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very much in favour of keeping as much of this as possible contained within the viewset class - patching the model with these methods seems like it was a necessary evil before we had an obvious place other than the model to hang this common functionality onto, but the viewset is the natural place for it now. I think it's reasonable to remove it outright - I can't imagine there being any expectation for these to be anything more than Wagtail-internal glue.

In an ideal world I'd like to see the snippet_viewset attribute dropped too, so that the view-level and model-level considerations are fully separate - but it sounds like it'd need some heavy refactoring of the bulk action views to make that happen, and patching a single snippet_viewset attribute is definitely a lot cleaner than patching multiple methods into the model.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll do this in a separate PR, as the get_admin_url_namespace is still used in a few places in Wagtail.

In an ideal world I'd like to see the snippet_viewset attribute dropped too, so that the view-level and model-level considerations are fully separate - but it sounds like it'd need some heavy refactoring of the bulk action views to make that happen, and patching a single snippet_viewset attribute is definitely a lot cleaner than patching multiple methods into the model.

We might be able to get away with something simpler to achieve this, e.g. the idea in #10178 (comment):

Or something simpler, we could change the SNIPPET_MODELS global list into a dictionary that maps each model to the viewset instance? People who rely on get_snippet_models() will still be able to iterate through it like a list of models, but we will be able to do something like get_snippet_models()[MyModel] and get the viewset. Still, it's not as handy compared to MyModel.snippet_viewset, though.

Or introduce another method, e.g.

def get_snippet_viewset(model):
    return get_snippet_models()[model]

Copy link
Collaborator

@gasman gasman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good to me! Will leave this unmerged in case you want to do the changes proposed in #10235 (comment) as part of this PR.

gasman added a commit that referenced this pull request Mar 23, 2023
@laymonage
Copy link
Member Author

Merged in 8d2b1c6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants