Enhance article templates and optimize database queries for tagged pages #1170
Conversation
Reviewer's GuideThis PR refactors the article summary template for cleaner author handling, enhances tagged pages view performance using select_related and prefetch_related, and adds dedicated search result templates for magazine issues and articles. Class diagram for optimized queryset usage in tagged pagesclassDiagram
LibraryItem <|-- ContentType
LibraryItem o-- Author
MagazineArticle <|-- ContentType
MagazineArticle o-- Author
MagazineArticle o-- Issue
NewsItem <|-- ContentType
WfPage <|-- ContentType
class LibraryItem {
+title
+tags
+authors
+content_type
}
class MagazineArticle {
+title
+tags
+authors
+content_type
+issue
}
class NewsItem {
+title
+tags
+content_type
}
class WfPage {
+title
+tags
+content_type
}
class Author {
+author
+live
}
class Issue {
+publication_date
}
class ContentType {
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@brylie has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds i18n and safer author and issue rendering to the magazine article summary template, introduces/updates per-type search templates for magazine articles and issues, and refactors several model and view get_queryset implementations to use manager-based live()/select_related()/prefetch_related() with StreamField deferral and a final Python-level sort for combined tag results. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant SearchView as Search Results Page
participant ArticleTmpl as search/magazine_article.html
participant Summary as magazine_article_summary.html
participant Wagtail as Wagtail (pageurl)
User->>SearchView: request results
SearchView->>ArticleTmpl: render entity
ArticleTmpl->>Summary: include (article=entity)
Summary->>Summary: with author_links = article.authors.all\nwith issue = article.get_parent
alt authors present
Summary->>Wagtail: pageurl(author) for live authors
Wagtail-->>Summary: author URLs
Summary-->>ArticleTmpl: render inline author list (links/text + commas)
else no authors
Summary-->>ArticleTmpl: omit authors block
end
alt issue present
Summary->>Wagtail: pageurl(issue)
Wagtail-->>Summary: issue URL
Summary-->>ArticleTmpl: render "Issue: [link] (Month Year)"
end
ArticleTmpl-->>User: rendered result item
sequenceDiagram
autonumber
participant TaggedView as TaggedPageListView
participant ORM as Django ORM
participant DB as Database
TaggedView->>ORM: build LibraryItem qs\n.filter(...).live().public()\n.select_related("content_type")\n.prefetch_related("authors__author")
ORM->>DB: query + prefetch
DB-->>ORM: rows + related
TaggedView->>ORM: build MagazineArticle qs\n.filter(...).live().public()\n.select_related("content_type")\n.prefetch_related("authors__author")
ORM->>DB: query + prefetch
DB-->>ORM: rows + related
TaggedView->>ORM: build NewsItem qs\n.filter(...).live().public()\n.select_related("content_type")
ORM->>DB: query
DB-->>ORM: rows
TaggedView->>ORM: build WfPage qs\n.filter(...).live().public()\n.select_related("content_type")
ORM->>DB: query
DB-->>ORM: rows
TaggedView->>TaggedView: chain all results and sort by title.lower()
TaggedView-->>Caller: combined sorted list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
magazine/templates/search/magazine_issue.html (2)
8-8: Use a semantic time element for datesMake the published date machine-readable and accessible.
- <p class="text-sm text-base-content/70">Published {{ entity.specific.publication_date|date:"F Y" }}</p> + <p class="text-sm text-base-content/70"> + Published + <time datetime="{{ entity.specific.publication_date|date:'Y-m-d' }}"> + {{ entity.specific.publication_date|date:"F Y" }} + </time> + </p>
7-8: Potential N+1: consider passing specific objects from the viewAccessing entity.specific in templates can trigger extra queries per row. If feasible, have the search view return specific() results and pass entity directly.
tags/views.py (2)
28-33: Avoid redundant DB ordering; rely on the combined Python sortSince the combined list is sorted by title afterward, the per-query .order_by("title") adds unnecessary DB work.
- .order_by("title") + # DB-level ordering is unnecessary; the combined list is sorted belowApply to all four query blocks.
Also applies to: 35-40, 42-46, 48-52
28-33: Leverage model-level optimized querysetsLibraryItem, MagazineArticle, and WfPage expose class-level get_queryset() with prefetches. Use them to centralize optimizations and avoid drift.
- library_items = ( - LibraryItem.objects.filter(filter_condition).live().public() + library_items = ( + LibraryItem.get_queryset().filter(filter_condition).live().public() .select_related("content_type") .prefetch_related("authors__author") ) - magazine_articles = ( - MagazineArticle.objects.filter(filter_condition).live().public() + magazine_articles = ( + MagazineArticle.get_queryset().filter(filter_condition).live().public() .select_related("content_type") - .prefetch_related("authors__author", "issue") + .prefetch_related("authors__author") # keep only concrete relations here ) - pages = ( - WfPage.objects.filter(filter_condition).live().public() + pages = ( + WfPage.get_queryset().filter(filter_condition).live().public() .select_related("content_type") )Also applies to: 35-40, 48-52
magazine/templates/search/magazine_article.html (1)
1-1: Confirm specific() is provided upstream to avoid N+1Including with article=entity.specific is fine, but it may trigger per-row queries. If the search view can return specific() results, pass article=entity.
magazine/templates/magazine/magazine_article_summary.html (2)
17-19: Add a space after commas in author listImproves readability and matches common UI conventions.
- <a href="{% pageurl author.author %}">{{ author.author }}</a>{% if not forloop.last %},{% endif %} + <a href="{% pageurl author.author %}">{{ author.author }}</a>{% if not forloop.last %}, {% endif %} ... - {{ author.author }}{% if not forloop.last %},{% endif %} + {{ author.author }}{% if not forloop.last %}, {% endif %}
30-35: Avoid duplicate get_parent() calls and use a semantic time elementCache the parent lookup and render a machine-readable date. Reduces potential extra queries and improves accessibility.
- <div class="card-actions"> - <span> - Issue: <a href="{% pageurl article.get_parent %}" class="link"> - {{ article.get_parent }} ({{ article.get_parent.specific.publication_date|date:"F Y" }}) - </a> - </span> - </div> + {% with issue=article.get_parent %} + <div class="card-actions"> + <span> + Issue: <a href="{% pageurl issue %}" class="link"> + {{ issue }} + {% if issue.specific.publication_date %} + (<time datetime="{{ issue.specific.publication_date|date:'Y-m-d' }}"> + {{ issue.specific.publication_date|date:"F Y" }} + </time>) + {% endif %} + </a> + </span> + </div> + {% endwith %}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
magazine/templates/magazine/magazine_article_summary.html(1 hunks)magazine/templates/search/magazine_article.html(1 hunks)magazine/templates/search/magazine_issue.html(1 hunks)tags/views.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.html
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.html: Follow WCAG 2.1 and ARIA 1.1; use semantic HTML, appropriate ARIA roles/attributes, and ensure all interactive elements are keyboard accessible
Use semantic HTML tags (header, nav, main, footer, section, article, aside, figure/figcaption, time, address) and proper heading structure
Wrap content rendered via Wagtail’s richtext filter in a Tailwindproseclass
Use Bootstrap Icons viabi bi-<icon-name>classes and keep icon usage consistent across the UI
Files:
magazine/templates/search/magazine_issue.htmlmagazine/templates/search/magazine_article.htmlmagazine/templates/magazine/magazine_article_summary.html
**/*.{html,css}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer Tailwind CSS and daisyUI for new styles; avoid introducing new Bootstrap styles
Files:
magazine/templates/search/magazine_issue.htmlmagazine/templates/search/magazine_article.htmlmagazine/templates/magazine/magazine_article_summary.html
🧬 Code graph analysis (2)
magazine/templates/search/magazine_issue.html (1)
magazine/models.py (6)
MagazineIssue(115-169)MagazineIndexPage(36-112)MagazineArticle(230-364)DeepArchiveIndexPage(492-554)ArchiveIssue(455-489)get_context(540-554)
tags/views.py (4)
library/models.py (1)
LibraryItem(32-167)magazine/models.py (3)
MagazineArticle(329-470)MagazineArticleTag(172-175)MagazineArticle(230-364)news/models.py (1)
NewsItem(103-156)wf_pages/models.py (1)
WfPage(60-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: runner-job
🔇 Additional comments (1)
magazine/templates/magazine/magazine_article_summary.html (1)
11-24: Authors block looks good and avoids extra count() queryUsing a with to cache article.authors.all and conditionally rendering is clean and efficient.
…Page in TaggedPageListView
…ing in magazine article summary
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tags/views.py (1)
91-95: Avoid recomputing get_queryset() in get_context_data (double work).ListView.get() already set self.object_list via get_queryset(). Use that to feed pagination.
- context["paginated_items"] = get_paginated_items( - items=self.get_queryset(), + items = context.get("object_list", getattr(self, "object_list", [])) + context["paginated_items"] = get_paginated_items( + items=items, items_per_page=self.paginate_by, page_number=page_number, )
♻️ Duplicate comments (1)
tags/views.py (1)
48-54: LGTM: visibility filters and content_type select_related added.This addresses the prior concern about exposing drafts/private content.
🧹 Nitpick comments (6)
magazine/templates/magazine/magazine_article_summary.html (2)
11-24: Authors block: good consolidation; consider list semantics and i18n (minor).
- Presenting multiple authors as a list is more accessible than comma-separated spans.
- Literal strings (“Author(s):”) should be wrapped for translation if i18n is enabled.
Example tweak:
- <div class="mb-2"> - <span class="font-medium">Author(s):</span> - {% for author in author_links %} - {% if author.author.live %} - <a href="{% pageurl author.author %}">{{ author.author }}</a>{% if not forloop.last %}, {% endif %} - {% else %} - {{ author.author }}{% if not forloop.last %}, {% endif %} - {% endif %} - {% endfor %} - </div> + <div class="mb-2"> + <span class="font-medium">{% trans "Authors" %}:</span> + <ul class="inline list-disc list-inside" role="list"> + {% for author in author_links %} + <li class="inline"> + {% if author.author.live %} + <a href="{% pageurl author.author %}">{{ author.author }}</a> + {% else %} + {{ author.author }} + {% endif %} + {% if not forloop.last %}, {% endif %} + </li> + {% endfor %} + </ul> + </div>
30-43: Heads-up: article.get_parent in a loop can cause N+1 queries.Rendering the parent issue per article will likely query once per row. If this page lists many articles, consider adding an explicit foreign key (e.g., MagazineArticle.issue) so you can prefetch it, or cache parent pages upstream.
If adding a relation isn’t feasible now, monitor query counts on this template.
tags/views.py (4)
28-36: Avoid redundant filters/prefetch; rely on model get_queryset where possible.
- LibraryItem.get_queryset() already applies live=True and prefetches authors__author; chaining .live() and .prefetch_related("authors__author") repeats work.
- library_items = ( - LibraryItem.get_queryset() - .filter(filter_condition) - .live() - .public() - .select_related("content_type") - .prefetch_related("authors__author") - ) + library_items = ( + LibraryItem.get_queryset() + .filter(filter_condition) + .public() + .select_related("content_type") + )
38-46: Same redundancy for MagazineArticle queryset.MagazineArticle.get_queryset() already prefetches authors__author. Keep .public(), but drop duplicate prefetch to reduce planner/ORM overhead.
- magazine_articles = ( - MagazineArticle.get_queryset() - .filter(filter_condition) - .live() - .public() - .select_related("content_type") - .prefetch_related("authors__author") - ) + magazine_articles = ( + MagazineArticle.get_queryset() + .filter(filter_condition) + .live() + .public() + .select_related("content_type") + )
28-36: Reconsider dropping DB-level ordering; Python sort can scale poorly.Global Python sorting forces full materialization of all four lists before pagination. For large tags, this is memory- and CPU-heavy. Options:
- Use Page.objects (base Wagtail Page) to query all tagged pages across types and order in DB with Lower("title"), then .specific() for polymorphism.
- Or pre-order each queryset in DB and perform a k-way merge iterator before pagination.
Also applies to: 38-46, 48-54, 56-63
74-77: Sorting key: prefer casefold() over lower() for robust case-insensitive ordering.casefold handles more Unicode cases.
- key=lambda instance: instance.title.lower(), + key=lambda instance: instance.title.casefold(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
magazine/templates/magazine/magazine_article_summary.html(1 hunks)magazine/templates/search/magazine_article.html(1 hunks)magazine/templates/search/magazine_issue.html(1 hunks)tags/views.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- magazine/templates/search/magazine_article.html
- magazine/templates/search/magazine_issue.html
🧰 Additional context used
📓 Path-based instructions (2)
**/*.html
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.html: Follow WCAG 2.1 and ARIA 1.1; use semantic HTML, appropriate ARIA roles/attributes, and ensure all interactive elements are keyboard accessible
Use semantic HTML tags (header, nav, main, footer, section, article, aside, figure/figcaption, time, address) and proper heading structure
Wrap content rendered via Wagtail’s richtext filter in a Tailwindproseclass
Use Bootstrap Icons viabi bi-<icon-name>classes and keep icon usage consistent across the UI
Files:
magazine/templates/magazine/magazine_article_summary.html
**/*.{html,css}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer Tailwind CSS and daisyUI for new styles; avoid introducing new Bootstrap styles
Files:
magazine/templates/magazine/magazine_article_summary.html
🧬 Code graph analysis (1)
tags/views.py (3)
library/models.py (1)
LibraryItem(32-167)magazine/models.py (3)
MagazineArticle(329-470)MagazineArticle(230-364)MagazineArticleTag(172-175)wf_pages/models.py (1)
WfPage(60-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: runner-job
🔇 Additional comments (1)
tags/views.py (1)
56-63: LGTM: WfPage queryset is aligned with the others.Consistent live/public filtering and content_type preload.
…and WfPage models
…magazine article summary
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1170 +/- ##
==========================================
+ Coverage 93.32% 93.69% +0.36%
==========================================
Files 130 131 +1
Lines 4332 4348 +16
==========================================
+ Hits 4043 4074 +31
+ Misses 289 274 -15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magazine/models.py (1)
302-325: Invalid prefetch target and duplicate queryset assignment will crash and waste work.
prefetch_related("issue")is invalid on MagazineArticle (no FK named "issue") and will raise a FieldError when evaluated.- The queryset for
articlesis built twice with identical logic.Remove the invalid prefetch and the duplicate block.
- context["articles"] = ( - MagazineArticle.objects.filter( - department__title=self.title, - ) - .live() - .prefetch_related( - "authors__author", - "issue", - ) - ) - - articles = ( + articles = ( MagazineArticle.objects.filter( department__title=self.title, ) .live() .prefetch_related( "authors__author", - "issue", ) ) - context["articles"] = articles + context["articles"] = articles
🧹 Nitpick comments (5)
magazine/models.py (1)
368-371: Prefer select_related for FK and keep prefetch for M2M/Orderables.
departmentis a FK;select_related("department")will be more efficient thanprefetch_related("department"). Keep prefetch for authors/tags.- """Prefetch authors, tags, and department for performance.""" - related_fields = ["authors__author", "tags", "department"] - return cls.objects.prefetch_related(*related_fields) + """Optimize related loads: select FK, prefetch M2M/Orderables.""" + related_prefetch = ["authors__author", "tags"] + return ( + cls.objects + .select_related("department") + .prefetch_related(*related_prefetch) + )library/models.py (1)
82-85: Good: switch to "tags" and filter via live(); tighten relation loading further.Approved as-is. For fewer queries, use
select_relatedfor single FKs and keep prefetch for M2M/Orderables.- related_fields = [ - "authors__author", - "topics__topic", - "item_audience", - "item_genre", - "item_medium", - "item_time_period", - "tags", - ] - return cls.objects.live().prefetch_related(*related_fields) + related_prefetch = [ + "authors__author", + "topics__topic", + "tags", + ] + return ( + cls.objects.live() + .select_related( + "item_audience", + "item_genre", + "item_medium", + "item_time_period", + ) + .prefetch_related(*related_prefetch) + )magazine/templates/magazine/magazine_article_summary.html (3)
11-29: Tighten list semantics: avoid bullets + commas, and drop redundant role.Using both
list-discand manual commas yields mixed semantics and extra verbosity for AT. Keep comma-separated inline items and remove bullet styling androle="list".- <ul class="inline list-disc list-inside" role="list"> + <ul class="inline">
38-46: Localize “Issue” label.Match the i18n pattern used for “Authors”.
- Issue: <a href="{% pageurl issue %}" class="link"> + <span class="font-medium">{% translate "Issue" %}:</span> + <a href="{% pageurl issue %}" class="link">
35-48: Potential N+1: article.get_parent in lists.In search/tagged listings this may issue 1 query per article. If query counts are high, consider passing
issuefrom the view (e.g., annotate or batch-resolve parents) and reusing it here.I can help profile the view and sketch a parent-batch helper if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
library/models.py(1 hunks)magazine/models.py(1 hunks)magazine/templates/magazine/magazine_article_summary.html(2 hunks)wf_pages/models.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.html
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.html: Follow WCAG 2.1 and ARIA 1.1; use semantic HTML, appropriate ARIA roles/attributes, and ensure all interactive elements are keyboard accessible
Use semantic HTML tags (header, nav, main, footer, section, article, aside, figure/figcaption, time, address) and proper heading structure
Wrap content rendered via Wagtail’s richtext filter in a Tailwindproseclass
Use Bootstrap Icons viabi bi-<icon-name>classes and keep icon usage consistent across the UI
Files:
magazine/templates/magazine/magazine_article_summary.html
**/*.{html,css}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer Tailwind CSS and daisyUI for new styles; avoid introducing new Bootstrap styles
Files:
magazine/templates/magazine/magazine_article_summary.html
🧬 Code graph analysis (1)
magazine/models.py (1)
content_migration/management/import_magazine_articles_handler.py (1)
handle_import_magazine_articles(79-139)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: runner-job
🔇 Additional comments (1)
wf_pages/models.py (1)
90-90: No lingeringtags__tagprefetches found; approve switch toprefetch_related('tags'). Confirm callers don’t rely on draft items; consider adding.live()if this queryset powers public views.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
magazine/models.py (1)
356-358: Keep parent queryset semantics; use select_related for FK (department).Switching from
super().get_queryset()tocls.objectsmay drop parent-level queryset tweaks (ordering/defer settings). Also,departmentis a single-valued FK, soselect_relatedis more efficient thanprefetch_related. Keep prefetching forauthors__authorandtags. Optionally add.defer_streamfields()if this queryset backs listings wherebodyisn't needed.@classmethod def get_queryset(cls): - """Prefetch authors, tags, and department for performance.""" - related_fields = ["authors__author", "tags", "department"] - return cls.objects.prefetch_related(*related_fields) + """Optimize related fetches for listings.""" + return ( + super() + .get_queryset() + .defer_streamfields() # optional: drop if detail pages rely on eager body + .select_related("department") + .prefetch_related("authors__author", "tags") + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
magazine/models.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: runner-job
… for better semantics
…d improve performance
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
magazine/templates/magazine/magazine_article_summary.html (2)
11-27: Minor a11y polish for the author list.Associate the “Authors” label with the list and normalize list styling.
Apply:
- <span class="font-medium">{% translate "Authors" %}:</span> - <ul class="inline"> + <span id="authors-label" class="font-medium">{% translate "Authors" %}:</span> + <ul class="inline list-none p-0 m-0" aria-labelledby="authors-label">Also consider
inline-flex flex-wrap gap-1on the<ul>if spacing needs improvement.
35-49: Keep link text succinct (date outside the anchor).Move the publication date outside the link to improve screen-reader verbosity and hit target clarity.
- <a href="{% pageurl issue %}" class="link"> - {{ issue }} - {% if issue.specific.publication_date %} - (<time datetime="{{ issue.specific.publication_date|date:'Y-m-d' }}"> - {{ issue.specific.publication_date|date:"F Y" }} - </time>) - {% endif %} - </a> + <a href="{% pageurl issue %}" class="link">{{ issue }}</a> + {% if issue.specific.publication_date %} + (<time datetime="{{ issue.specific.publication_date|date:'Y-m-d' }}"> + {{ issue.specific.publication_date|date:"F Y" }} + </time>) + {% endif %}Note: Using article.get_parent() in a list can trigger N+1 queries. If lists are large, consider measuring with Django Debug Toolbar.
library/models.py (1)
75-89: Optional: Defer the StreamField payload in get_queryset
Since the project is pinned to Wagtail 7.0.2 and you’re already using.defer_streamfields()in other models (e.g. magazine/models.py), you can optionally add it here to slim down the listing payload:return ( - cls.objects.live() + cls.objects.live().defer_streamfields() .select_related( "item_audience", "item_genre", "item_medium", "item_time_period", ) .prefetch_related(*related_prefetch) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
library/models.py(1 hunks)magazine/models.py(1 hunks)magazine/templates/magazine/magazine_article_summary.html(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.html
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.html: Follow WCAG 2.1 and ARIA 1.1; use semantic HTML, appropriate ARIA roles/attributes, and ensure all interactive elements are keyboard accessible
Use semantic HTML tags (header, nav, main, footer, section, article, aside, figure/figcaption, time, address) and proper heading structure
Wrap content rendered via Wagtail’s richtext filter in a Tailwindproseclass
Use Bootstrap Icons viabi bi-<icon-name>classes and keep icon usage consistent across the UI
Files:
magazine/templates/magazine/magazine_article_summary.html
**/*.{html,css}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer Tailwind CSS and daisyUI for new styles; avoid introducing new Bootstrap styles
Files:
magazine/templates/magazine/magazine_article_summary.html
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: runner-job
🔇 Additional comments (2)
magazine/templates/magazine/magazine_article_summary.html (1)
1-1: Good i18n inclusion.Loading i18n up front is correct; new strings (“Authors”, “Issue”) are wrapped with translate.
magazine/models.py (1)
356-365: Approve: optimized queryset is correct
defer_streamfields(),select_related("department"), andprefetch_related("authors__author", "tags")are fully supported in our pinned Wagtail 7.0.2 (method present since at least v2.13.4 and documented in v5.2.1) (docs.wagtail.org)
… for better semantics
…for improved performance
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
magazine/templates/magazine/magazine_article_summary.html (1)
35-47: Minor: reduce repeated .specific access and tighten variable scope.Bind the specific parent once; it’s a tiny readability win and avoids repeated attribute resolution.
Apply this diff:
-{% with issue=article.get_parent %} +{% with issue=article.get_parent.specific %} ... - <a href="{% pageurl issue %}" class="link">{{ issue }}</a> - {% if issue.specific.publication_date %} - (<time datetime="{{ issue.specific.publication_date|date:'Y-m-d' }}"> - {{ issue.specific.publication_date|date:"F Y" }} + <a href="{% pageurl issue %}" class="link">{{ issue }}</a> + {% if issue.publication_date %} + (<time datetime="{{ issue.publication_date|date:'Y-m-d' }}"> + {{ issue.publication_date|date:"F Y" }} </time>) {% endif %}tags/tests.py (3)
173-178: Store authors on self for clearer assertions.Promote local vars to instance attrs to reference them unambiguously in assertions.
Apply this diff:
- person_live = PersonFactory() - # Publish person_live so template renders it as a link - person_live.save_revision().publish() - - person_not_live = PersonFactory() + self.person_live = PersonFactory() + # Publish person_live so template renders it as a link + self.person_live.save_revision().publish() + + self.person_not_live = PersonFactory()Additionally, add at file top:
import re
179-183: Associate the correct Person objects to the article.Use the instance attributes to match the refactor above.
Apply this diff:
- MagazineArticleAuthor.objects.create(article=self.article, author=person_live) + MagazineArticleAuthor.objects.create(article=self.article, author=self.person_live) MagazineArticleAuthor.objects.create( article=self.article, - author=person_not_live, + author=self.person_not_live, )
196-198: Assert the exact machine-readable date.Check the precise datetime attribute value matches the issue’s publication_date.
Apply this diff:
- # Issue label and machine-readable date present - self.assertIn("Issue", html) - self.assertIn('<time datetime="', html) + # Issue label and machine-readable date present + self.assertIn("Issue", html) + expected_date = ( + self.article.get_parent().specific.publication_date.strftime("%Y-%m-%d") + ) + self.assertIn(f'<time datetime="{expected_date}"', html)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
library/models.py(1 hunks)magazine/templates/magazine/magazine_article_summary.html(2 hunks)tags/tests.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- library/models.py
🧰 Additional context used
📓 Path-based instructions (3)
{**/tests.py,**/tests/*.py,**/tests/**/*.py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write tests using Django’s testing framework (e.g., django.test.TestCase) rather than unittest or pytest
Files:
tags/tests.py
**/*.html
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.html: Follow WCAG 2.1 and ARIA 1.1; use semantic HTML, appropriate ARIA roles/attributes, and ensure all interactive elements are keyboard accessible
Use semantic HTML tags (header, nav, main, footer, section, article, aside, figure/figcaption, time, address) and proper heading structure
Wrap content rendered via Wagtail’s richtext filter in a Tailwindproseclass
Use Bootstrap Icons viabi bi-<icon-name>classes and keep icon usage consistent across the UI
Files:
magazine/templates/magazine/magazine_article_summary.html
**/*.{html,css}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer Tailwind CSS and daisyUI for new styles; avoid introducing new Bootstrap styles
Files:
magazine/templates/magazine/magazine_article_summary.html
🧬 Code graph analysis (1)
tags/tests.py (2)
magazine/models.py (5)
MagazineArticle(317-465)MagazineArticleAuthor(468-489)MagazineArticle(230-364)MagazineArticleAuthor(367-383)MagazineArticleTag(172-175)contact/factories.py (2)
PersonFactory(44-74)slug(56-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: runner-job
🔇 Additional comments (4)
magazine/templates/magazine/magazine_article_summary.html (2)
1-1: LGTM: i18n loaded correctly.Importing i18n alongside wagtailcore_tags is appropriate here.
11-29: Author rendering approach looks good and aligns with prefetching.Conditional section, inline list semantics, and linking only live authors are correct.
tags/tests.py (2)
7-7: LGTM: import used.MagazineArticleAuthor is used to attach authors in tests.
12-12: LGTM: import used.PersonFactory is used to create author pages.
| <span id="authors-label" class="font-medium">{% translate "Authors" %}:</span> | ||
| <ul class="inline list-none p-0 m-0" aria-labelledby="authors-label"> | ||
| {% for author in author_links %} |
There was a problem hiding this comment.
Fix duplicate IDs for accessibility and validity.
Using id="authors-label" on multiple cards will create duplicate IDs on listing/search pages. Make the ID unique per article and update aria-labelledby accordingly.
Apply this diff:
- <span id="authors-label" class="font-medium">{% translate "Authors" %}:</span>
- <ul class="inline list-none p-0 m-0" aria-labelledby="authors-label">
+ <span id="authors-label-{{ article.id }}" class="font-medium">{% translate "Authors" %}:</span>
+ <ul class="inline list-none p-0 m-0" aria-labelledby="authors-label-{{ article.id }}">I can update the related tests in tags/tests.py to assert the new dynamic ID.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <span id="authors-label" class="font-medium">{% translate "Authors" %}:</span> | |
| <ul class="inline list-none p-0 m-0" aria-labelledby="authors-label"> | |
| {% for author in author_links %} | |
| <span id="authors-label-{{ article.id }}" class="font-medium">{% translate "Authors" %}:</span> | |
| <ul class="inline list-none p-0 m-0" aria-labelledby="authors-label-{{ article.id }}"> | |
| {% for author in author_links %} |
🤖 Prompt for AI Agents
magazine/templates/magazine/magazine_article_summary.html around lines 14-16:
the id "authors-label" is static and will be duplicated across multiple article
cards; change it to a unique id per article (e.g., append the article.pk or
forloop.counter: "authors-label-{{ article.pk }}" or similar) and update the
ul's aria-labelledby to reference that same dynamic id so they match; also
update tags/tests.py to assert the new dynamic id is rendered (use the same
identifier logic in the test).
…le summary template
…rove date formatting
…with dynamic ID extraction and non-live author checks
Improve article templates for better author display and optimize database queries by using
select_relatedandprefetch_relatedfor efficiency.Summary by Sourcery
Streamline article templates for cleaner author display, improve efficiency of tagged page queries with optimized ORM fetching, and introduce new search result templates for magazine issues and articles
New Features:
Enhancements:
withblockselect_relatedon content_type andprefetch_relatedon related authors and issuesSummary by CodeRabbit
New Features
Refactor
Tests