Feature/10921 snippet copy function

[smark-1]

Adds #10921

Please check the following:

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

Please describe additional details for testing this change.

Footnotes

1. Development Testing ↩

2. Browser and device support ↩

3. Accessibility Target ↩

[squash-labs]

Manage this branch in Squash

Test this branch here: https://smark-1feature10921-snippet-co-p882k.squash.io

[lb-]

@smark-1 thank you for this PR and well done on getting your first PR up.

I have started the CI, please review any failures and resolve them if you can.

Additionally, it would be good for you to set up some tests for this.

[smark-1]

I resolved some of the failures. but many of the errors seem to be caused by raise NoReverseMatch(msg) django.urls.exceptions.NoReverseMatch: Reverse for 'copy' with keyword arguments ... I can't figure out what the root cause is for this error.

[dkirkham]

Hi @smark-1, just looking at this both as the original author of the issue, and because I'd like to see this addition make it into the next release. I also will need a snippet copy function for code I'm porting over from ModelAdmin, which I want to make work under Wagtail 5.2 LTS, so will need backport equivalent functionality for that.

I have a suggestion, which will make the code a bit more consistent and hopefully clear up the NoReverseMatch test errors. This is to slightly restructure the code in wagtail/admin/views/generic/models.py:427-444 exactly like the edit, inspect and delete code around it. In particular:

• calculate copy_url using a new method get_copy_url(instance) based on get_edit_url(instance), and an attribute copy_url_name. You will see in that method that the instance key is passed to reverse() via args, not kwargs, which should fix up the test failure
• calculate can_copy from the permissions and copy_view_enabled, ahead of the conditional to add the button.

Hope this helps.

[smark-1]

@dkirkham Thank you. I can't figure out where the edit_url_name, inspect_url_name, and delete_url_name are overridden to be a different value. so I can't figure out where it would be appropriate to override copy_url_name to make it work this way.

[smark-1]

@dkirkham I was able to figure out how the inspect view works and I got the new copy view to work the same way. Now there are only 35 errors that all the message "django.urls.exceptions.NoReverseMatch: Reverse for 'copy' not found. 'copy' is not a valid view function or pattern name." This is definitely an improvement, but I am not sure how to resolve these errors.

[dkirkham]

Hi @smark-1, I see what's happening. Some of your changes are to ModelViewSet and the generic model views, and some are to SnippetViewSet. In particular, the new copy view URL paths are being created in the SnippetViewSet, but the IndexView which needs the new copy URL paths (hence using reverse()) is in ModelViewSet. This means the ModelViewSet tests, in wagtail.admin.tests.viewsets.test_model_viewset, end up failing because these tests don't use the SnippetViewSet and the necessary URL paths haven't been created.

This does beg the question, should we be adding the copy function to ModelViewSet, which then gets inherited by SnippetViewSet, or only to SnippetViewSet?

I think that a copy function is pretty fundamental and applicable to a model, and not related to the goodness that snippets add with all the publishing-related functions. Hence I'd suggest seeing if the copy function can be added primarily to ModelViewSet, and make sure SnippetViewSet inherits it correctly.

Maybe @lb- or @laymonage, you would care to comment?

[dkirkham]

Thanks for the update @smark-1. As mentioned by @lb-, there will need to be a couple of tests for the new functionality.

Meanwhile, I'll try this update in a small project and see how it performs.

[dkirkham]

Hi @smark-1, I've set up a simple project with a minimal model, snippet and snippet with inline model. The copy functionality in both ModelViewSet and SnippetViewSet appears to be working fine with each.

The next step is to add some tests, for both ModelViewSet and SnippetViewSet, and update the documentation. Then it should be ready for review by the core team.

[smark-1]

@dkirkham I was very busy this past week. I hope to finish adding the tests and documentation sometime this week.

[smark-1]

@lb- I think this is ready for review

[lb-]

@smark-1 are you able to rebase on main, looks like there is a conflict with test_model_viewset.py. I will flag for a review though.

[lb-]

Just a note, whatever naming/approach we decide here we would probably want to use that on Page models also. It's likely that we would want to follow the same convention to solve #10147 (by providing a custom form class).

I know that this copy function is about the view class but maybe we should consider a way to override the form class in isolation of the view.

[dkirkham]

Another comment @lb-, I left a question at #10921 (comment) about what hooks should be called during the copy process, with no response so far. I haven't checked this, but I expect the copy code in this PR will only call the after_create_snippet hook, which we may want to adjust (and document).

[smark-1]

@dkirkham It seems like you don't have any specific use cases for the before_copy_snippet and after_copy_snippet hooks and I can't think of any use cases for this myself. If there is a rare use case someone should be able to override the copy_view_class to provide themselves with the desired functionality.

[dkirkham]

Hi @smark-1, I don't have any specific use cases, but I could imagine there might be some, for instance to prepare the data being copied before it is presented to editors. The argument for inclusion is for consistency with the existing hooks eg. https://docs.wagtail.org/en/stable/reference/hooks.html#snippets and the Page copy hooks.

[lb-]

Thank you @smark-1 & @dkirkham - this feature works great.

I tested the default and disabled behaviour, tested user permissions and a few other smoke tests of creation and could not find any issues.

I think the scope of this is small and a really great improvement to the Model/Snippet ViewSet functionality.

It would be great to see the ability to override just the copy form class more easily and maybe review the hooks but they can be raised as new issues if the community feels there is value there.

As noted on the original issue, it might be nice to update the page titles for the copy view but that too can be done as a future improvement.

I have rebased and made a few final touches on the PR (below) and will push and check the CI all runs.

1. Consistently add copy code/attributes/docs above the inspect, various parts of the changes had this below or above the inspect part. Just trying to be consistent.
2. Add basic documentation for this feature in the relevant snippets/generic viewset pages.
3. Clean up a few code line breaks / white space changes that were not needed.
4. Add release notes considerations.