-
Notifications
You must be signed in to change notification settings - Fork 153
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
Mark fields for non extraction #6158
Mark fields for non extraction #6158
Conversation
* removing modeltranslation + migrations
* rebase migration fix rogue migration file in bg rebase migration fix rogue migration file in bg rogue migration file in bg migrations + removing modeltranslation from requirements rebase synchronizefield code * Update network-api/networkapi/wagtailpages/pagemodels/products.py
60aa5f1
to
7dc0363
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me but I'm going to defer to @TheoChevalier here.
network-api/networkapi/wagtailpages/pagemodels/dear_internet.py
Outdated
Show resolved
Hide resolved
* removing modeltranslation + migrations
* rebase migration fix rogue migration file in bg rebase migration fix rogue migration file in bg rogue migration file in bg migrations + removing modeltranslation from requirements rebase synchronizefield code * Update network-api/networkapi/wagtailpages/pagemodels/products.py
9113ee5
to
ba54dad
Compare
@TheoChevalier migrations are fixed, should be working fine now. Had to do some migration surgery. Should be able to run |
Thanks @KalobTaulien, working now! I’m testing PNI, and I caught a few things:
|
Hey @TheoChevalier thanks for catching that. I changed those to SynchronizedFields. You can see the diff in the commit here d72eade |
8bc423c
to
5b6beb9
Compare
@KalobTaulien Thanks, the changes are looking good, but we still need to make this label translatable and the URL field synchronized: |
Hey @TheoChevalier Yep, I had one field but not the other in that model. I added them here: https://github.com/mozilla/foundation.mozilla.org/pull/6158/files#diff-4edb38b0ab78223309e545e5d159d907247579d18df094053b9b495fbecb82d8R167-R170 I had to go through some merge conflicts, and Im a tad worried I got the translation fields backwards for a couple. May require another full review. I also think we should work on closing this ticket ASAP because the database migrations are leaking into this branch causing painful migration conflicts (and soaking up time that it doesn't need to). Let's work on closing this ticket and opening new smaller tickets if we need them. Thoughts on that @TheoChevalier ? |
Thanks, but that doesn’t seem to work unfortunately… I can’t manage to have these fields appear in the French page
Yeah, totally fine with me, we can work on the privacy policy labels in a follow up ticket. Also if there’s any adjustment needed to these fields, I’ll definitely catch those during the content migration, so let’s merge this I guess? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
R+, with the privacy policy labels issue being moved to a separate ticket
Perfect, thanks @TheoChevalier! I opened #6257 |
Closes #6031
This needs a thorough review by @TheoChevalier — I'm not 100% confident I did the right thing with the
override_translatable_fields