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

Generic Index Page #78

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Generic Index Page #78

wants to merge 24 commits into from

Conversation

kacperpONS
Copy link
Contributor

@kacperpONS kacperpONS commented Jan 16, 2025

What is the context of this PR?

Note that the Front End will differ from the current About us page that's given on the Jira card as an example of an Index page; that includes but isn't limited to the fact that the list is displayed vertically.
I've talked to Helen and John D and it appears that the design for the Index page hasn't been finalised yet, so it's likely it'll have to be revisited.

Because there's no design to refer to and the Jira card doesn't specify that, I've decided give the publisher the flexibility to add Featured pages with or without the description field.
That might cause some formatting issues when some of the block will and some won't have the description.
In order to prevent that the backend might have to be modified to get rid of this problem by making the description mandatory, making the size of the Featured items consistent.

Similarly, when child pages are displayed as the Featured items, they'll currently be displayed without the description field. Depending on the design, this might have to be changed for example by allowing the publisher to add a description for the child pages or automatically pull information from the child page summary.

How to review

Command to the unit tests just for this page:

poetry run coverage run ./manage.py test --parallel --settings=cms.settings.test cms.standard_pages.tests.test_models --keepdb
  1. For Functional testing, make sure that the new "Featured items" functionality works:

    • Check that if no "Featured items" are selected, the children pages of the Index page are displayed,
    • Check that if at least one "Featured item" is selected, then it's displayed correctly
  2. The page also uses the RelatedContentBlock in a new context - so far it was only used as one of the StreamFields available in the main content of pages. Here - it's used on it's own and as rendered as a separate onsRelatedContent component the Design System. Worth double checking that it renders correctly :)

Jira card

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

@zerolab zerolab changed the title Generic Index Page CMS-180 Generic Index Page Jan 23, 2025
@kacperpONS
Copy link
Contributor Author

Just a note - the linters just made me format the automatically generated migrations file, might be a sign our linters are a tad too restrictive...

@kacperpONS kacperpONS marked this pull request as ready for review January 23, 2025 17:11
@kacperpONS kacperpONS requested a review from a team as a code owner January 23, 2025 17:11
model = LinkBlock

title = factory.Faker("text", max_nb_chars=20)
page = factory.Maybe(factory.SubFactory(PageChooserBlockFactory), None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at https://factoryboy.readthedocs.io/en/stable/reference.html#maybe, this is not quite there. factory.Maybe takes a boolean decider, yes declaration, no declaration.

So you probably want something like https://factoryboy.readthedocs.io/en/stable/reference.html#traits which enables you to have calls like LinkBlockFactory(with_page=True)

I would default the factory to set page to None, but the provide a trait that would set it. something like

class LinkBlockFactory(StructBlockFactory):
    """Factory for LinkBlock."""

    class Meta:
        model = LinkBlock

    title = factory.Faker("text", max_nb_chars=20)
    page = None
    external_url = factory.Faker("url")

    class Params:
        with_page = factory.Trait(
            page = factory.SubFactory(PageChooserBlockFactory),
            external_url = None
        )

Copy link
Contributor Author

@kacperpONS kacperpONS Jan 24, 2025

Choose a reason for hiding this comment

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

@sanjeevz3009 I think that's used in your #41 (Main menu), might be worth updating that in your branch for consistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will do. It's a small change and let's keep it consistent =) Thanks Kacper for this. Good stuff!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@sanjeevz3009 sanjeevz3009 Jan 24, 2025

Choose a reason for hiding this comment

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

I will be raising a new PR off the main menu PR to address this as I don't want to make any more new commits unless it's a pressing/ crucial change I get on there since it's being reviewed by a few people as we speak.

Or when this PR goes in before or after. As if the main menu goes in, we can address this as part of this PR.

I do realise I got my LinkBlockFactory in cms/core/blocks/tests/factories.py. You got it in cms/core/tests/factories.py.

The decision for this was due to LinkBlockFactory sitting in cms/core/blocks/base.py which was extracted out of related.py after @zerolab and I decided on this. So I have the LinkBlockFactory in factories.py within a tests folder.

We should discuss or finalise where the LinkBlockFactory should sit. I am not too picky around this as these are nit picks at this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sanjeevz3009 please de-nest that. I was hoping others would review your PR, but will have to take it on Monday morning.

the tests and factories need to be at the app level (so cms/core/tests/)

Comment on lines 12 to 18
{# fmt:off #}
{{
onsDocumentList({
"documents": formatted_featured_pages
})
}}
{# fmt:on #}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{# fmt:off #}
{{
onsDocumentList({
"documents": formatted_featured_pages
})
}}
{# fmt:on #}
{{ onsDocumentList({"documents": formatted_featured_pages}) }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

class IndexPage(BasePage): # type: ignore[django-manager-missing]
template = "templates/pages/index_page.html"

parent_page_types: ClassVar[list[str]] = ["home.HomePage", "IndexPage", "InformationPage"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
parent_page_types: ClassVar[list[str]] = ["home.HomePage", "IndexPage", "InformationPage"]
parent_page_types: ClassVar[list[str]] = ["home.HomePage", "IndexPage"]

I don't think we should allow generic index pages under information pages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

subpage_types: ClassVar[list[str]] = ["IndexPage", "InformationPage"]

description = models.TextField(blank=True)
featured_pages = StreamField([("featured_page", RelatedContentBlock())], blank=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

given we allow both pages and external links, this is not really featured_pages, no? The dev notes indicate calling this featured_items.

Copy link
Contributor

Choose a reason for hiding this comment

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

additionally, this could use some help_text that indicates that populating this field will stop the index from automatically showing a list of child pages"

Copy link
Contributor Author

Choose a reason for hiding this comment

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


search_fields: ClassVar[list[index.SearchField | index.AutocompleteField]] = [
*BasePage.search_fields,
index.SearchField("description"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
index.SearchField("description"),
index.SearchField("description"),
index.SearchField("content"),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

index.SearchField("description"),
]

def get_formatted_featured_pages(self) -> list[dict[str, str]]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def get_formatted_featured_pages(self) -> list[dict[str, str]]:
def get_formatted_items(self, request: "HttpRequest") -> list[dict[str, str]]:

note: we pass the request object to use in deriving the child page URLs - https://docs.wagtail.org/en/stable/advanced_topics/performance.html#page-urls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

def get_context(self, request: "HttpRequest", *args: Any, **kwargs: Any) -> dict:
context: dict = super().get_context(request, *args, **kwargs)

context["formatted_featured_pages"] = self.get_formatted_featured_pages()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
context["formatted_featured_pages"] = self.get_formatted_featured_pages()
context["formatted_items"] = self.get_formatted_items(request)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"""
child_page = InformationPageFactory(parent=self.page)

self.page.save_revision().publish()
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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


self.assertEqual(response.status_code, 200)

featured_page_html = f"""
Copy link
Contributor

Choose a reason for hiding this comment

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

this is brittle in that any changes to the DS component will result in the tests failing. I would rather test that the links are there in the content

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to test that all the 3 keys (title, url and description) that are returned from get_formatted_items() are on the page, because what this test is effectively doing is testing the funciton. Lmk what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


formatted_related_links = self.page.get_formatted_related_links_list()

self.assertEqual(formatted_related_links, [{"title": "An external page", "url": "external-url.com"}])
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs a test with @override_settings(IS_EXTERNAL_ENV=True) that checks we get a 200 for this page

    @override_settings(IS_EXTERNAL_ENV=True)
    def test_load_in_external_env(self):
        """Test the page loads in external env."""
        response = self.client.get(self.page_url)
        self.assertEqual(response.status_code, 200)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

3 participants