Main Menu Template Tags #85

sanjeevz3009 · 2025-01-22T19:13:03Z

What is the context of this PR?

This is a follow-up PR to the #41.

This PR introduces template tags for the main menu, by removing front-end logic. By separating concerns, this ensures templates are focused on presentation while business logic remains in Python. This improves readability and testability, simplifying the template structure. Additionally, we will have a more maintainable code by reducing complexity and allowing for performance optimisations when the page is rendered.

How to review

Test the main menu per norm as instructed in #41 description. Everything should work normally when adding a main menu -> highlights -> columns - > sections -> topics.

⚠️ This PR will be merged with main once the #41 gets merged to main.

Follow-up Actions

List any follow-up actions (if applicable), like needed documentation updates or additional testing.

zerolab · 2025-01-23T09:22:10Z

cms/navigation/tests/test_navigation_tags.py

+        for column in columns:
+            self.assertIn("column", column)
+            self.assertIn("linksList", column)
+            self.assertIsInstance(column["linksList"], list)
+
+            for section in column["linksList"]:
+                self.assertIn("text", section)
+                self.assertIn("url", section)
+                self.assertIn("children", section)
+                self.assertIsInstance(section["children"], list)
+
+                for child in section["children"]:
+                    self.assertIn("text", child)
+                    self.assertIn("url", child)


Can you do the full check? that is
self.assertListEqual(columns, [...]) where [...] is the expected output.

Ditto for highlights

Done! I think I implemented what you envisioned!

@zerolab https://github.com/ONSdigital/dis-wagtail/pull/85/files#diff-bf6f55e1f68b6b5da79c34faa58386f8c997b00aa2637c7f2ac20207f9898a99R51

nehakerung

LGTM - works as expected

RealOrangeOne · 2025-01-28T15:30:44Z

cms/navigation/jinja2tags.py

+    """Extends the Jinja functionality with additional Django, Wagtail,
+    and other package template tags.
+    """


Suggested change

"""Extends the Jinja functionality with additional Django, Wagtail,

and other package template tags.

"""

"""Extends Jinja templates with what's needed to render the navigation.

"""

Docstrings updated!

RealOrangeOne · 2025-01-28T15:31:43Z

cms/navigation/templatetags/navigation_tags.py

+    return highlights
+
+
+def _extract_url_item(value: "StructValue", request: Optional["HttpRequest"] = None) -> LinkItem:


Nit: This method looks almost identical to _extract_highlight_item - could the logic be shared?

Refactored and now there is one single function with shared logic.

RealOrangeOne · 2025-01-28T15:36:48Z

cms/navigation/tests/test_navigation_tags.py

+class MainMenuTemplateTagTests(TestCase):
+    @classmethod
+    def setUpTestData(cls):
+        cls.mock_request = Mock()


Suggestion: wagtail.coreutils.get_dummy_request or django.test.RequestFactory would be better here, so make sure it looks request-like 🦆

RealOrangeOne · 2025-01-28T15:38:49Z

cms/navigation/templatetags/navigation_tags.py

+    from cms.navigation.models import MainMenu
+
+
+register = template.Library()


Issue: This appears to be unused

Good spot. Removed!

ababic · 2025-01-28T15:57:36Z

cms/navigation/templatetags/navigation_tags.py

+    linksList: list["LinkItem"]
+
+
+def _extract_highlight_item(value: "StructValue", request: Optional["HttpRequest"] = None) -> dict[str, str]:


@sanjeevz3009 Might it be simpler/easier to define this sort of thing as a method on the model class?

@ababic Made some changes from Jake's comments above and now I have _extract_item. Do you still think it may be worth adding _extract_item to the model class?

The base branch was changed.

cms/core/blocks/tests/factories.py

helenb

Thanks @sanjeevz3009 I have functionally tested and it works well. Added one query to the template.

cms/jinja2/templates/base.html

…navigation_template_tags added and navigation_tags refactored.

* Front-end hacked together to complete the back-end * Navigation app added, models created for navigation settings * Navigation bar on the front-end test * Front-end code reverted due to the front-end main menu being done separately * New line * Code formatted * Type hinted and code formatted * Code refactored, abstract link class created, validation messages added, section restricted to 3, URL label added, icon for navigation settings added and restricting it to so only 1 main menu can be created * Migration file * Main menu modelling complete, restricting user to be able to only create one main menu instance in the wagtail admin. Migration file added. * LinkBlock extracted out of core.blocks.related and moved to core.blocks.base to be reusable across the project. LinkBlock from core.blocks.base is being used instead of the abstract class originally defined in models.py. * Code formatted, zombie code removed, unused imports removed and MainMenuViewSet added to restrict creating more than one main menu instance * Front-end integrated * Type hints added * Nav template issues fixed * gettext_lazy added * Preview added * Use the title provided by the user over the default title from the page * help text update * Type hints added * Migration file regenerated * Tests added * TODO removed and translatable added * Description help text updated * Tests setup * Example StreamField value setting in test_highlights_streamfield_limit * Debug * Errors being raised when adding more than 3 highlights * Committing working factories to save * Clean method for ColumnBlock added and tests updated * Updated tests * test_section_streamfield_limit test working * Added more tests, factories updated * Removed tests * Max num code removed * Code formatted * Utilising pageurl to point to get internal wagtail pages * Code refactored * Reverting format for md files * Reverting md files formatting * Lint errors corrected * Unused import removed and base template updated * Base template update * Type checking added back in * Template code updated to make sure to only display live wagtail pages * Main menu live preview fixed * MegaLinter errors fixed * Using a preview template for live previews * Mark NavigationSettings as allowed for write in ExternalEnvRouter * Use our own BaseSiteSetting which works with the ExternalEnvRouter * RevisionMixin implementation and mypy fix * Blocks refactor * Code linted * Tests for main menu viewset added * Type ignore added * Duplicate page and external URL validation for sections and topic links within columns complete. * Duplicate page and URL for columns complete. * test_forms in place * Update test_clean_highlights_no_duplicate test * Tests complete for forms.py * test_forms.py code clean up * Example usage zombie code removed * Code formatted and refactored * Code refactored, type hinted, tests refactored. * gettext translation added * LinkBlock Factory moved * De-nest the blocks tests folder

…ion doc strings updated and unused code in navigation_tags file removed.

…ble function

zerolab changed the base branch from main to main-menu-CMS-169 January 22, 2025 20:13

zerolab reviewed Jan 23, 2025

View reviewed changes

nehakerung previously approved these changes Jan 23, 2025

View reviewed changes

RealOrangeOne mentioned this pull request Jan 28, 2025

Main Menu #41

Merged

RealOrangeOne reviewed Jan 28, 2025

View reviewed changes

ababic reviewed Jan 28, 2025

View reviewed changes

Base automatically changed from main-menu-CMS-169 to main January 29, 2025 10:17

sanjeevz3009 marked this pull request as ready for review January 29, 2025 11:30

sanjeevz3009 requested a review from a team as a code owner January 29, 2025 11:30

zerolab reviewed Jan 29, 2025

View reviewed changes

cms/core/blocks/tests/factories.py Outdated Show resolved Hide resolved

helenb reviewed Jan 30, 2025

View reviewed changes

cms/jinja2/templates/base.html Show resolved Hide resolved

sanjeevz3009 and others added 11 commits January 30, 2025 23:41

Navigation templates tags added

b5a2898

base template updated to use navigation template tags functionality, …

97439c5

…navigation_template_tags added and navigation_tags refactored.

Code formatted

799f6d2

navigation tags tests improved

abcc3c4

Type hints issues fixed

a44fff4

Tests folder removed containing redundant factories, NavigationExtens…

5c50a91

…ion doc strings updated and unused code in navigation_tags file removed.

Unused code removed.

d27d42d

_extract_highlight_item and _extract_url_item combined into one reusa…

84ee4d9

…ble function

Type hinting fixed

c354b97

Using get_dummy_quest from wagtail for mocking requests

f17986e

sanjeevz3009 force-pushed the main-menu-CMS-template-tags-298 branch from 6f19227 to f17986e Compare January 30, 2025 23:43

Accidental files added, now removed

4dee24e

helenb mentioned this pull request Jan 31, 2025

Try out the statistical article hero wtih the hero component from the design system #91

Draft

sanjeevz3009 requested review from zerolab, RealOrangeOne, helenb and ababic January 31, 2025 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Main Menu Template Tags #85

Main Menu Template Tags #85

sanjeevz3009 commented Jan 22, 2025 •

edited

Loading

zerolab Jan 23, 2025

sanjeevz3009 Jan 24, 2025

sanjeevz3009 Jan 29, 2025

nehakerung left a comment

RealOrangeOne Jan 28, 2025

sanjeevz3009 Jan 29, 2025

RealOrangeOne Jan 28, 2025

sanjeevz3009 Jan 30, 2025

RealOrangeOne Jan 28, 2025

sanjeevz3009 Jan 30, 2025

RealOrangeOne Jan 28, 2025

sanjeevz3009 Jan 29, 2025 •

edited

Loading

ababic Jan 28, 2025

sanjeevz3009 Jan 30, 2025 •

edited

Loading

helenb left a comment

		return highlights


		def _extract_url_item(value: "StructValue", request: Optional["HttpRequest"] = None) -> LinkItem:

		from cms.navigation.models import MainMenu


		register = template.Library()

		linksList: list["LinkItem"]


		def _extract_highlight_item(value: "StructValue", request: Optional["HttpRequest"] = None) -> dict[str, str]:

Main Menu Template Tags #85

Are you sure you want to change the base?

Main Menu Template Tags #85

Conversation

sanjeevz3009 commented Jan 22, 2025 • edited Loading

What is the context of this PR?

How to review

Follow-up Actions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nehakerung left a 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

sanjeevz3009 Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanjeevz3009 Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

helenb left a comment

Choose a reason for hiding this comment

sanjeevz3009 commented Jan 22, 2025 •

edited

Loading

sanjeevz3009 Jan 29, 2025 •

edited

Loading

sanjeevz3009 Jan 30, 2025 •

edited

Loading