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

Move VPN Resource Center images from Contentful CDN to Bedrock media #14198

Merged
merged 5 commits into from
Mar 19, 2024

Conversation

stevejalim
Copy link
Collaborator

@stevejalim stevejalim commented Feb 9, 2024

Do not merge for now.

One-line summary

This changeset moves the VPN RC away from using images live-hosted by Contentful.

It:

  • adds all of the images, in the appropriate size, from the Contentful CDN to our bedrock codebase
  • contains a data migration that, when run, updates the contentful_contentfulentry table appropriately. (It swaps the src and/or srcset attributes of img tags from full Contentful CDN URLs to local image paths instead, matching the files added to the codebase.
  • EDIT: stops the regular syncing of Contentful content, else a change (or a forced update on Dev or a Demo) would overwrite the changes to use the local files

Significant changes and points to review

The data migration is not pretty, but should make sense. Please call out anything that looks too strange.

Issue / Bugzilla link

No ticket

Testing

Locally:

  • on main run ./bin/run-db-download.py --force
  • switch to this branch
  • ./manage.py migrate which should apply one migration (0007) in the contentful app
  • Go to http://localhost:8000/en-US/products/vpn/resource-center/
    • Confirm there are no missing images, broken HTML, etc and that images are loaded from localhost not https://images.ctfassets.net
    • View each child page and confirm all OK and loading local images - having the Network tab in Devtools helps
    • When a child page contains an image, use devtools to drop the DPR from 2 to 1 and confirm via the Network tab that the lower-res image is loaded, and that it's locally sourced

@stevejalim stevejalim requested review from robhudson and pmac February 9, 2024 12:09
Copy link

codecov bot commented Feb 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.10%. Comparing base (7acd730) to head (10a6262).

❗ Current head 10a6262 differs from pull request most recent head e48c310. Consider uploading reports for the commit e48c310 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14198      +/-   ##
==========================================
+ Coverage   76.60%   77.10%   +0.50%     
==========================================
  Files         144      145       +1     
  Lines        7818     7894      +76     
==========================================
+ Hits         5989     6087      +98     
+ Misses       1829     1807      -22     

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

@stevejalim stevejalim added Needs Review Awaiting code review Review: S Code review time: 30 mins to 1 hour labels Feb 9, 2024
Copy link
Member

@alexgibson alexgibson left a comment

Choose a reason for hiding this comment

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

I tested this locally and things seem to work as expected 👍

I did notice a lot of the images are quite large, and we can save between 60% and 80% in file size by running things through tinypng. It would be nice to do that before merging?

@stevejalim stevejalim marked this pull request as draft February 9, 2024 14:44
@stevejalim stevejalim force-pushed the move-from-contentful-cdn-to-mozilla-statics branch 2 times, most recently from f95d42b to c5a29c1 Compare February 9, 2024 15:42
@stevejalim stevejalim marked this pull request as ready for review February 9, 2024 15:51
@stevejalim stevejalim requested a review from alexgibson February 9, 2024 15:54
@stevejalim stevejalim force-pushed the move-from-contentful-cdn-to-mozilla-statics branch from 642af99 to 88d155b Compare February 9, 2024 17:23
@stevejalim
Copy link
Collaborator Author

@alexgibson Images optimised in 88d155b - thanks for the reminder!

@stevejalim stevejalim marked this pull request as draft February 9, 2024 21:33
@stevejalim stevejalim marked this pull request as draft February 9, 2024 21:33
@stevejalim stevejalim marked this pull request as ready for review February 9, 2024 22:14
@stevejalim
Copy link
Collaborator Author

Marking this as Do Not Merge, but could still do with a final review. It's ready to roll, but we may as well leave it until later in case we need to edit any copy. If we change images, this PR would need updating, but not copy changes.

Copy link
Member

@alexgibson alexgibson left a comment

Choose a reason for hiding this comment

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

r++

…nstead

Some of the filenames are not very elegant, but they match what we are
using in the templates, which draw data fro the DB, so if we rename them
as files, here we should rename them in the DB too. They will only be around
for a relatively short time as we move them back to a CMS before long.
…e VPN Resource Center contentful pages, so that they match the renamed files just added to the codebase
Now we're serving images from Bedrock's statics, and we know we're not
adding new content, we want to turn off the sync so that it doesn't rewrite
the HTML back to using the Contentful CDN
@stevejalim stevejalim force-pushed the move-from-contentful-cdn-to-mozilla-statics branch from 10a6262 to e48c310 Compare March 19, 2024 13:53
@stevejalim stevejalim merged commit 19f25f7 into main Mar 19, 2024
3 checks passed
@stevejalim stevejalim deleted the move-from-contentful-cdn-to-mozilla-statics branch March 19, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: S Code review time: 30 mins to 1 hour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants