wagtail / wagtail-localize

Translation plugin for Wagtail CMS
https://wagtail-localize.org/
Other
222 stars 84 forks source link

Add support for saving revisions of snippets #751

Closed mcmeeking closed 6 months ago

mcmeeking commented 8 months ago

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:

codecov-commenter commented 8 months ago

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 :warning:
wagtail_localize/models.py 92.30% 1 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mcmeeking commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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

zerolab commented 7 months ago

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

zerolab commented 7 months ago

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 commented 6 months ago

@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 commented 6 months ago

Merged in 2773147 (with the removal of the 4.2 conditional

Thank you