Release calendar pages #16

zerolab · 2024-11-01T09:32:06Z

https://jira.ons.gov.uk/browse/CMS-46 and https://jira.ons.gov.uk/browse/CMS-80

Adds the ContactDetails snippet
add the release calendar index and page model and tests
plugs in the templates
adds test coverage
expands the core tests

Screencast: https://www.loom.com/share/5a7d8a9ca004448981526056e28ab5cd
Dev walkthrough: https://www.loom.com/share/39f7bde66cdc48b9872fa3d5cace22d6

https://jira.ons.gov.uk/browse/CMS-80

…igned with djhtml

adds a few extra files to the ignore. and explicitly filters the RemovedInDjango60Warning

RealOrangeOne · 2024-11-01T10:26:02Z

cms/jinja2/templates/components/streamfield/release_date_change_block.html

@@ -0,0 +1,6 @@
+<li class="ons-list__item">
+    <h3 class="ons-u-mb-no ons-u-fs-r--b">{{ _("Previous date") }}</h3>
+    <p>{{ value.previous_date|date("j F Y g:ia") }}</p>


Suggestion: Should this format be defined globally, since I suspect all will be shown in a standard way

RealOrangeOne · 2024-11-01T10:27:42Z

cms/jinja2/templates/pages/release_calendar/release_calendar_page.html

+                </div>
+            {% endif %}
+
+            <p></p>{# added empty p for some margin #}


Nit: This feels like a hack - is there a better way to add some margin? Perhaps with CSS or a <br>?

I am not doing all the front-end here. We have FE tasks for heroes

RealOrangeOne · 2024-11-01T10:28:48Z

cms/jinja2/templates/pages/release_calendar/release_calendar_page.html

+            {% if page.next_release_date or page.next_release_text %}
+                <div class="ons-grid__col ons-col-8@m ons-col-12@s ons-u-p-no@xxs@m">
+                    <span class="ons-u-fs-r--b">{{ _("Next release") }}:</span>
+                    <span class="ons-u-nowrap">{{ page.next_release_date|date("j F Y g:ia") or page.next_release_text }}</span>


Question: How does |date handle receiving None? Might this need to be expanded to an if?

|date returns an empty string for None

RealOrangeOne · 2024-11-01T10:33:58Z

cms/release_calendar/migrations/0002_create_releasecalendarindex.py

+        content_type=index_content_type,
+        depth=home_page.depth + 1,
+        url_path=f"{home_page.url_path}releases/",
+        locale=Locale.objects.first(),  # cannot use Locale.get_default(), but since we only have one, this works


Nit: Worth using .get to enforce there's only one?

Suggested change

locale=Locale.objects.first(), # cannot use Locale.get_default(), but since we only have one, this works

locale=Locale.objects.get(), # cannot use Locale.get_default(), but since we only have one, this works

leaving this so things don't break when add i18n

RealOrangeOne · 2024-11-01T10:35:00Z

cms/release_calendar/models.py

+        blank=True, null=True, help_text=_("Required once the release has been confirmed.")
+    )
+    release_date_text = models.CharField(
+        max_length=50, blank=True, help_text=_("Format: 'Month YYYY', or 'Month YYYY to Month YYYY'.")


Question: Should this format be validated?

good shout.

RealOrangeOne · 2024-11-01T10:40:09Z

cms/release_calendar/tests/test_models.py

+    del release_calendar_page.table_of_contents  # clear the cached property
+    release_calendar_page.is_census = True
+    release_calendar_page.is_accredited = False
+
+    assert release_calendar_page.table_of_contents == expected_with_census_or_accredited
+    assert release_calendar_page.get_context(request)["table_of_contents"] == expected_with_census_or_accredited
+
+    del release_calendar_page.table_of_contents  # clear the cached property
+    release_calendar_page.is_census = False
+    release_calendar_page.is_accredited = True
+


Suggestion: This feels like it's testing a number of different conditions in the same test. It's probably worth sticking to "Arrange Act Assert" where possible.

RealOrangeOne · 2024-11-01T10:42:51Z

cms/settings/base.py

+RICH_TEXT_BASIC = ["bold", "italic", "link", "ol", "ul"]
+RICH_TEXT_FULL = ["h3", "h4", *RICH_TEXT_BASIC, "document-link"]


Question: Why are these separate settings rather than using the editors setting below? Saves needing to import them around.

We have two types of editors - basic for things like notices, summary etc. And full (for paragraph).

We can swap them around.. ie make basic the default and use the full one where needed

RealOrangeOne · 2024-11-01T10:43:14Z

conftest.py

+
+@pytest.fixture()
+def home_page() -> HomePage:
+    """Returns the home page."""


Suggested change

"""Returns the home page."""

"""Returns the home page, which is created via a migration."""

RealOrangeOne · 2024-11-01T10:44:39Z

conftest.py

+@pytest.fixture()
+def release_calendar_index() -> ReleaseCalendarIndex:
+    """Returns the release calendar index page, which is created via a migration."""
+    return ReleaseCalendarIndex.objects.first()


Thought: Currently there's no constraint that only one of these exists.

Should there be a constraint, or

Should the fixture be a factory instead?

There's one index per homepage.

RealOrangeOne · 2024-11-01T10:45:01Z

conftest.py

+@pytest.fixture()
+def home_page() -> HomePage:
+    """Returns the home page."""
+    return HomePage.objects.first()


Nit: Since there's only ever meant to be one homepage

Suggested change

return HomePage.objects.first()

return HomePage.objects.get()

But we won't, since we will have translations

zerolab added 13 commits October 28, 2024 16:54

Add the Contact Details snippet

7df793d

https://jira.ons.gov.uk/browse/CMS-80

Tidy up ForeignKey typing

f29e024

Add a basic table block

6141db8

Add the release calendar app with models

57dbf68

Add the basic templates

cce16a8

Tidy up the basic rich text features

687887a

Add a base form class for easier validation

1212cb1

Add icons to the various panels

00166e4

Add validation logic

d3cec6e

Update templates

9e9d7b1

Update editorconfig so that html files are indented with 4 spaces, al…

c1dfcf4

…igned with djhtml

Programmatically create the release calendar index

086144b

Add tests

96047c2

zerolab requested a review from a team as a code owner November 1, 2024 09:32

zerolab added 2 commits November 1, 2024 09:40

Expand core tests

8481215

Expand coverage configuration

b041e97

adds a few extra files to the ignore. and explicitly filters the RemovedInDjango60Warning

RealOrangeOne reviewed Nov 1, 2024

View reviewed changes

zerolab added 6 commits November 1, 2024 11:06

Placate mypy

004b2f8

Tidy up based on code review

3d8f3d1

Make the accredited / census blocks separate includes

86d16c3

Include changes to release dates when status is confirmed

6d87b73

Expand rendered tests

7ec148e

Allow setting the release date when switching to confirmed

da97d78

zerolab mentioned this pull request Nov 1, 2024

Chore/wagtail 6.3 #17

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release calendar pages #16

Release calendar pages #16

zerolab commented Nov 1, 2024 •

edited

Loading

RealOrangeOne Nov 1, 2024

RealOrangeOne Nov 1, 2024

zerolab Nov 1, 2024

RealOrangeOne Nov 1, 2024

zerolab Nov 1, 2024

RealOrangeOne Nov 1, 2024

zerolab Nov 1, 2024

RealOrangeOne Nov 1, 2024

zerolab Nov 1, 2024

RealOrangeOne Nov 1, 2024

RealOrangeOne Nov 1, 2024

zerolab Nov 1, 2024

RealOrangeOne Nov 1, 2024

RealOrangeOne Nov 1, 2024

zerolab Nov 1, 2024

RealOrangeOne Nov 1, 2024

zerolab Nov 1, 2024

	locale=Locale.objects.first(), # cannot use Locale.get_default(), but since we only have one, this works
	locale=Locale.objects.get(), # cannot use Locale.get_default(), but since we only have one, this works

		RICH_TEXT_BASIC = ["bold", "italic", "link", "ol", "ul"]
		RICH_TEXT_FULL = ["h3", "h4", *RICH_TEXT_BASIC, "document-link"]

	"""Returns the home page."""
	"""Returns the home page, which is created via a migration."""

	return HomePage.objects.first()
	return HomePage.objects.get()

Release calendar pages #16

Are you sure you want to change the base?

Release calendar pages #16

Conversation

zerolab commented Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zerolab commented Nov 1, 2024 •

edited

Loading