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 support for saving revisions of snippets #751

Closed
wants to merge 9 commits into from

Conversation

mcmeeking
Copy link
Contributor

@mcmeeking mcmeeking commented Dec 16, 2023

I'm working on a project which has a need for setting new snippet translations as drafts by default, so I wanted to close #666.

Changes:

  • page_revision field in TranslationLog model is now simply revision.
  • Default action when WAGTAILLOCALIZE_SYNC_LIVE_STATUS_ON_TRANSLATE = False when creating/updating translations for a DraftStateMixin model is to save as a draft.
  • Publishing translated DraftStateMixin models publishes the revision in the same way as publishing a translated Page model (using save_target() rather than just copying the segments).

@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (a3efa8f) 92.60% compared to head (27a4dfa) 92.54%.
Report is 14 commits behind head on main.

Files Patch % Lines
wagtail_localize/views/edit_translation.py 75.00% 2 Missing and 4 partials ⚠️
wagtail_localize/models.py 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #751      +/-   ##
==========================================
- Coverage   92.60%   92.54%   -0.06%     
==========================================
  Files          46       47       +1     
  Lines        4017     4092      +75     
  Branches      598      608      +10     
==========================================
+ Hits         3720     3787      +67     
- Misses        175      178       +3     
- Partials      122      127       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

Hey @mcmeeking,

This is a great start! I think making TestSnippet inherit from DraftStateMixin was a good idea. Made a suggestion to improve the "no draft" model, though.

We need a few tests with drafts / revisions for the test snippet, so as to cover the new logic paths.

And finally, https://www.wagtail-localize.org/how-to/installation/#disabling-default-publication-of-translated-pages could use an update as that is no longer entirely true

wagtail_localize/models.py Outdated Show resolved Hide resolved
wagtail_localize/test/models.py Outdated Show resolved Hide resolved
wagtail_localize/tests/test_edit_translation.py Outdated Show resolved Hide resolved
@mcmeeking mcmeeking requested a review from zerolab January 6, 2024 17:15
docs/how-to/installation.md Outdated Show resolved Hide resolved
wagtail_localize/tests/test_translation_model.py Outdated Show resolved Hide resolved
@mcmeeking mcmeeking marked this pull request as draft January 23, 2024 16:42
@mcmeeking
Copy link
Contributor Author

mcmeeking commented Jan 23, 2024

@zerolab - I've just taken this for a test drive today and it's not right yet, the snippet edit view isn't rendering correctly. This is just an FYI not to merge this until further changes have been pushed.

@mcmeeking mcmeeking marked this pull request as ready for review January 25, 2024 16:34
@mcmeeking
Copy link
Contributor Author

@zerolab - I've just taken this for a test drive today and it's not right yet, the snippet edit view isn't rendering correctly. This is just an FYI not to merge this until further changes have been pushed.

I'm pretty sure this is actually fine, I'm just not sure how to build the JS components when including my fork as a requirements.txt target, so the React components aren't loading on the edit page because /wagtail_localize/js/wagtail-localize.js is returning a 404 for me (since it's not being built).

@zerolab
Copy link
Collaborator

zerolab commented Jan 25, 2024

you can run nvm use, npm ci, npm run build, then pip install -e path/to/local/wagtail-localize locally and you should get all the assets

@zerolab
Copy link
Collaborator

zerolab commented Feb 6, 2024

@mcmeeking the changes so far look quite good. However the translation editing interface doesn't pick up the drafts.

You can test this locally by:

  1. in your repository root run (fnm use or nvm use)
  2. then npm ci and npm run build
  3. edit tox.ini so the [testenv:interactive] section looks like:
[testenv:interactive]
basepython = python3.11

deps =
    Wagtail < 6.0

comands_pre =
# the rest of the current tox.ini below
  1. (pip install tox if you haven't already) and tox -e interactive
  2. go to http://localhost:8020/admin (admin/changeme)
  3. then Snippets > Test snippets and save a draft and play around

@mcmeeking
Copy link
Contributor Author

Thanks for this, I'll try and take a look and sort this week 👍

@zerolab
Copy link
Collaborator

zerolab commented Feb 6, 2024

I will do some more tests tomorrow too (and compare with the page functionality)

@zerolab
Copy link
Collaborator

zerolab commented Feb 7, 2024

Having compared to what happens with pages, I think this does what it says on the tin.
Will re-test with this lens tonight, and likely open a follow up issue to at least document what it means when you have a published page/draftable model with a translation, then you save a draft and sync translation

@mcmeeking
Copy link
Contributor Author

@mcmeeking the changes so far look quite good. However the translation editing interface doesn't pick up the drafts.

Sorry, took a little longer to get around to this than expected - I've taken it for another test drive yesterday following the steps suggested and it appears to be saving draft and allowing publish fine - but I think I see what you're referring to about the UI in that the status in the side panel still states "Live" even when there are new drafts.

From my checks, this seems to be consistent with the Page translation edit page too.

  1. Is that what you were referring to?
  2. Shall I try and fix that in this PR as well?

zerolab pushed a commit that referenced this pull request Mar 8, 2024
@zerolab
Copy link
Collaborator

zerolab commented Mar 8, 2024

Merged in 2773147 (with the removal of the 4.2 conditional

Thank you

@zerolab zerolab closed this Mar 8, 2024
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.

Support for translating snippets in draft mode
3 participants