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

Add Wagtail 6 support + extra way to create a sharing link #1

Merged
merged 29 commits into from
Oct 9, 2024

Conversation

stevejalim
Copy link
Collaborator

@stevejalim stevejalim commented Oct 1, 2024

This changeset adds support for Wagtail 6, with which the current version of wagtaildraftsharing does not work, because of changes to the History view and the switch of some parts of the UI to use Stimulus JS. It also expands the functionality of the app to make it more user-friendly in terms of generating a sharing link.

(Don't worry too much if this is not a familiar project to you - the testing section below should cover it.)

Wagtail 6 support

This changeset mirrors the current functionality for Wagtail 5 in Wagtail 6 by creating a Stimulus JS controller and hooking it into Wagtail in a similar way as done for Wagtail 5.

For the page history view, the HTML templating in Wagtail 6 is different and DOMContentLoaded is no longer a reliable event for attaching custom behaviour to the page. The previous approach overrode wagtailadmin/pages/revisions/_actions.html to slot in custom markup with JS touchpoints. For Wagtail 6 we do something similar, extending wagtailadmin/generic/history/action_cell.html and mark it up in a way that suits a Stimulus controller. This change remains in the mode/spirit of the project while moving in the direction of Wagtail's UI improvements.

Existing sharing links are registered by wagtaildraftsharing as Snippets, and these remain browsable via a dedicated view. This changeset updates the HTML output for the share_url method to hook in the new Stimulus controller. The change is only applicable to Wagtail 6, and the code remains the same for versions below 6.

The JS in the new Stimulus controller is deliberately very similar to the original JS. I wondered about refactoring into a shared module the main duplicated code. However, for now - and with a view potentially to deprecating pre-Wagtail 6 behaviour - I've gone with some repetition rather than DRY code.

Note that we don't load both JS files into the browser - we only load the appropriate JS file based on the version of Wagtail in use.

New functionality

This changeset also adds a new button to the Action Menu (the green combo button/menu thing at the bottom of the page when editing). If the page has a unpublished draft saved, clicking the option will generate a new sharing URL and copy it to the clipboard - just the same as doing it from the history view, but from this place, too. If the page has no unpublished draft, the button is not shown.

Testing

  1. You'll need a project that's using wagtaildraftsharing. (If that means Bedrock - see setup instructions in Add our fork of wagtaildraftsharing to Bedrock mozilla/bedrock#15232 then come back here]. install this branch's code into the active virtualenv for your projeect wtih pip install -U -e git+https://github.com/mozmeao/wagtaildraftsharing.git@add-wagtail-6-support#egg=wagtaildraftsharing

  2. Edit a page in your site in a way you'll easily notice and save the changes as a Draft.

  3. Go to the history view of the page
    history_panel_link

  4. Pick a revision (e.g. the latest one) and click the three-dots menu, then click on 'Copy external sharing URL' - you'll see it change to 'Copied' then back to the original text.
    history_context_menu

  5. You've made a 'draft sharing link' and your clipboard now contains a URL - paste that into a new tab, to confirm it shows you the draft you generated a link to

  6. Try the same in a private tab to prove that the page works without CMS authentication

  7. All sharing links are discoverable via the CMS admin - go to the Draftsharing links panel in the sidebar:
    draftsharing_menu_item

  8. You'll see the sharing link you just made
    sharing_links_snippet_view

  9. You can edit this link here like a regular Django model, including changing its expiry or deleting it - but don't delete it yet.

  10. Clear/flush/put something different in your clipboard buffer. Now click the 'Copy' button for the "Share URL" - this will put the sharing link back into your buffer - a handy way to get an existing sharing link

  11. You can also make new sharing links from the main edit page. Edit your page again and save a new draft. Afterwards, use the 'Create draft sharing links` button in the action menu to create a new sharing link.
    action_menu

  12. Verify that the new link is also listed in the main list of sharing links (step 7)

  13. Return to your test page and publish it, so that there is no unpublished draft for the page. Confirm that the action menu now no longer contains the option to link to a draft.
    action_menu_sharing_not_available

This commit mirrors the current functionality for Wagtail 5 in Wagtail 6 by
creating a Stimulus JS controller and hooking it into Wagtail in a similar
way as for Wagtail 5.

For the page history view, the HTML templating is different, so the previous
approach overrode wagtailadmin/pages/revisions/_actions.html to slot in custom
markup that the JS could hook into. For Wagtail 6 we do something similar,
extending wagtailadmin/generic/history/action_cell.html to slot in a bit of
custom markup.

For snippet-list view, we update the HTML output for the share_url method
to hook in the Stimulus controller used above.

The JS in the Stimulus controller is deliberately very similar to the
original JS. I wondered about refactoring out the main duplicated code
(the main function which triggers the creation of new links via a POST
to the backend, and the function which upgrades the View button on the
list view of existing sharing links to be a clipboard link). However,
for now - and with a view potentially to deprecating pre-Wagtail 6 behaviour
- I've gone with some repetition rather than DRY code.

Note that we don't load both JS files into the browser - we only load
the appropriate one based on the version of Wagtail in use
...if there is an unpublished draft that can be shared
@stevejalim
Copy link
Collaborator Author

Pinging @alexgibson for an early review, both of the code and also the general UX/behaviour

@stevejalim
Copy link
Collaborator Author

cc @KIRA009 - Hi Shohan! Just to keep you in the loop, this is the core of the changes I'm making. I'll be adding more tests etc and would happily submit this branch as a PR for the main project if you want it

Copy link
Member

@alexgibson alexgibson left a comment

Choose a reason for hiding this comment

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

I went through the steps and everything worked for me running bedrock locally just as described. nice work! r+

I don't feel like I can really give a good code-level review of this, as I have no exposure to how this project works, or to Stimulus, and I'm afraid I don't currently have available headspace to start learning it. At a high level, the JS looks good to me, but I'm hesitant to start suggesting changes to it that might not follow the typical conventions for the rest of the project.

@stevejalim
Copy link
Collaborator Author

Thanks for the review and test drive @alexgibson. A high-level look-over the JS was all I was expecting yeah, as Stimulus is new to both of us, but also we're just dipping a toe in, really

incl datetime_now -> timezone_now to avoid mis-assumption about which library it's from
Brings coverage up to 98%, and effectively all code that needs coverage
…was coming back as a naive datetime

So we patch it in the tests and warn about it
Note the tolerance of a should-not-build version python3.12-django3.2-wagtail6.1 - may be there are better ways around that
@stevejalim stevejalim force-pushed the add-wagtail-6-support branch 4 times, most recently from e3e3f6a to a9f0331 Compare October 7, 2024 21:20
"THIRDPARTY",
"FIRSTPARTY",
"LOCALFOLDER"
]

Choose a reason for hiding this comment

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

If you get rid of this you'll want to make sure ruff has the appropriate config for sorting imports, e.g. lint.isort.known-first-party, lint.isort.section-order, and/or lint.isort.sections.django.

… expiry timestamps

Note the addition to the README, requiring the use of settings.USE_TZ=True
@stevejalim stevejalim marked this pull request as ready for review October 8, 2024 09:47
@stevejalim
Copy link
Collaborator Author

@KIRA009 This is ready for a review if you wish

@stevejalim
Copy link
Collaborator Author

@robhudson If you could give this a further look over, that'd be great. Thanks!

@stevejalim stevejalim requested a review from pmac October 9, 2024 09:02
Copy link
Member

@pmac pmac left a comment

Choose a reason for hiding this comment

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

This looks amazing to me. Fantastic work Steve!

@stevejalim stevejalim merged commit 6349819 into main Oct 9, 2024
4 checks passed
@stevejalim stevejalim deleted the add-wagtail-6-support branch October 9, 2024 14:02
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.

4 participants