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

fix 'ready for review'/'published' status bug #43

Merged
merged 4 commits into from
May 17, 2022
Merged

Conversation

chris48s
Copy link
Collaborator

@chris48s chris48s commented May 6, 2022

This PR is a fix for an issue reported on slack.

Basically the issue here is that we were just looking at

  • Is this LanguageCloudFile object imported: Yes/No?
  • Is the target page it is pointing at published: Yes/No?

This meant that as we translated the same page on multiple occasions over time the status for old LanguageCloudFile objects was changing.

This PR attempts to address that by attaching a FK to a PageRevision at the point we import the LanguageCloudFile so we can accurately track whether the changes related to that specific LanguageCloudFile are live or not. I've also added a less specific fallback status to handle some edge cases. The key one being for historic LanguageCloudFile objects that don't have a revision attached to them.

side issue: I seem to have hit the same issue Karl did over in wagtail/wagtail-localize#549 with 0066_collection_management_permissions. I followed the advice from wagtail/wagtail-localize#549 (comment) and just dropped it but it would be useful to understand why Django thinks that needs to be applied first when really it doesn't.

@zerolab
Copy link
Owner

zerolab commented May 6, 2022

Looks like you're using a more recent version of Wagtail which has 0066_collection_management_permissions (2.16, I believe), so Django happily takes that as the latest.. The dark side of supporting loads of versions..

@chris48s
Copy link
Collaborator Author

I've pushed a couple more commits. I think this resolves everything in line with the review comments/slack conversations and also makes the logic a bit easier to understand. Are you able to have another quick look over this @zerolab ? Thanks

Copy link
Owner

@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.

LGTM

@chris48s chris48s merged commit 9aef259 into main May 17, 2022
@zerolab zerolab deleted the publish-status-fix branch May 17, 2022 09:38
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