Migrate VPN resource center to Wagtail (#14860)

[alexgibson]

One-line summary

Migrates VPN Resource Center index and detail pages to Wagtail.

Significant changes and points to review

One thing we might have to think about is how we plan to roll these pages out once this merges. This will be the first time we're testing localization through Smartling, and we need to be careful not to temporarily "lose" any content in the process.

Also, there were a few things in the old Contentful pages that had code but were never implemented (such as related article categories). I'm not going to port those over now, seeing as they never made it to production?

Issue / Bugzilla link

#14860

Testing

1. Run make preflight
2. Run ./manage.py makemigrations and then ./manage.py migrate
3. Run npm start.
4. Sign into the CMS: http://localhost:8000/cms-admin/
5. Publish a new Structural page called Mozilla Products with a slug products.
6. Add a child page (another Structural page) called Mozilla VPN with a slug vpn, and hit publish.
7. Navigate to the /vpn/ structural page and add a child page of type Vpn resource center index page. Make sure it has a slug of resource-center.
8. Add a title, sub title, and a call to action snippet (replicating https://www.mozilla.org/en-US/products/vpn/resource-center/ is a good test).
9. Add a few child pages of type Vpn resource center detail page (this should be the only child type that's allowed by the parent).
10. Re-create some detail / article pages from https://www.mozilla.org/en-US/products/vpn/resource-center/.
11. If you create a Call to action bottom snippet, it should only show up when you have more than 6 child articles.

---

Review TODOs for @stevejalim

• Check the above
• [non blocking] Raise issue about auto-translation of subtrees failing -> Page tree problems after translating including subtree wagtail/wagtail-localize#773
• [non blocking] Raise issue about translation UI Translation UI opens additional, possibly unrelated, input and scrolls to it wagtail/wagtail-localize#833
• Test translation of pages into more than one locale
  • Fix issue where prefer_cms falls back to CMS-based en-US page even though other locales exist for that path outside the CMS
  • [non blocking] Test Smartling integration with real-world content
    • String concatenation problems with strings pulled back from Smartling wagtail-localize-smartling#31
• Deal with untranslated pages being Published immediately, rather than remaining drafts -> ticketed Stop auto-publishing of new locale variants when a page is first sent to translation #15345
• Add animated GIF support for Wagtail-managed images #15330

[codecov]

Codecov Report

Attention: Patch coverage is 98.38710% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.55%. Comparing base (9371f6f) to head (6b9193a).

Files with missing lines	Patch %	Lines
bedrock/products/models.py	97.82%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15236      +/-   ##
==========================================
+ Coverage   78.40%   78.55%   +0.15%     
==========================================
  Files         157      158       +1     
  Lines        8195     8257      +62     
==========================================
+ Hits         6425     6486      +61     
- Misses       1770     1771       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[alexgibson]

Note that prefer_cms might not be working quite right here, since I no longer see the list of existing Fluent translations in the language selector once published, and all locales then redirect to wagtail published en-US page.

[stevejalim]

Yeah i think prefer_cms needs to be able to be locale-aware, and I suspect it's not right now. Will take a look as part of reviewing this PR

[stevejalim]

Also, there were a few things in the old Contentful pages that had code but were never implemented (such as related article categories). I'm not going to port those over now, seeing as they never made it to production?

Yeah, I agree - no need to add stuff we're not using (yet?)

[stevejalim]

This will be the first time we're testing localization through Smartling, and we need to be careful not to temporarily "lose" any content in the process.

If we can get the prefer_cms decorator to work, I'd expect that we can roll out en-US pages, publish those and then have them served from the CMS while other locales fall back to the regular/current Django view, then as translations flow back in, the CMS takes over from the Django-powered pages. But that does depend on prefer_cms doing what we need it to do, obvs

[alexgibson]

If we can get the prefer_cms decorator to work, I'd expect that we can roll out en-US pages, publish those and then have them served from the CMS while other locales fall back to the regular/current Django view, then as translations flow back in, the CMS takes over from the Django-powered pages. But that does depend on prefer_cms doing what we need it to do, obvs

This sounds like a good plan to me 👍

[stevejalim]

From the instructions

2. Run ./manage.py makemigrations and then ./manage.py migrate

We shouldn't need to make any migrations, just run the ones in the branch.

[stevejalim]

Commenting now for discussion of the Snippet stuff before I test drive, but looking really good IMO.

I'll try out fixing up the prefer_cms decorator now and see how I go, but wanted to bounce the comments your way in the meantime

[stevejalim]

Notes from test driving

• We should consider falling back to page.title if page.seo_title is not available for the <title> element

• Child page selection needs filtering to be the same locale as the index page - change suggested in a comment

• I can create a new CTA Snippet when none exists, but I can't create addtional ones directly in the Snippets section of the Wagtail Admin - that's odd. Is there an extra permission we need to add somewhere? (I can help with this, but just writing it down now)

[alexgibson]

I can create a new CTA Snippet when none exists, but I can't create addtional ones directly in the Snippets section of the Wagtail Admin - that's odd. Is there an extra permission we need to add somewhere? (I can help with this, but just writing it down now)

I see this behavior too. Maybe it's intended, since those snippets need to be associated with a page model when they are created?

[alexgibson]

@stevejalim thanks for the review so far. Pushed a commit with some fixes

[stevejalim]

I see this behavior too. Maybe it's intended, since those snippets need to be associated with a page model when they are created?

The problem is that the current workflow only then allows the creation of one snippet ever - once one exists in the system, the user is prompted to use that, rather than make another. Plus it should be possible to change the snippet associated with a part of page if we want to

[alexgibson]

The problem is that the current workflow only then allows the creation of one snippet ever - once one exists in the system, the user is prompted to use that, rather than make another. Plus it should be possible to change the snippet associated with a part of page if we want to

Hmm, I don't see this behavior - I'm pretty sure I created two snippets?

Edit: no you're correct - something has changed as now I can only create one as well :/

[alexgibson]

Oh, so it is still possible but it's a little non-intuitive. You need to drill down into the snippet type:

[alexgibson]

@stevejalim one other thing I think we might still need to take care of here, is when publishing a page in the CMS, it seems to override active_locales so only the languages published in the CMS turn up in the language selector? I think that might create a problem when it comes to migrating existing articles, since as soon as we publish en-US in the CMS, all the existing translated articles become unreachable?

[stevejalim]

@stevejalim one other thing I think we might still need to take care of here, is when publishing a page in the CMS, it seems to override active_locales so only the languages published in the CMS turn up in the language selector? I think that might create a problem when it comes to migrating existing articles, since as soon as we publish en-US in the CMS, all the existing translated articles become unreachable?

So yeah, this is how things used to work with the Contentful-powered version - and, indeed, across all Wagtail-powered pages now -- we work out what locales a page is available in based on the actual translations in the CMS - see here

I'm about to test whether the prefer_cms decorator will fall back to Django views for, say, /fr/products/vbn/what-is-a-vpn/ while still serving /en-US/products/vbn/what-is-a-vpn/ from the CMS, plus the same for the index page.
If that's all ok, then that would mean the only downside is the lack of the language selector working. Howeverrrrrr, looking at https://www.mozilla.org/en-US/products/vpn/resource-center/ right now, it seems that the selector is not showing for the static/Contentful/Django-only version of the VPN RC right now anyway -- I suspect that's related to the proportion of translated contnet compared to the en-US versions and that it slipped below the active locales threshold

bedrock/bedrock/products/views.py

Lines 225 to 228 in 029f88e

	active_locales = locales_with_available_content(
	classification=CONTENT_CLASSIFICATION_VPN,
	content_type=CONTENT_TYPE_PAGE_RESOURCE_CENTER,
	)

bedrock/bedrock/contentful/utils.py

Line 13 in 029f88e

def locales_with_available_content(

bedrock/bedrock/settings/base.py

Lines 1077 to 1081 in 029f88e

	CONTENTFUL_LOCALE_SUFFICIENT_CONTENT_PERCENTAGE = config(
	"CONTENTFUL_LOCALE_SUFFICIENT_CONTENT_PERCENTAGE",
	default="1" if DEV is True else "60",
	parser=float,
	)

It might be that dropping that threshold will reinstate the lang picker, but that's a tangent to the CMS work.

So right now, given this, plus the fact we have the localized content available and we'll be able to get it into Smartling relatively quickly, I think it might not be a problem.

What do you think?

[stevejalim]

Put another way, the other locales are currently also unreachable via the lang picker from the index page. (Though the lang picker does work on an individual page, which would break when we add en-US, you're 100% right on that)

[alexgibson]

I'm about to test whether the prefer_cms decorator will fall back to Django views for, say, /fr/products/vbn/what-is-a-vpn/ while still serving /en-US/products/vbn/what-is-a-vpn/ from the CMS, plus the same for the index page.

My testing seems to indicate that all previous translations are disabled, and redirected to en-US. So it's not just an issue with the language selector.

So right now, given this, plus the fact we have the localized content available and we'll be able to get it into Smartling relatively quickly, I think it might not be a problem.

I think we should prioritize getting this into Wagtail as opposed to fixing existing issues in the contentful content, yeah.

[stevejalim]

Feeding back from local testing so far:

• if we use ./manage.py fixtree after translating a subtree, then things work as we expect and I don't get the odd 404 issue we had before

  • I'm hoping the need to use fixtree will be gone soon as Torchbox is working on a fix to wagtail-localize - Page tree problems after translating including subtree wagtail/wagtail-localize#773

• I've come across a different problem with doing non-Smartling translations - the UI opens the wrong input when Edit is clicked - ie, editing Field C triggers the input for Field Q. Have reported this and will get back -> Translation UI opens additional, possibly unrelated, input and scrolls to it wagtail/wagtail-localize#833

• Translating snippets works but need some UX touches (which I'll make locally and push up)

  • We should set the verbose_name and verbose_name_plural fields on the the VPNCallToActionSnippet so that the capitalisation is appropriate -> see 41ef99a
  • The snippet list can be filtered by locale to help navigate it, but it'd be nice to see the locale name alongside the snippet title in the list -> see ac07555

[stevejalim]

I'm about to test whether the prefer_cms decorator will fall back to Django views for, say, /fr/products/vbn/what-is-a-vpn/ while still serving /en-US/products/vbn/what-is-a-vpn/ from the CMS, plus the same for the index page.

My testing seems to indicate that all previous translations are disabled, and redirected to en-US. So it's not just an issue with the language selector.

Ah, yes, I can reproduce that. I will see if a tweak to prefer_cms will let us fall back to locales that the Django version does know about. I think it's because right now prefer_cms strips the locale and lets Wagtail infer it from Accept-Language, but really we should keep it there if it came in as a request.

So right now, given this, plus the fact we have the localized content available and we'll be able to get it into Smartling relatively quickly, I think it might not be a problem.

I think we should prioritize getting this into Wagtail as opposed to fixing existing issues in the contentful content, yeah.

Agreed

[stevejalim]

FYI I've just tested submitting the VPN RC index page (featuring two separate Snippets) to Smartling and it worked, making one job for the main page and one job for each of the Snippets 👍

[stevejalim]

After more test driving and tweaking of the l10n side, this is a good-for-now r+ from me.

The core Wagtail work is solid - the issues that have cropped up are all related to l10n and the workflows around it, which are grouped in #15348

As long as we're happy with the updated/fixed behaviour of the prefer_cms decorator we can definitely merge this now.

If we use it for the en-US version of the VPN RC, the only downside will be that non-en-US loacles will not be reachable from the en-US version because the locale picker either only looks at CMS pages or only looks at Django pages, but neither looks at the other.