Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #17030 +/- ##
==========================================
+ Coverage 81.42% 81.51% +0.09%
==========================================
Files 171 172 +1
Lines 9140 9188 +48
==========================================
+ Hits 7442 7490 +48
Misses 1698 1698 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9b75e4e to
9bbce98
Compare
e9a7976 to
6ad83e7
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new CMS-managed “media springboard” block for the homepage, moving the previously hardcoded springboard include into CMS content blocks.
Changes:
- Introduces
SpringboardBlock/SpringboardItemBlock(plus settings) and wires it intoHomePageStreamField. - Adds a new CMS block template for rendering the springboard section.
- Adds fixtures, factories, and tests to validate springboard rendering and behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| bedrock/mozorg/blocks/common.py | Adds Springboard blocks and tweaks existing block icons. |
| bedrock/mozorg/models.py | Registers springboard_block on HomePage StreamField. |
| bedrock/mozorg/migrations/0025_alter_homepage_content.py | StreamField migration to include springboard block. |
| bedrock/mozorg/templates/mozorg/cms/blocks/springboard_block.html | New template for CMS-rendered springboard section. |
| bedrock/base/templates/macros-m24.html | Updates springboard_item macro to mark link attributes as safe. |
| bedrock/mozorg/templates/mozorg/cms/home/home.html | Removes hardcoded media-springboard include (now expected via CMS). |
| bedrock/mozorg/templates/mozorg/home/includes/m24/media-springboard.html | Removes unused macro import. |
| bedrock/mozorg/fixtures/springboard_fixtures.py | Adds fixtures to generate springboard variants and a test page. |
| bedrock/mozorg/tests/factories.py | Adds Wagtail factories for springboard blocks/items. |
| bedrock/mozorg/tests/test_blocks.py | Adds test coverage for springboard rendering/content/attributes. |
| for variant in variants: | ||
| anchor_id = variant["value"].get("settings", {}).get("anchor_id") | ||
| if anchor_id: | ||
| # Find section by anchor ID | ||
| section = soup.find("section", id=anchor_id) | ||
| assert section is not None, f"Section with id '{anchor_id}' not found" | ||
| assert_springboard_block_attributes(section, variant) |
There was a problem hiding this comment.
This test only asserts the positive case (anchor_id present). Since assert_springboard_block_attributes also defines expectations for the empty-anchor case, add an assertion for variants where anchor_id is empty to verify the section does not render an id attribute (or otherwise validate the expected behavior).
|
|
||
| # Check if test page already exists | ||
| test_page = HomePage.objects.filter(slug="springboard-block-test").first() | ||
| if test_page: |
There was a problem hiding this comment.
Returning an existing page without updating its content can make tests brittle when fixture variants change (stale DB state means tests may not reflect the current variant set). Consider overwriting test_page.content with the latest variants (and republishing), or deleting/recreating the page deterministically so test data is always in sync.
| if test_page: | |
| if test_page: | |
| # Ensure existing test page content stays in sync with current variants | |
| test_page.content = variants | |
| test_page.save_revision().publish() |
stevejalim
left a comment
There was a problem hiding this comment.
Looking very promising @wen-2018 - thanks for this!
I've added some comments and also expanded some of the thing Copilot talks about - take a look and see what you think. Happy to re-review one the tweaks are done.
Also, am I right that this page isn't used at all in prod yet (not even Stephanie's earlier cut)? If so, we'll have to plan and practise a rollout, because swapping the homepage of a Wagtail site isn't always straightforward
| choices=[ | ||
| ("Article", "Article"), | ||
| ("Podcast", "Podcast"), | ||
| ("Video", "Video"), |
There was a problem hiding this comment.
The values and they keys here all start with a capital letter (Video) whereas below, for the icon field, they are lowercase keys. Can we make it consistent? The Django convention is lowercase keys with underscores for spaces.
There was a problem hiding this comment.
The lower case for icon fields are used to set the CSS class for different custom icons. The Capitals for topic and type here are used as string options displayed on the page directly
There was a problem hiding this comment.
We should only display the second item / the valye in the page, not the first / the key - is that easy to make happen?
There was a problem hiding this comment.
I don't think it accepts a single value. They must come it key - value tuples. https://docs.djangoproject.com/en/6.0/ref/models/fields/#choices
Co-authored-by: Steve Jalim <stevejalim@mozilla.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
efbc417 to
508c9dd
Compare
There was a problem hiding this comment.
This file was removed (changed, renamed, and relocated) in a different PR, likely by mistake. Adding it back in.
That's correct! This page is not used in prod yet. There is one more section (showcase section, currently with state of Mozilla as the content) to be added in CMS. The hero and prod section are not planned to be added in CMS since they rarely change. Once this PR and the PR for showcase section are merged, we could plan out how the switch works:
|
If this changeset needs to go into the FXC codebase, please add the
WMO and FXClabel.One-line summary
This PR adds CMS blocks for homepage media springboard section.
Significant changes and points to review
Issue / Bugzilla link
WT-659
Testing
home-2026slug