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

Support Wagtail 6 + add sharing link to Action Menu #3

Open
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

stevejalim
Copy link

@stevejalim stevejalim commented Oct 9, 2024

This PR is based on our fork of the project and I'm very happy to offer our changes back upstream.

The changeset:

  • adds Wagtail 6 / Stimulus JS support to wagtaildraftsharing
  • adds an additional way to generate a sharing link, from the Action Menu at the foot of the page - if there is an unpublished Draft only
  • makes some of the wording around 'draftsharing' customisable, in a way that won't trigger migrations if a project does use non-default wording
  • expands test coverage, bringing things up to ~98% and effectively 100%
  • adds CI using tox and Github Actions
  • sketches in a Release GHA which can be uncommented for use on this upstream repo
  • crops Python 3.8 support (which is now EOL), adds 3.9, 3.10, 3.11 and 3.12 to tox.

For more details, please see the PR on our side: mozmeao#1


Note that this hasn't been tested in production yet, so feel free to wait a few weeks before merging. Equally, if during review you spot something that could be done better, please feel free to mention it!

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
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
No need to have three things going when ruff does the lot
… expiry timestamps

Note the addition to the README, requiring the use of settings.USE_TZ=True
Add Wagtail 6 support + extra way to create a sharing link
@stevejalim
Copy link
Author

@KIRA009 @zerolab FYI

Copy link
Collaborator

@KIRA009 KIRA009 left a comment

Choose a reason for hiding this comment

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

Hi @stevejalim I have just one outstanding question regarding the naming of the settings. Thanks for all the hard work, the tests are really thorough

wagtaildraftsharing/settings.py Outdated Show resolved Hide resolved
Amend settings naming and update docs
…ython-api

Move the link generation out of the view, to make it more reusable from Python
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.

2 participants