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 sitemap generation #14481

Merged
merged 2 commits into from
Apr 19, 2024
Merged

Conversation

stevejalim
Copy link
Collaborator

Since the refactor to use Django's i18n mechanism in #14256, not our original Prefixer, resolved URLs are automatically prepended with settings.LANGUAGE_CODE, which means the update_sitemaps management command (used by www-sitemap-generator) creates a sitemap.json containing paths prefixed with en-US/, which downstream use of this data for sitemap generation is not expecting.

The simplest fix was to drop that part of the path.

Issue / Bugzilla link

Resolves #14480

Testing

Tricky to test end-to-end, or to write meaningful tests for, but I have extracted three examples of the sitemap.json file, showing that this changeset generates an identical (except for ordering of array elements) version of the data:

  • main currently generates this: broken_sitemap.json (36090 lines and en-US prefixing every path)
  • The commit immediately to the i18n-refactor merge (b54650a) generates this: original_sitemap.json (15146 lines, with no lang-code prefix on paths)
  • The fixed version of the sitemap.json looks like this fixed_sitemap.json (15146 lines and no lang-code prefix on paths)

…mitted in sitemap.json

...so that we don't break www-sitemap-generator when it then prepends various locales to the URLs

Fixes mozilla#14480
@stevejalim stevejalim requested a review from pmac April 19, 2024 12:07
Copy link

codecov bot commented Apr 19, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 76.44%. Comparing base (d82f824) to head (52a63e0).
Report is 1 commits behind head on main.

Files Patch % Lines
bedrock/sitemaps/utils.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14481      +/-   ##
==========================================
- Coverage   76.46%   76.44%   -0.02%     
==========================================
  Files         148      148              
  Lines        7975     7977       +2     
==========================================
  Hits         6098     6098              
- Misses       1877     1879       +2     

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

Copy link
Member

@pmac pmac left a comment

Choose a reason for hiding this comment

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

🎂

@stevejalim stevejalim merged commit 64ce2a4 into mozilla:main Apr 19, 2024
5 checks passed
@stevejalim stevejalim deleted the 14480-fix-sitemap-update branch April 19, 2024 14:14
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.

Sitemap generation fails after refactor to use Django's i18n logic
2 participants