From bd27bbd40551e2b2b3d63999af83eae5254d9fd2 Mon Sep 17 00:00:00 2001 From: Brylie Christopher Oxley Date: Thu, 13 Jul 2023 10:55:51 +0300 Subject: [PATCH 1/5] Ignore temporary panel fix pending GitHub issue --- magazine/panels.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/magazine/panels.py b/magazine/panels.py index 423040642..c8d7ba3e2 100644 --- a/magazine/panels.py +++ b/magazine/panels.py @@ -8,10 +8,10 @@ class NestedInlinePanel(InlinePanel): Issue: https://github.com/wagtail/wagtail/issues/5126 """ - def widget_overrides(self): - widgets = {} - child_edit_handler = self.get_child_edit_handler() - for handler_class in child_edit_handler.children: - widgets.update(handler_class.widget_overrides()) - widget_overrides = {self.relation_name: widgets} - return widget_overrides + def widget_overrides(self) -> dict: + widgets = {} # pragma: no cover + child_edit_handler = self.get_child_edit_handler() # pragma: no cover + for handler_class in child_edit_handler.children: # pragma: no cover + widgets.update(handler_class.widget_overrides()) # pragma: no cover + widget_overrides = {self.relation_name: widgets} # pragma: no cover + return widget_overrides # pragma: no cover From 3fa0c5db20fa6c0991844926137bf459461a795f Mon Sep 17 00:00:00 2001 From: Brylie Christopher Oxley Date: Thu, 13 Jul 2023 11:49:24 +0300 Subject: [PATCH 2/5] Add type annotations; small refactor --- magazine/models.py | 119 +++++++++++++++++++++++++++++++-------------- 1 file changed, 82 insertions(+), 37 deletions(-) diff --git a/magazine/models.py b/magazine/models.py index 3deb39fdd..7c6588bb2 100644 --- a/magazine/models.py +++ b/magazine/models.py @@ -3,7 +3,10 @@ import arrow from django.core.paginator import EmptyPage, PageNotAnInteger, Paginator +from django.core.paginator import Page as PaginatorPage from django.db import models +from django.db.models import QuerySet +from django.http import HttpRequest from django_flatpickr.widgets import DatePickerInput from modelcluster.contrib.taggit import ClusterTaggableManager # type: ignore from modelcluster.fields import ParentalKey # type: ignore @@ -115,7 +118,7 @@ def get_context(self, request, *args, **kwargs): return context -class MagazineIssue(DrupalFields, Page): +class MagazineIssue(DrupalFields, Page): # type: ignore cover_image = models.ForeignKey( "wagtailimages.Image", on_delete=models.SET_NULL, @@ -131,19 +134,24 @@ class MagazineIssue(DrupalFields, Page): drupal_node_id = models.PositiveIntegerField(null=True, blank=True, db_index=True) @property - def featured_articles(self): + def featured_articles(self) -> list["MagazineArticle"]: # Return a cursor of related articles that are featured return ( MagazineArticle.objects.child_of(self).filter(is_featured=True).specific() ) @property - def publication_end_date(self): + def publication_end_date(self) -> datetime.date | None: + """Return the first day of the month after the publication date. + + NOTE: we can return any day in the following month, + since we only use the year and month components + """ if self.publication_date: # TODO: try to find a cleaner way to add a month to the publication date - # I.e. the 'add a month' approach may be flawed altogether. - # Note: this should probably add more than one month, - # since the magazine is not published monthly + # We add 31 days here since we can't add a month directly + # 31 days is a safe upper bound for adding a month + # since the publication date will be at least 28 days prior return self.publication_date + timedelta(days=+31) return None @@ -162,7 +170,12 @@ class Meta: models.Index(fields=["drupal_node_id"]), ] - def get_context(self, request, *args, **kwargs): + def get_context( + self, + request: HttpRequest, + *args: tuple, + **kwargs: dict, + ) -> dict: context = super().get_context(request) context["articles_by_department"] = ( MagazineArticle.objects.child_of(self).live().order_by("department__title") @@ -170,7 +183,7 @@ def get_context(self, request, *args, **kwargs): return context - def get_sitemap_urls(self): + def get_sitemap_urls(self) -> list[dict]: return [{"location": self.full_url, "lastmod": self.latest_revision_created_at}] @@ -185,7 +198,12 @@ class MagazineArticleTag(TaggedItemBase): class MagazineTagIndexPage(Page): max_count = 1 - def get_context(self, request, *args, **kwargs): + def get_context( + self, + request: HttpRequest, + *args: tuple, + **kwargs: dict, + ) -> dict: tag = request.GET.get("tag") articles = MagazineArticle.objects.filter(tags__name=tag) @@ -200,10 +218,16 @@ class MagazineDepartmentIndexPage(Page): content_panels = Page.content_panels + [FieldPanel("intro")] + parent_page_types = ["MagazineIndexPage"] subpage_types: list[str] = ["MagazineDepartment"] max_count = 1 - def get_context(self, request, *args, **kwargs): + def get_context( + self, + request: HttpRequest, + *args: tuple, + **kwargs: dict, + ) -> dict: departments = MagazineDepartment.objects.all() context = super().get_context(request) @@ -226,15 +250,15 @@ class MagazineDepartment(Page): autocomplete_search_field = "title" # TODO: remove if not using autocomplete - def autocomplete_label(self): + def autocomplete_label(self) -> str: return self.title # TODO: remove if not using autocomplete - def __str__(self): + def __str__(self) -> str: return self.title -class MagazineArticle(DrupalFields, Page): +class MagazineArticle(DrupalFields, Page): # type: ignore teaser = RichTextField( # type: ignore blank=True, help_text="Try to keep teaser to a couple dozen words.", @@ -323,7 +347,7 @@ class MagazineArticle(DrupalFields, Page): parent_page_types = ["MagazineIssue"] subpage_types: list[str] = [] - def get_sitemap_urls(self): + def get_sitemap_urls(self) -> list[dict]: return [ { "location": self.full_url, @@ -333,26 +357,34 @@ def get_sitemap_urls(self): ] @property - def is_public_access(self): + def is_public_access(self) -> bool: """Check whether article should be accessible to all readers or only subscribers based on issue publication date.""" parent_issue = self.get_parent() + # TODO: try to find a good way to shift the date + # without using arrow + # so we can remove the arrow dependency since it is only used here today = arrow.utcnow() six_months_ago = today.shift(months=-6).date() # Issues older than six months are public access - return parent_issue.specific.publication_date <= six_months_ago - - def get_context(self, request, *args, **kwargs): + return parent_issue.specific.publication_date <= six_months_ago # type: ignore + + def get_context( + self, + request: HttpRequest, + *args: tuple, + **kwargs: dict, + ) -> dict: context = super().get_context(request) # Check whether user is subscriber # make sure they are authenticated first, # to avoid checking for "is_subscriber" on anonymous user user_is_subscriber = ( - request.user.is_authenticated and request.user.is_subscriber + request.user.is_authenticated and request.user.is_subscriber # type: ignore ) # Subscribers and superusers can always view full articles @@ -361,7 +393,7 @@ def get_context(self, request, *args, **kwargs): # user can view full article if any of these conditions is True context["user_can_view_full_article"] = ( user_is_subscriber - or request.user.is_superuser + or request.user.is_superuser # type: ignore or self.is_public_access or self.is_featured ) @@ -462,7 +494,7 @@ class Meta: ] -class ArchiveIssue(DrupalFields, Page): +class ArchiveIssue(DrupalFields, Page): # type: ignore publication_date = models.DateField( null=True, help_text="Please select the first day of the publication month", @@ -510,12 +542,15 @@ class DeepArchiveIndexPage(Page): parent_page_types = ["MagazineIndexPage"] subpage_types: list[str] = ["ArchiveIssue"] - def get_publication_years(self): + def get_publication_years(self) -> list[int]: publication_dates = ArchiveIssue.objects.dates("publication_date", "year") return [publication_date.year for publication_date in publication_dates] - def get_filtered_archive_issues(self, request): + def get_filtered_archive_issues( + self, + request: HttpRequest, + ) -> QuerySet[ArchiveIssue]: # Check if any query string is available query = request.GET.dict() @@ -530,32 +565,42 @@ def get_filtered_archive_issues(self, request): return ArchiveIssue.objects.all().filter(**facets) - def get_paginated_archive_issues(self, archive_issues, request): + def get_paginated_archive_issues( + self, + request: HttpRequest, + archive_issues: QuerySet[ArchiveIssue], + ) -> PaginatorPage: items_per_page = 9 paginator = Paginator(archive_issues, items_per_page) archive_issues_page = request.GET.get("page") - try: - paginated_archive_issues = paginator.page(archive_issues_page) - except PageNotAnInteger: - # If page is not an integer, deliver first page. - paginated_archive_issues = paginator.page(1) - except EmptyPage: - # If page is out of range (e.g. 9999), deliver last page of results. - paginated_archive_issues = paginator.page(paginator.num_pages) - - return paginated_archive_issues - - def get_context(self, request, *args, **kwargs): + # Make sure page is numeric and less than or equal to the total pages + if ( + archive_issues_page + and archive_issues_page.isdigit() + and int(archive_issues_page) <= paginator.num_pages + ): + paginator_page_number = int(archive_issues_page) + else: + paginator_page_number = 1 + + return paginator.page(paginator_page_number) + + def get_context( + self, + request: HttpRequest, + *args: tuple, + **kwargs: dict, + ) -> dict: context = super().get_context(request) archive_issues = self.get_filtered_archive_issues(request) paginated_archive_issues = self.get_paginated_archive_issues( - archive_issues, request, + archive_issues, ) context["archive_issues"] = paginated_archive_issues From 8be9eee08f165a68d7937b7b90c2d0943b4fdcc5 Mon Sep 17 00:00:00 2001 From: Brylie Christopher Oxley Date: Thu, 13 Jul 2023 11:56:12 +0300 Subject: [PATCH 3/5] Initial MagazineIssueTest setUp with two tests --- magazine/tests.py | 80 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 1 deletion(-) diff --git a/magazine/tests.py b/magazine/tests.py index a39b155ac..566834ecf 100644 --- a/magazine/tests.py +++ b/magazine/tests.py @@ -1 +1,79 @@ -# Create your tests here. +import datetime +from django.test import TestCase +from wagtail.models import Page, Site +from home.models import HomePage +from .models import ( + MagazineDepartmentIndexPage, + MagazineDepartment, + MagazineIssue, + MagazineIndexPage, + MagazineArticle, +) + + +class MagazineIssueTest(TestCase): + def setUp(self) -> None: + site_root = Page.objects.get(id=2) + + self.home_page = HomePage(title="Home") + site_root.add_child(instance=self.home_page) + + Site.objects.all().update(root_page=self.home_page) + + self.magazine_index = MagazineIndexPage(title="Magazine") + self.home_page.add_child(instance=self.magazine_index) + + self.magazine_issue = MagazineIssue( + title="Issue 1", + issue_number=1, + publication_date="2020-01-01", + ) + + self.magazine_department_index = MagazineDepartmentIndexPage( + title="Departments", + ) + self.magazine_index.add_child(instance=self.magazine_department_index) + self.magazine_department = MagazineDepartment( + title="Department 1", + ) + self.magazine_department_index.add_child(instance=self.magazine_department) + + self.magazine_index.add_child(instance=self.magazine_issue) + self.magazine_article_one = self.magazine_issue.add_child( + instance=MagazineArticle( + title="Article 1", + department=self.magazine_department, + is_featured=True, + ), + ) + self.magazine_article_two = self.magazine_issue.add_child( + instance=MagazineArticle( + title="Article 2", + department=self.magazine_department, + is_featured=False, + ), + ) + + def test_featured_articles(self) -> None: + """Test that the featured_articles property returns the correct + articles.""" + self.assertEqual( + list(self.magazine_issue.featured_articles), + [self.magazine_article_one], + ) + + def test_publication_end_date(self) -> None: + """Test that the publication_end_date property returns the correct + date.""" + self.assertEqual( + self.magazine_issue.publication_end_date, + datetime.date(2020, 2, 1), + ) + + def test_get_context(self) -> None: + # Here you would add tests for the get_context method. + pass + + def test_get_sitemap_urls(self) -> None: + # Here you would add tests for the get_sitemap_urls method. + pass From e2fe6ed298bff37aa0dc5684025d42e30f113bae Mon Sep 17 00:00:00 2001 From: Brylie Christopher Oxley Date: Thu, 13 Jul 2023 12:21:45 +0300 Subject: [PATCH 4/5] Add articles_by_department property --- magazine/models.py | 22 +++++++------------ .../templates/magazine/magazine_issue.html | 2 +- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/magazine/models.py b/magazine/models.py index 7c6588bb2..5789339db 100644 --- a/magazine/models.py +++ b/magazine/models.py @@ -134,12 +134,19 @@ class MagazineIssue(DrupalFields, Page): # type: ignore drupal_node_id = models.PositiveIntegerField(null=True, blank=True, db_index=True) @property - def featured_articles(self) -> list["MagazineArticle"]: + def featured_articles(self) -> QuerySet["MagazineArticle"]: # Return a cursor of related articles that are featured return ( MagazineArticle.objects.child_of(self).filter(is_featured=True).specific() ) + @property + def articles_by_department(self) -> QuerySet["MagazineArticle"]: + # Return a cursor of child articles ordered by department + return ( + MagazineArticle.objects.child_of(self).live().order_by("department__title") + ) + @property def publication_end_date(self) -> datetime.date | None: """Return the first day of the month after the publication date. @@ -170,19 +177,6 @@ class Meta: models.Index(fields=["drupal_node_id"]), ] - def get_context( - self, - request: HttpRequest, - *args: tuple, - **kwargs: dict, - ) -> dict: - context = super().get_context(request) - context["articles_by_department"] = ( - MagazineArticle.objects.child_of(self).live().order_by("department__title") - ) - - return context - def get_sitemap_urls(self) -> list[dict]: return [{"location": self.full_url, "lastmod": self.latest_revision_created_at}] diff --git a/magazine/templates/magazine/magazine_issue.html b/magazine/templates/magazine/magazine_issue.html index df43c8334..f23288be5 100644 --- a/magazine/templates/magazine/magazine_issue.html +++ b/magazine/templates/magazine/magazine_issue.html @@ -31,7 +31,7 @@

Featured Articles

{% endif %} - {% regroup articles_by_department by specific.department as departments %} + {% regroup page.specific.articles_by_department by specific.department as departments %} {% for department in departments %}

From 2ce79d702c7646f3b1481ad30e322eca2e18a251 Mon Sep 17 00:00:00 2001 From: Brylie Christopher Oxley Date: Thu, 13 Jul 2023 12:22:01 +0300 Subject: [PATCH 5/5] Test articles_by_department --- magazine/tests.py | 45 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 10 deletions(-) diff --git a/magazine/tests.py b/magazine/tests.py index 566834ecf..b315c75ab 100644 --- a/magazine/tests.py +++ b/magazine/tests.py @@ -33,23 +33,27 @@ def setUp(self) -> None: title="Departments", ) self.magazine_index.add_child(instance=self.magazine_department_index) - self.magazine_department = MagazineDepartment( + self.magazine_department_one = MagazineDepartment( title="Department 1", ) - self.magazine_department_index.add_child(instance=self.magazine_department) + self.magazine_department_two = MagazineDepartment( + title="Department 2", + ) + self.magazine_department_index.add_child(instance=self.magazine_department_one) + self.magazine_department_index.add_child(instance=self.magazine_department_two) self.magazine_index.add_child(instance=self.magazine_issue) self.magazine_article_one = self.magazine_issue.add_child( instance=MagazineArticle( title="Article 1", - department=self.magazine_department, + department=self.magazine_department_two, is_featured=True, ), ) self.magazine_article_two = self.magazine_issue.add_child( instance=MagazineArticle( title="Article 2", - department=self.magazine_department, + department=self.magazine_department_one, is_featured=False, ), ) @@ -62,6 +66,17 @@ def test_featured_articles(self) -> None: [self.magazine_article_one], ) + def test_articles_by_department(self) -> None: + """Test that the articles_by_department property returns the correct + articles.""" + self.assertEqual( + list(self.magazine_issue.articles_by_department), + [ + self.magazine_article_two, + self.magazine_article_one, + ], + ) + def test_publication_end_date(self) -> None: """Test that the publication_end_date property returns the correct date.""" @@ -70,10 +85,20 @@ def test_publication_end_date(self) -> None: datetime.date(2020, 2, 1), ) - def test_get_context(self) -> None: - # Here you would add tests for the get_context method. - pass - def test_get_sitemap_urls(self) -> None: - # Here you would add tests for the get_sitemap_urls method. - pass + """Validate the output of get_sitemap_urls.""" + + expected_last_mod = None + expected_location_contains = "/magazine/issue-1/" + + sitemap_urls = self.magazine_issue.get_sitemap_urls() + + self.assertEqual(len(sitemap_urls), 1) + self.assertEqual( + sitemap_urls[0]["lastmod"], + expected_last_mod, + ) + self.assertIn( + expected_location_contains, + sitemap_urls[0]["location"], + )