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

Mark fields for non extraction #6158

Merged
merged 25 commits into from
Feb 25, 2021

Conversation

KalobTaulien
Copy link
Contributor

Closes #6031

This needs a thorough review by @TheoChevalier — I'm not 100% confident I did the right thing with the override_translatable_fields

Pomax
Pomax previously requested changes Feb 3, 2021
network-api/networkapi/wagtailpages/pagemodels/base.py Outdated Show resolved Hide resolved
network-api/networkapi/wagtailpages/pagemodels/base.py Outdated Show resolved Hide resolved
network-api/networkapi/wagtailpages/pagemodels/base.py Outdated Show resolved Hide resolved
@Pomax Pomax temporarily deployed to wagtail-loca-6031-markf-dann7j February 8, 2021 18:31 Inactive
@mofodevops mofodevops temporarily deployed to foundation-s-6031-markf-1jz4qh February 8, 2021 18:31 Inactive
@Pomax Pomax temporarily deployed to wagtail-loca-6031-markf-f6v67z February 8, 2021 18:31 Inactive
Pomax and others added 6 commits February 8, 2021 10:33
* 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
@mofodevops mofodevops temporarily deployed to foundation-s-6031-markf-1jz4qh February 8, 2021 18:34 Inactive
@Pomax Pomax temporarily deployed to wagtail-loca-6031-markf-dann7j February 8, 2021 18:34 Inactive
@Pomax Pomax temporarily deployed to wagtail-loca-6031-markf-dann7j February 8, 2021 18:36 Inactive
@mofodevops mofodevops temporarily deployed to foundation-s-6031-markf-1jz4qh February 8, 2021 18:36 Inactive
@mofodevops mofodevops temporarily deployed to foundation-s-6031-markf-1jz4qh February 8, 2021 18:41 Inactive
@Pomax Pomax temporarily deployed to wagtail-loca-6031-markf-dann7j February 8, 2021 18:41 Inactive
Copy link
Contributor

@Pomax Pomax 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 fine to me but I'm going to defer to @TheoChevalier here.

@TheoChevalier
Copy link
Contributor

Hmm, I’m getting this error when running an inv new-env… Will have a look at the code, but it’s a bit more hard to make sure I’m not forgetting something

Capture d’écran 2021-02-18 à 12 18 02

Pomax and others added 10 commits February 18, 2021 09:30
* 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
@KalobTaulien KalobTaulien force-pushed the 6031-markfields-for-non-extraction branch from 9113ee5 to ba54dad Compare February 18, 2021 17:40
@KalobTaulien
Copy link
Contributor Author

@TheoChevalier migrations are fixed, should be working fine now. Had to do some migration surgery. Should be able to run inv new-env now.

@TheoChevalier
Copy link
Contributor

Thanks @KalobTaulien, working now! I’m testing PNI, and I caught a few things:

  • The slug is not synchronized (tested on General product)
  • We will need to synchronize a few more fields:
    • company
    • phone_number
    • live_chat
    • email
    • twitter
  • The Privacy Policy link label is not translated

@KalobTaulien
Copy link
Contributor Author

KalobTaulien commented Feb 22, 2021

Hey @TheoChevalier thanks for catching that. I changed those to SynchronizedFields. You can see the diff in the commit here d72eade

@TheoChevalier
Copy link
Contributor

@KalobTaulien Thanks, the changes are looking good, but we still need to make this label translatable and the URL field synchronized:

Capture d’écran 2021-02-25 à 16 25 56

@KalobTaulien
Copy link
Contributor Author

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 ?

@TheoChevalier
Copy link
Contributor

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

Thanks, but that doesn’t seem to work unfortunately… I can’t manage to have these fields appear in the French page

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 ?

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?

Copy link
Contributor

@TheoChevalier TheoChevalier left a 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

@KalobTaulien
Copy link
Contributor Author

Perfect, thanks @TheoChevalier! I opened #6257

@KalobTaulien KalobTaulien merged commit b305848 into wagtail-localize Feb 25, 2021
@KalobTaulien KalobTaulien deleted the 6031-markfields-for-non-extraction branch February 25, 2021 18:15
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.

4 participants