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

Convert to alias #515

Merged
merged 10 commits into from
Feb 17, 2022
Merged

Convert to alias #515

merged 10 commits into from
Feb 17, 2022

Conversation

zerolab
Copy link
Collaborator

@zerolab zerolab commented Feb 7, 2022

This PR fixes #466 by adding functionality to convert translations (translated pages, to be more exact) into aliases

It also adds a wagtail_localize.convert_to_alias PageLogEntry action, and its own icon.

Demo: https://youtu.be/N6iVvq4sqjU

@zerolab zerolab requested a review from kaedroho February 7, 2022 17:24
@zerolab zerolab force-pushed the feature/translated-page-back-to-alias branch from a8c9161 to 25ffc81 Compare February 7, 2022 18:02
@zerolab zerolab added this to the 1.1 milestone Feb 8, 2022
Copy link
Contributor

@kaedroho kaedroho 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 really good! Just some minor tweaks and a couple of questions

wagtail_localize/views/convert.py Show resolved Hide resolved
next_url = get_valid_next_url_from_request(request)

if request.method == "POST":
page.alias_of_id = source_page_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we synchronise the content of the source page at this point?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

probably worth it, will check

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The consensus is that ye, we should synchronise with the source page at this point

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tl;dr
Tried with Translation.save_target() but that is very much dependent on the translatable_fields definition or lack thereof.

Resorted to a tweaked copy of Page.update_alias

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, do you think we could get those tweaks rolled back in to Wagtail core?

I'm happy with this change anyhow

wagtail_localize/views/convert.py Outdated Show resolved Hide resolved
wagtail_localize/views/convert.py Outdated Show resolved Hide resolved
wagtail_localize/wagtail_hooks.py Outdated Show resolved Hide resolved
@zerolab zerolab requested a review from kaedroho February 11, 2022 10:31
@codecov-commenter
Copy link

codecov-commenter commented Feb 11, 2022

Codecov Report

Merging #515 (ae28a5b) into main (58d3231) will decrease coverage by 0.16%.
The diff coverage is 68.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #515      +/-   ##
==========================================
- Coverage   91.72%   91.56%   -0.17%     
==========================================
  Files          36       37       +1     
  Lines        3081     3200     +119     
  Branches      494      513      +19     
==========================================
+ Hits         2826     2930     +104     
- Misses        145      154       +9     
- Partials      110      116       +6     
Impacted Files Coverage Δ
wagtail_localize/wagtail_hooks.py 80.11% <61.22%> (-4.74%) ⬇️
wagtail_localize/views/edit_translation.py 88.01% <66.66%> (-0.29%) ⬇️
wagtail_localize/views/convert.py 74.60% <74.60%> (ø)
wagtail_localize/components.py 85.56% <0.00%> (ø)
wagtail_localize/locales/views.py 93.37% <0.00%> (ø)
wagtail_localize/views/submit_translations.py 94.07% <0.00%> (+0.24%) ⬆️
wagtail_localize/segments/extract.py 87.50% <0.00%> (+5.88%) ⬆️
wagtail_localize/segments/ingest.py 81.25% <0.00%> (+6.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58d3231...ae28a5b. Read the comment docs.

@zerolab zerolab force-pushed the feature/translated-page-back-to-alias branch from 7d97ecd to ae28a5b Compare February 11, 2022 10:34
Copy link
Contributor

@kaedroho kaedroho left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@zerolab zerolab merged commit cf3cfed into main Feb 17, 2022
@zerolab zerolab deleted the feature/translated-page-back-to-alias branch February 17, 2022 15:11
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.

Revert a translated page back to an alias page
4 participants