Add Monitor article pages to CMS #15751 by stephaniehobson · Pull Request #16084 · mozilla/bedrock

stephaniehobson · 2025-03-13T23:25:42Z

One-line summary

Add Monitor article pages to CMS

Significant changes and points to review

Add wagtail_richtext filter to help style markup in articles
- Adds appropriate Protocol class to ul and ols
- Adds an id to headings
- Adds a space around em dashes
- Adds rel and target attributes to external links
Add Monitor landing and article page models
Add Monitor landing and article page templates
- Including a sub nav
- Including a sticky table of contents

Issue / Bugzilla link

#15751

Testing

CMS

Auth into the CMS
Descend a directory into Mozilla Products (this should be included in local db)
Create a new Monitor article index page, with a slug of monitor. Fill in the other required fields.
Publish the index page.

Check intro button link works
Check page displays properly

Edit the index page and fill in all fields

Check page displays properly

Create any number of Monitor article page. (Content is in New Product Introduction Page: Monitor #15751 if you want real data) Testing the following:

Articles appear on the index page
Article icons display at 40px on the index page regardless of image selected
On article pages OL and UL lists are formatted
On article pages external links have rel and target defined
Article pages function well with or without defined CTAs
The table of contents is correctly assembled from any h2 tags in the content field

HTML & CSS

Note: this is English only content

page title & favicon
Sub navigation
Breadcrumbs
Sticky menu sticks if there is enough space
Sticky menu does not stick if prefers-reduced-motion: reduce

bedrock/products/migrations/0005_monitorcalltoactionsnippet_monitorarticleindexpage_and_more.py

stevejalim · 2025-03-18T15:51:53Z

Does this need the WMO and FXC label? I don't think it's heading to Springfield @stephaniehobson

bedrock/products/models.py

docs/cms.rst

bedrock/mozorg/templates/mozorg/cms/about/blocks/leadership_block.html

bedrock/base/templatetags/helpers.py

codecov · 2025-03-18T17:25:12Z

Codecov Report

Attention: Patch coverage is 96.29630% with 3 lines in your changes missing coverage. Please review.

Project coverage is 79.70%. Comparing base (29d944d) to head (a138aaa).
Report is 11 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16084      +/-   ##
==========================================
+ Coverage   79.55%   79.70%   +0.15%     
==========================================
  Files         160      160              
  Lines        8436     8514      +78     
==========================================
+ Hits         6711     6786      +75     
- Misses       1725     1728       +3

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

stephaniehobson · 2025-03-18T22:36:39Z

Does this need the WMO and FXC label? I don't think it's heading to Springfield @stephaniehobson

Yes, I think the richtext changes will benefit springfield

bedrock/products/models.py

bedrock/products/templates/products/monitor/cms/article.html

bedrock/products/urls.py

stevejalim

Spotted a couple of things to fix up - happy to re-review later, or to chat about them

stevejalim

Backend/Wagtail-focused review: r+wc - nothing blocking

I'll leave an FE dev to test drive themselves, too

Nice one @stephaniehobson!

bedrock/base/templatetags/helpers.py

stevejalim · 2025-03-21T22:44:49Z

bedrock/base/tests/test_helpers.py

+            </body>
+        </html>
+        """
+        expected_html = """


This is fine/workable as is, but you mentioned in Slack how it made you a bit 🤢 -- so you may find Python's textwrap.dedent to be handy, but don't feel like you have to shoehorn that in right now

It did not immediately work so I'll leave it for a future improvement.

bedrock/cms/cms_only_urls.py

stevejalim · 2025-03-21T22:48:40Z

bedrock/products/models.py

+        max_length=60,
+        blank=True,
+    )
+    split_content = RichTextField(null=True, blank=True, features=["bold", "italic", "link", "ul", "ol"])


Nit: we should move this features list in to a setting eg RICHTEXT_FEATURES_MINIMAL

stevejalim · 2025-03-21T22:49:48Z

bedrock/products/models.py

+
+    parent_page_types = ["MonitorArticleIndexPage"]  # must be child of MonitorArticleIndexPage
+
+    def get_context(self, request):


If we can't add a test for this now, please feel free to ticket up adding one and pop my face on it. Happy to help

alexgibson

When I create a new Monitor article index page and fill out the required fields, I get the following error:


Traceback (most recent call last):
  File "/Users/alexgibson/.pyenv/versions/3.12.6/envs/bedrock/lib/python3.12/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexgibson/.pyenv/versions/3.12.6/envs/bedrock/lib/python3.12/site-packages/wagtail/models/__init__.py", line 717, in _get_response
    response = obj.serve_preview(request, preview_mode)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexgibson/Documents/GitHub/bedrock/bedrock/cms/models/base.py", line 88, in serve_preview
    return self._render_with_fluent_string_support(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexgibson/Documents/GitHub/bedrock/bedrock/cms/models/base.py", line 69, in _render_with_fluent_string_support
    context = self.get_context(request, *args, **kwargs)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Exception Type: TypeError at /en-US/products/monitor/
Exception Value: MonitorArticleIndexPage.get_context() takes 2 positional arguments but 3 were given

alexgibson

It would also be nice to have some Wagtail specific tests for the new models, to make sure they render content in the templates as expected?

stephaniehobson · 2025-03-25T22:22:17Z

I can't reproduce the error but I made a change that will fix it hopefully 🤞

stevejalim · 2025-03-26T08:04:06Z

When I create a new Monitor article index page and fill out the required fields, I get the following error:


Traceback (most recent call last):
  File "/Users/alexgibson/.pyenv/versions/3.12.6/envs/bedrock/lib/python3.12/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexgibson/.pyenv/versions/3.12.6/envs/bedrock/lib/python3.12/site-packages/wagtail/models/__init__.py", line 717, in _get_response
    response = obj.serve_preview(request, preview_mode)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexgibson/Documents/GitHub/bedrock/bedrock/cms/models/base.py", line 88, in serve_preview
    return self._render_with_fluent_string_support(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexgibson/Documents/GitHub/bedrock/bedrock/cms/models/base.py", line 69, in _render_with_fluent_string_support
    context = self.get_context(request, *args, **kwargs)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Exception Type: TypeError at /en-US/products/monitor/
Exception Value: MonitorArticleIndexPage.get_context() takes 2 positional arguments but 3 were given

My gut said that this was triggered by Alex using the live preview, which would pass an extra argument to get_context(). This is what happens when I try live preview, too:

So the fix here is to fully match the signature for get_context() whenever we override/extend it.

I've done that in 69477ef and pushed it up.

alexgibson · 2025-03-26T09:26:04Z

So the fix here is to fully match the signature for get_context() whenever we override/extend it.

This fixed the issue for me ^

alexgibson

A couple of small issues / suggestions but otherwise everything works as described. nice work!

bedrock/products/models.py

bedrock/products/templates/products/monitor/cms/article.html

- Add Monitor landing and article page models - Add Monitor landing and article page templates - Including a sub nav - Including a sticky table of contents - Add wagtail_richtext filter to help style markup in articles - Adds appropriate Protocol class to ul and ols - Adds an id to headings - Adds a space around em dashes - Adds rel and target attributes to external links

...so that we don't end up changing the spec of the wagtail.models.Page class, which in turn generates a migration that we cannot (and should not) add to the codebase

…DEV=False

- Add Monitor landing and article page models - Add Monitor landing and article page templates - Including a sub nav - Including a sticky table of contents - Add wagtail_richtext filter to help style markup in articles - Adds appropriate Protocol class to ul and ols - Adds an id to headings - Adds a space around em dashes - Adds rel and target attributes to external links

…gtail expedts ...and that calls to super().get_context() also pass those through to the superclass (eg wagtail.Page) This fixes a blow-up with Monitor pages when being live previewed, and also tightens up existing pages that use get_context

stephaniehobson · 2025-03-27T04:10:15Z

bedrock/products/tests/factories.py


-    sub_title = wagtail_factories.CharBlockFactory
+    articles_heading = wagtail_factories.CharBlockFactory
+    split_heading = wagtail_factories.CharBlockFactory


This is throwing an error because it's too long {'split_heading': ['Ensure this value has at most 60 characters (it has 69).']}. Is there a way to give the restriction to the Factory? Or should I hard code something?

You can stop the tests blowing up by setting a hard-coded value for the CharBlockFactory

split_heading = wagtail_factories.CharBlockFactory(value="Test split heading")

There are a couple of places where this would need adding.

However, I don't think we should fix the tests like that.

I think (and wish I'd thought it sooner) that having a 60-char limit (or any hard limit) on strings that will get translated could easily bite us later when they flow back from Smartling, because:

To my understandingwagtail-localize doesn't enforce a characarter limit

Therefore nor does our wagtail-localize-smartling integration

I've not seen anything in Smartling itself that limits the length of translated strings, either

I think all of those above are the right design decision, because the Finnish, say, for a word is likely to be longer than the English source in some cases.

If we have a 60 char limit and the translated version is 61, the Wagtail<->Smartling sync will definitely break because the translated field is too long.

I think instead the better, more robust path is to remove the hard limit on the headings and add help_text="Please aim for no more than 60 characters maximum" or similar to each field.

I know there's a risk of translations creating bad flows/line breaks, but the translators in Smartling do get a live view of the content they are translating, including Snippets.

Hope that helps. DM me if you need me and time is tight

Updated to remove the character limits temporarily. We should discuss how to have some kind of limit, at least in English but that can wait.

stephaniehobson · 2025-03-27T22:39:24Z

Updated!

alexgibson

r+

* Add Monitor article pages to CMS - Add Monitor landing and article page models - Add Monitor landing and article page templates - Including a sub nav - Including a sticky table of contents - Add wagtail_richtext filter to help style markup in articles - Adds appropriate Protocol class to ul and ols - Adds an id to headings - Adds a space around em dashes - Adds rel and target attributes to external links - Add new Monitor CMS pages to the allowlist so they can be added when DEV=False - add test for add_bedrock_attributes helper - review fixes - fix get_context error? - Ensure that get_context() signatures on our page models match what Wagtail expedts - ...and that calls to super().get_context() also pass those through to the superclass (eg wagtail.Page) - This fixes a blow-up with Monitor pages when being live previewed, and also tightens up existing pages that use get_context - Add tests --------- Co-authored-by: Steve Jalim <stevejalim@mozilla.com> Co-authored-by: Stephanie Hobson <shobson@mozilla.com>

stevejalim reviewed Mar 17, 2025

View reviewed changes

bedrock/products/migrations/0005_monitorcalltoactionsnippet_monitorarticleindexpage_and_more.py Outdated Show resolved Hide resolved

stephaniehobson force-pushed the monitor-plus branch 2 times, most recently from 41d21d2 to ddc31d5 Compare March 17, 2025 21:43

stephaniehobson added the WMO and FXC Code relevant to both mozilla/bedrock (www.mozilla.org) and mozmeao/springfield (www.firefox.com) label Mar 17, 2025

stephaniehobson mentioned this pull request Mar 17, 2025

New Product Introduction Page: Monitor #15751

Closed

2 tasks

stephaniehobson force-pushed the monitor-plus branch 2 times, most recently from 3968083 to adaf3ac Compare March 17, 2025 23:04

stevejalim reviewed Mar 18, 2025

View reviewed changes

bedrock/products/models.py Outdated Show resolved Hide resolved

stevejalim reviewed Mar 18, 2025

View reviewed changes

docs/cms.rst Show resolved Hide resolved

stevejalim reviewed Mar 18, 2025

View reviewed changes

bedrock/mozorg/templates/mozorg/cms/about/blocks/leadership_block.html Outdated Show resolved Hide resolved

stevejalim reviewed Mar 18, 2025

View reviewed changes

bedrock/base/templatetags/helpers.py Outdated Show resolved Hide resolved

stephaniehobson force-pushed the monitor-plus branch from 0bcec71 to 0b0e03a Compare March 19, 2025 02:58

stephaniehobson commented Mar 19, 2025

View reviewed changes

bedrock/products/models.py Outdated Show resolved Hide resolved

maureenlholland added the Wagtail Development related to our use of Wagtail CMS label Mar 19, 2025

stephaniehobson force-pushed the monitor-plus branch 2 times, most recently from d6de04c to d5b44b0 Compare March 19, 2025 18:56

stevejalim reviewed Mar 21, 2025

View reviewed changes

bedrock/products/templates/products/monitor/cms/article.html Outdated Show resolved Hide resolved

stevejalim reviewed Mar 21, 2025

View reviewed changes

bedrock/products/urls.py Outdated Show resolved Hide resolved

stevejalim requested changes Mar 21, 2025

View reviewed changes

stephaniehobson marked this pull request as ready for review March 21, 2025 17:18

stephaniehobson requested review from a team as code owners March 21, 2025 17:18

stephaniehobson assigned alexgibson Mar 21, 2025

stephaniehobson force-pushed the monitor-plus branch 2 times, most recently from 465fd48 to 2e2bbc8 Compare March 21, 2025 18:04

stevejalim approved these changes Mar 21, 2025

View reviewed changes

alexgibson reviewed Mar 25, 2025

View reviewed changes

alexgibson suggested changes Mar 26, 2025

View reviewed changes

bedrock/products/models.py Show resolved Hide resolved

bedrock/products/models.py Outdated Show resolved Hide resolved

bedrock/products/templates/products/monitor/cms/article.html Outdated Show resolved Hide resolved

stephaniehobson and others added 11 commits March 26, 2025 21:08

Use a different approach to patching the help text of the title field

b6e9583

...so that we don't end up changing the spec of the wagtail.models.Page class, which in turn generates a migration that we cannot (and should not) add to the codebase

Add new Monitor CMS pages to the allowlist so they can be added when …

5207b30

…DEV=False

remove dependancy from migration

6ff4104

rename filter

5d92f63

review fixes

f57d673

add test for add_bedrock_attributes helper

ee72f43

review fixes

dee4984

fix get_context error?

33987c1

stephaniehobson force-pushed the monitor-plus branch from 69477ef to 7c60676 Compare March 27, 2025 04:08

stephaniehobson commented Mar 27, 2025

View reviewed changes

stephaniehobson force-pushed the monitor-plus branch 2 times, most recently from 3a5a6e8 to a956389 Compare March 27, 2025 17:21

stephaniehobson added 2 commits March 27, 2025 15:37

Add tests

89f09c3

review fixes

a138aaa

stephaniehobson force-pushed the monitor-plus branch from a956389 to a138aaa Compare March 27, 2025 22:38

alexgibson approved these changes Apr 1, 2025

View reviewed changes

stephaniehobson merged commit 95bd1c6 into main Apr 1, 2025
7 checks passed

stephaniehobson deleted the monitor-plus branch April 1, 2025 15:45

janbrasna mentioned this pull request Apr 1, 2025

Fixup CMS docs #16141

Merged

alexgibson removed the WMO and FXC Code relevant to both mozilla/bedrock (www.mozilla.org) and mozmeao/springfield (www.firefox.com) label May 13, 2025


		parent_page_types = ["MonitorArticleIndexPage"] # must be child of MonitorArticleIndexPage

		def get_context(self, request):

Conversation

stephaniehobson commented Mar 13, 2025 • edited by alexgibson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

One-line summary

Significant changes and points to review

Issue / Bugzilla link

Testing

CMS

HTML & CSS

Uh oh!

Uh oh!

stevejalim commented Mar 18, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

stephaniehobson commented Mar 18, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevejalim left a comment

Choose a reason for hiding this comment

Uh oh!

stevejalim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexgibson left a comment

Choose a reason for hiding this comment

Uh oh!

alexgibson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephaniehobson commented Mar 25, 2025

Uh oh!

stevejalim commented Mar 26, 2025

Uh oh!

alexgibson commented Mar 26, 2025

Uh oh!

alexgibson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephaniehobson commented Mar 27, 2025

Uh oh!

alexgibson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

stephaniehobson commented Mar 13, 2025 •

edited by alexgibson

Loading

codecov bot commented Mar 18, 2025 •

edited

Loading

alexgibson left a comment •

edited

Loading