Story 2096: Implement Wagtail Integration#2100
Open
jlchilders11 wants to merge 29 commits into
Open
Conversation
jlchilders11
changed the title
gregjkal
previously requested changes
Feb 27, 2026
gregjkal
left a comment
gregjkal
left a comment
Collaborator
There was a problem hiding this comment.
Per our conversation in standup: please test the homepage to make sure it won't break without manually migrating the existing testimonial and/or news items. If it does break, we need to either make these changes backwards-compatible (my strong preference!), with a follow-up change that cleans any bridge code up, or add a data migration so it can be fixed during the deploy instead of crashing until manual fixes.
julioest
changed the title
julioest
approved these changes
May 18, 2026
julhoang
reviewed
May 20, 2026
julhoang
left a comment
julhoang
left a comment
Collaborator
There was a problem hiding this comment.
Hi @jlchilders11 ! This is awesome work! I just left a few of clarifying questions below.
Besides them, it looks like the HTML templates are now outdated. Do you have a next-step recommendation for us to update those templates to V3 versions? Also, do we need to update the V3 Create Post page and various other existing queries to query from Wagtail's PostPage instead of Entry?
Thanks so much for your work here!
jlchilders11
force-pushed
the
jc/wagtail-integration
branch
from
a984bd7 to
bd274ee
Compare
9 tasks
Collaborator
Author
For updating these to the V3 and existing things, I believe that should be handled in the data integration phase of those pages, which I believe are still outstanding tickets. If not, I do think that that work should be handled as part of a separate ticket, as it somewhat escapes the "migration" scope of this ticket. |
herzog0
reviewed
May 21, 2026
herzog0
left a comment
herzog0
left a comment
Collaborator
There was a problem hiding this comment.
Hey @jlchilders11 this is looking great!
I just have a couple of questions that I'll write in a Slack thread to keep it as a better reference for the future
julhoang
previously requested changes
Jun 1, 2026
julhoang
left a comment
julhoang
left a comment
Collaborator
There was a problem hiding this comment.
Hi @jlchilders11 , I've left a couple of suggestions below – please let me know your thoughts!
A few more things:
1/ I noticed that a few video entries are looking like this when they have a empty summary – can we make sure to not render that block in that case?
2/ The filters on the http://localhost:8000/pages/news/ are not redirecting to the new /pages path – I understand if this makes more sense to tackle in this ticket instead: #2378. If that's the case, would you mind adding a note for Katty so that she's aware?
Collaborator
Author
|
julhoang
reviewed
Jun 4, 2026
julhoang
left a comment
julhoang
left a comment
Collaborator
There was a problem hiding this comment.
Hi @jlchilders11 , I have 1 questions regarding the summary_dispatcher, others are just optional nits 🙏
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Wagtail pages app, routed PostPage content models, migration and conversion tooling, and updates news tasks, templates, and routing to support the new page-based content flow. ChangesPages App and Dual-Model Content System
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
news/utils.py (2)
27-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd timeouts to both outbound thumbnail requests.
Lines 28 and 35 call external URLs without a timeout. These run in Celery/backfill workers, so one slow upstream can pin a worker indefinitely and stall the queue.
🛡️ Proposed fix
- response = requests.get(url) + response = requests.get(url, timeout=10) @@ - thumbnail_file = File( - BytesIO(requests.get(json.get("thumbnail_url")).content), + thumbnail_response = requests.get(json.get("thumbnail_url"), timeout=10) + thumbnail_response.raise_for_status() + thumbnail_file = File( + BytesIO(thumbnail_response.content), name={f"{video.slug} Thumbnail"}, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@news/utils.py` around lines 27 - 35, The outbound requests to the OEmbed endpoint and to fetch the thumbnail are missing timeouts and can hang workers; update both requests.get calls (the one that uses YOUTUBE_OEMBED_ENDPOINT + f"?url={video.external_url}" and the one inside the thumbnail_file = File(BytesIO(requests.get(json.get("thumbnail_url")).content), ...)) to pass a timeout parameter (e.g. timeout=10 or a module-level/configurable constant like REQUEST_TIMEOUT), and optionally wrap them in a try/except for requests.Timeout/requests.RequestException to raise or log a clear error if the call times out.
34-37:⚠️ Potential issue | 🔴 CriticalPass a string filename to
File(...).Line 36 uses
name={f"{video.slug} Thumbnail"}, which creates aset; Django file handling expectsnameto be a string (so saving to aFileFieldcan error).🐛 Proposed fix
- thumbnail_file = File( - BytesIO(requests.get(json.get("thumbnail_url")).content), - name={f"{video.slug} Thumbnail"}, - ) + thumbnail_file = File( + BytesIO(requests.get(json.get("thumbnail_url")).content), + name=f"{video.slug} Thumbnail", + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@news/utils.py` around lines 34 - 37, The thumbnail_file creation passes a set to File(...).name by using name={f"{video.slug} Thumbnail"}; change this to pass a string instead (e.g. name=f"{video.slug} Thumbnail") so Django receives a proper filename string; update the File(...) invocation in the thumbnail_file creation (the call that wraps BytesIO(requests.get(json.get("thumbnail_url")).content)) to use a plain formatted string for name and ensure the BytesIO/requests/get/json.get/video.slug references remain unchanged.
🧹 Nitpick comments (4)
marketing/models.py (1)
164-164: ⚡ Quick winRemove the leftover request-path print.
This writes every outreach request path to stdout and bypasses the structured logging configured in settings. If you still need rollout visibility here, switch it to a logger debug call instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@marketing/models.py` at line 164, Remove the stray print(path_components) call that emits request paths to stdout; replace it with a structured logger debug call instead (e.g., use the module-level logger or logging.getLogger(__name__)). Locate the print(path_components) usage (referenced symbol: path_components / print(path_components)) and change it to logger.debug with a clear message and the path_components as structured data so it uses the configured logging backend rather than stdout.pages/models.py (2)
338-343: 💤 Low valueOptional: Use iterable unpacking for content_panels.
Consider using iterable unpacking instead of concatenation for consistency with modern Python style and to match the suggestion in
mixins.py.♻️ Proposed refactor
- content_panels = BasePage.content_panels + [ + content_panels = [ + *BasePage.content_panels, "content", "image", "summary", "video_thumbnail", ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pages/models.py` around lines 338 - 343, The content_panels assignment currently concatenates BasePage.content_panels with a list; change it to use iterable unpacking so it mirrors the style used in mixins.py. Update the content_panels definition on the Page model to produce a single list by unpacking BasePage.content_panels (e.g., using the * operator) followed by the added panel names ("content", "image", "summary", "video_thumbnail"), ensuring the symbol names content_panels and BasePage.content_panels are used so the intent and ordering remain identical.Source: Linters/SAST tools
144-185: ⚡ Quick winReduce duplication by generating filter_terms from POST_CONTENT_TYPES.
The filter terms are hard-coded here and duplicated in
news/views.py. The TODO comment on line 143 notes this should be generated fromPOST_CONTENT_TYPES. Consider implementing this now to reduce duplication and ensure consistency across the codebase.♻️ Proposed refactor to generate filter_terms
+ # Generate filter terms from POST_CONTENT_TYPES + filter_terms = [] + for content_type in POST_CONTENT_TYPES: + if content_type.filter_name: + filter_terms.append({ + "label": content_type.content_type + "s" if content_type.content_type != "News" else content_type.content_type, + "value": content_type.filter_name, + "url": self.get_url() + f"?type={content_type.filter_name}", + }) + else: + # "All" filter + filter_terms.insert(0, { + "label": "All", + "value": "all", + "url": self.get_url(), + }) + + # Add placeholder filters (Discussions, Achievements, Issues) + # TODO: Implement these content types when ready + filter_terms.extend([ { "label": "Discussions", "value": "discussions", "url": self.get_url(), }, { "label": "Achievements", "value": "achievements", "url": self.get_url(), }, { "label": "Issues", "value": "issues", "url": self.get_url(), }, - ] + ]) + ctx["filter_terms"] = filter_terms - ctx["filter_terms"] = [ - { - "label": "All", - "value": "all", - "url": self.get_url(), - }, - { - "label": "News", - "value": "news", - "url": self.get_url() + "?type=news", - }, - { - "label": "Blogs", - "value": "blogpost", - "url": self.get_url() + "?type=blogpost", - }, - { - "label": "Links", - "value": "link", - "url": self.get_url() + "?type=link", - }, - { - "label": "Videos", - "value": "video", - "url": self.get_url() + "?type=video", - }, - { - "label": "Discussions", - "value": "discussions", - "url": self.get_url(), - }, - { - "label": "Achievements", - "value": "achievements", - "url": self.get_url(), - }, - { - "label": "Issues", - "value": "issues", - "url": self.get_url(), - }, - ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pages/models.py` around lines 144 - 185, The filter_terms list in the pages model is hard-coded and duplicates logic in news/views.py; instead iterate over the POST_CONTENT_TYPES mapping to build each term so they stay consistent. Update the code that sets ctx["filter_terms"] (and any related method using get_url()) to loop through POST_CONTENT_TYPES items, creating dicts with "label" from the human-readable name, "value" from the content type key, and "url" using self.get_url() with "?type={value}" conditionally appended (ensure the "all" entry still maps to self.get_url() without a query). Replace the static list with this generated list so filter_terms is derived solely from POST_CONTENT_TYPES.pages/mixins.py (1)
41-41: 💤 Low valueOptional: Use iterable unpacking for content_panels.
Consider using iterable unpacking instead of concatenation for better readability and consistency with modern Python style.
♻️ Proposed refactor
- content_panels = Page.content_panels + ["tags"] + content_panels = [*Page.content_panels, "tags"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pages/mixins.py` at line 41, Replace the list concatenation used to extend content_panels with iterable unpacking for clarity: change the expression that sets content_panels (currently using Page.content_panels + ["tags"]) to use unpacking of Page.content_panels and the "tags" item so the result is built via a single list literal with *Page.content_panels and "tags"; update the assignment to the content_panels attribute in the pages/mixins.py mixin where content_panels and Page.content_panels are referenced.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/urls.py`:
- Around line 410-412: The new Wagtail mount at path("outreach/",
include(wagtail_urls)) is being shadowed by an earlier route handled by
WhitePaperView (and the existing outreach/<slug>/<slug> pattern) so requests
never reach marketing.OutreachHomePage.route(); fix by making the routing
non-overlapping — either move the Wagtail include for "outreach/" so it appears
before the WhitePaperView route in urls.py, or tighten the WhitePaperView
pattern (or its view logic) to exclude the Wagtail path/shape (e.g., require a
different prefix or more specific regex) so marketing.OutreachHomePage.route()
can receive outreach topic/detail requests.
In `@news/tasks.py`:
- Around line 191-200: set_summary_for_event_page currently grabs
page.content[0] (a StreamChild) and passes it to summarize_content, which can
crash because PostPage.content is a StreamField; instead iterate the StreamField
(PostPage.content), extract plain text from text-like blocks (e.g.
RichTextBlock, MarkdownBlock, TextBlock) by reading each StreamChild's
value/text, skip non-text blocks (e.g. PollBlock, URLBlock), join those pieces
into a single str, then use that string for the length check and as the first
arg to summarize_content; update usage sites in set_summary_for_event_page (and
keep save_page_summary_value as the callback) so summarize_content.apply_async
receives the cleaned plain text.
In `@news/utils.py`:
- Around line 22-25: The PostPage guard in set_thumbnail_for_video_page is
inverted: change the condition for PostPage from "not video.post_content_type !=
'video'" to check inequality directly so only non-video pages raise;
specifically, locate the PostPage branch that currently reads "elif
isinstance(video, PostPage) and not video.post_content_type != 'video'" and
replace it with a direct inequality check ("and video.post_content_type !=
'video'") so valid video pages proceed and only pages whose post_content_type is
not "video" trigger the Exception.
In `@pages/blocks.py`:
- Around line 10-14: CustomVideoBlock is missing the thumbnail child so the
StreamField StructBlock schema no longer matches existing migrations and stored
data; restore schema-compatibility by adding a thumbnail field to
CustomVideoBlock (e.g. thumbnail = ImageChooserBlock(required=False)) and import
ImageChooserBlock at the top, ensuring the class definition (CustomVideoBlock)
and templates remain compatible with PostPage.video_thumbnail; alternatively, if
you intend to move thumbnails off the block, create a migration that removes
thumbnail from the content StreamField and include a data migration to strip
thumbnail entries from existing stored JSON, but the simplest fix is to re-add
the thumbnail ImageChooserBlock to CustomVideoBlock.
In `@pages/management/commands/convert_news_entries.py`:
- Around line 71-73: The code currently raises a bare Exception ("No Post Index
Page found...") which should be replaced with a command-native exception to
produce cleaner CLI output; update the raise in convert_news_entries.py (the
raise Exception(...) call) to raise a click.ClickException (or Django's
CommandError if this is a Django management command) and add the appropriate
import (click or CommandError from django.core.management.base) so the command
fails with a concise, user-friendly error message.
- Around line 49-52: Replace title-based dedupe with a stable file identity:
change the Image.objects.get_or_create call that currently uses
get_or_create(title=image.name, ...) to use the image file as the lookup key
(e.g., get_or_create(file=image.name) or
get_or_create(file=<file-field-or-filename>)) so you match existing Image
records by their file, not title; keep the same defaults (width, height, file)
and assign the result to wagtail_image as before (reference:
Image.objects.get_or_create, wagtail_image, image).
In `@pages/tests.py`:
- Line 1: pages/tests.py is empty so conversion logic in convert_news_entries
lacks coverage; add tests that exercise idempotent upsert behavior, content
block mapping, and image/video transfer. Write unit tests targeting the
convert_news_entries function: create fixture source entries with varied content
blocks and media, run convert_news_entries once and assert created targets and
correct mapped blocks/media, run it again and assert no duplicates (idempotent
upsert), and include edge cases (missing media, empty blocks) to validate
conversions and error handling. Ensure tests use the same symbols
convert_news_entries and any helper mappers/uploaders used in the diff so they
fail fast if mapping logic changes.
In `@templates/blocks/custom_video_block.html`:
- Line 1: The template currently bypasses escaping by outputting the EmbedBlock
with "{{ self.video|safe }}"; instead render the EmbedBlock with Wagtail's
include_block so the block's own safe rendering is used (use the {%
include_block %} tag targeting self.video), removing the "|safe" usage and
ensuring the EmbedBlock template is invoked (look for occurrences of
"self.video" and the template "templates/blocks/custom_video_block.html" to
update).
---
Outside diff comments:
In `@news/utils.py`:
- Around line 27-35: The outbound requests to the OEmbed endpoint and to fetch
the thumbnail are missing timeouts and can hang workers; update both
requests.get calls (the one that uses YOUTUBE_OEMBED_ENDPOINT +
f"?url={video.external_url}" and the one inside the thumbnail_file =
File(BytesIO(requests.get(json.get("thumbnail_url")).content), ...)) to pass a
timeout parameter (e.g. timeout=10 or a module-level/configurable constant like
REQUEST_TIMEOUT), and optionally wrap them in a try/except for
requests.Timeout/requests.RequestException to raise or log a clear error if the
call times out.
- Around line 34-37: The thumbnail_file creation passes a set to File(...).name
by using name={f"{video.slug} Thumbnail"}; change this to pass a string instead
(e.g. name=f"{video.slug} Thumbnail") so Django receives a proper filename
string; update the File(...) invocation in the thumbnail_file creation (the call
that wraps BytesIO(requests.get(json.get("thumbnail_url")).content)) to use a
plain formatted string for name and ensure the
BytesIO/requests/get/json.get/video.slug references remain unchanged.
---
Nitpick comments:
In `@marketing/models.py`:
- Line 164: Remove the stray print(path_components) call that emits request
paths to stdout; replace it with a structured logger debug call instead (e.g.,
use the module-level logger or logging.getLogger(__name__)). Locate the
print(path_components) usage (referenced symbol: path_components /
print(path_components)) and change it to logger.debug with a clear message and
the path_components as structured data so it uses the configured logging backend
rather than stdout.
In `@pages/mixins.py`:
- Line 41: Replace the list concatenation used to extend content_panels with
iterable unpacking for clarity: change the expression that sets content_panels
(currently using Page.content_panels + ["tags"]) to use unpacking of
Page.content_panels and the "tags" item so the result is built via a single list
literal with *Page.content_panels and "tags"; update the assignment to the
content_panels attribute in the pages/mixins.py mixin where content_panels and
Page.content_panels are referenced.
In `@pages/models.py`:
- Around line 338-343: The content_panels assignment currently concatenates
BasePage.content_panels with a list; change it to use iterable unpacking so it
mirrors the style used in mixins.py. Update the content_panels definition on the
Page model to produce a single list by unpacking BasePage.content_panels (e.g.,
using the * operator) followed by the added panel names ("content", "image",
"summary", "video_thumbnail"), ensuring the symbol names content_panels and
BasePage.content_panels are used so the intent and ordering remain identical.
- Around line 144-185: The filter_terms list in the pages model is hard-coded
and duplicates logic in news/views.py; instead iterate over the
POST_CONTENT_TYPES mapping to build each term so they stay consistent. Update
the code that sets ctx["filter_terms"] (and any related method using get_url())
to loop through POST_CONTENT_TYPES items, creating dicts with "label" from the
human-readable name, "value" from the content type key, and "url" using
self.get_url() with "?type={value}" conditionally appended (ensure the "all"
entry still maps to self.get_url() without a query). Replace the static list
with this generated list so filter_terms is derived solely from
POST_CONTENT_TYPES.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 73ccd1cf-0c21-49fd-839a-61d4d5efbef2
📒 Files selected for processing (30)
config/settings.pyconfig/urls.pycore/mixins.pymarketing/models.pynews/management/commands/backpopulate_thumbnails.pynews/models.pynews/tasks.pynews/utils.pynews/views.pypages/__init__.pypages/admin.pypages/apps.pypages/blocks.pypages/management/commands/convert_news_entries.pypages/migrations/0001_initial.pypages/migrations/0002_postpage_video_thumbnail.pypages/migrations/0003_alter_postpage_content.pypages/migrations/__init__.pypages/mixins.pypages/models.pypages/tests.pypages/views.pypages/wagtail_hooks.pystatic/css/v3/post-detail.cssstatic/css/v3/posts-list.csstemplates/blocks/custom_video_block.htmltemplates/news/v3/detail.htmltemplates/pages/routable_home_page.htmltemplates/v3/includes/_post_list_card.htmltemplates/v3/posts_list.html
| @@ -0,0 +1 @@ | |||
| # Create your tests here. | |||
There was a problem hiding this comment.
Add migration/conversion test coverage before merge.
pages/tests.py is still a placeholder (Line 1), so critical conversion paths in convert_news_entries are untested (idempotent upsert, content block mapping, image/video transfer). This is a high-risk gap for a data migration feature.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pages/tests.py` at line 1, pages/tests.py is empty so conversion logic in
convert_news_entries lacks coverage; add tests that exercise idempotent upsert
behavior, content block mapping, and image/video transfer. Write unit tests
targeting the convert_news_entries function: create fixture source entries with
varied content blocks and media, run convert_news_entries once and assert
created targets and correct mapped blocks/media, run it again and assert no
duplicates (idempotent upsert), and include edge cases (missing media, empty
blocks) to validate conversions and error handling. Ensure tests use the same
symbols convert_news_entries and any helper mappers/uploaders used in the diff
so they fail fast if mapping logic changes.
javiercoronadonarvaez
approved these changes
Jun 10, 2026
javiercoronadonarvaez
left a comment
javiercoronadonarvaez
left a comment
Collaborator
There was a problem hiding this comment.
LGTM with the latest PR comments. Just a matter of solving the merge conflicts.
herzog0
previously requested changes
Jun 11, 2026
herzog0
left a comment
herzog0
left a comment
Collaborator
There was a problem hiding this comment.
Heya, sorry for the delay, still looking through some things. It's a bit difficult to test all cases as this integration is a bit large. But I found some things that, unfortunately, can't be postponed to a follow-up ticket for improvements.
jlchilders11
force-pushed
the
jc/wagtail-integration
branch
from
29dc2a1 to
9e354d6
Compare
Collaborator
|
Test instructions provided by @jlchilders11 in Slack, for future reference: So the quick setup to see this working:
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
marketing/models.py (1)
164-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove leftover debug
This executes on every request through
OutreachHomePage.route()and will spam stdout/logs in production.🧹 Proposed fix
- print(path_components) if not path_components:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@marketing/models.py` at line 164, Remove the leftover debug output in OutreachHomePage.route() by deleting the print(path_components) statement from the request path handling. Keep the route logic unchanged otherwise, and ensure no other debug printing remains in this method so it does not spam stdout/logs in production.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@news/tasks.py`:
- Around line 287-288: PostPage summarization is hardcoded to "gpt-oss-120b"
instead of using the configured summarization model, so update the PostPage task
call sites to use the same `SUMMARIZATION_MODEL` value used by the Entry path.
Locate the `summarize_content.apply_async` usage in `news/tasks.py` and replace
the literal model argument with the existing config-based model reference so
both code paths stay consistent with environment changes.
- Around line 283-288: Empty PostPage content is still being passed into
summarize_content, which later triggers generate_summary to raise ValueError.
Add an early no-op guard in generate_summary and in the enqueueing path that
logs and returns when content is empty or blank, before
summarize_content.apply_async is called. Use the existing summarize_content and
generate_summary symbols to locate both paths and keep the behavior consistent
across them.
- Around line 299-310: The PostPage link summary fetch in the content extraction
flow is using raw requests.get instead of the hardened helper, so update the
fetch logic around the external_url handling to use safe_get just like the Entry
path. Make sure the same SSRF protection is preserved by catching UnsafeURLError
alongside the existing requests.RequestException handling, and keep the rest of
the extraction flow (response.text, extract_content, and logging in the current
function) unchanged.
- Around line 141-146: The PostPage summary save callback should ignore empty
model output, since summarize_content can return None or an empty string. Update
save_page_summary_value to mirror save_entry_summary_value by checking summary
before fetching PostPage and assigning to page.summary, so empty summaries are
not persisted or re-triggered. Use the save_page_summary_value and
save_entry_summary_value callbacks as the reference points for the guard
behavior.
In `@pages/models.py`:
- Around line 226-231: Guard the next-post lookup in the page context builder so
drafts without a publish timestamp do not filter on first_published_at__gt=None.
In the method that sets ctx["next_post_items"] and ctx["related_posts"], check
self.first_published_at first; if it is missing, assign an empty next-post list
and skip the publish-date filter, otherwise keep the existing pages.filter(...)
logic.
- Around line 35-44: The route resolution in route() is using request.path
instead of the Wagtail-provided path_components, which can ভুলly treat the mount
prefix as the child slug. Update route() to derive base/rest from
path_components and keep the recursive call on
match_child.specific.route(request, rest), so routing works correctly under
mounted paths.
- Around line 325-330: The external_url method returns the StreamField child
object for Link posts instead of the actual URL string. Update the Link branch
in external_url to access the child’s underlying value, consistent with the
Video branch’s .value.url usage, so callers receive a URL string rather than a
block object.
- Around line 292-294: `PostPage.get_absolute_url` is currently decorated as a
cached property, which turns it into a string attribute and breaks callers that
invoke `obj.get_absolute_url()`. In `pages.models.PostPage`, remove the
`@cached_property` decorator from `get_absolute_url` and keep it as a normal
method returning `self.url` so existing method-style usages continue to work.
---
Nitpick comments:
In `@marketing/models.py`:
- Line 164: Remove the leftover debug output in OutreachHomePage.route() by
deleting the print(path_components) statement from the request path handling.
Keep the route logic unchanged otherwise, and ensure no other debug printing
remains in this method so it does not spam stdout/logs in production.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e82a94d0-c5d5-4930-a054-3a695e717efc
📒 Files selected for processing (31)
config/settings.pyconfig/urls.pycore/mixins.pymarketing/models.pynews/management/commands/backpopulate_thumbnails.pynews/models.pynews/tasks.pynews/utils.pynews/views.pypages/__init__.pypages/admin.pypages/apps.pypages/blocks.pypages/management/commands/convert_news_entries.pypages/migrations/0001_initial.pypages/migrations/0002_postpage_video_thumbnail.pypages/migrations/0003_alter_postpage_content.pypages/migrations/0004_alter_postpage_content.pypages/migrations/__init__.pypages/mixins.pypages/models.pypages/tests.pypages/views.pypages/wagtail_hooks.pystatic/css/v3/post-detail.cssstatic/css/v3/posts-list.csstemplates/blocks/custom_video_block.htmltemplates/news/v3/detail.htmltemplates/pages/routable_home_page.htmltemplates/v3/includes/_post_list_card.htmltemplates/v3/posts_list.html
✅ Files skipped from review due to trivial changes (4)
- pages/admin.py
- pages/views.py
- pages/tests.py
- static/css/v3/post-detail.css
🚧 Files skipped from review as they are similar to previous changes (13)
- pages/apps.py
- templates/v3/posts_list.html
- news/views.py
- templates/news/v3/detail.html
- templates/v3/includes/_post_list_card.html
- news/models.py
- core/mixins.py
- news/management/commands/backpopulate_thumbnails.py
- config/settings.py
- config/urls.py
- static/css/v3/posts-list.css
- pages/blocks.py
- pages/management/commands/convert_news_entries.py
herzog0
dismissed stale reviews from julhoang, gregjkal, and themself
Issues were addressed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement Wagtail Integrations:
Mixins
TaggableMixin- Adds the ability to tag pages for categorizationRouting
RoutableHomePage- New home page for all wagtail interactions that allows for forwarding routing to child pages, e.g. if you get to wagtail through/outreach/, you will be routed through theOutreachHomePagePosts
This is the bulk of the work, and is a porting of the existing
EntryModel to WagtailPostIndexPage- Index Page for Posts, implements most of the logic in formatting found in thenews.views.EntryListView. NOTE: this uses queryparams for filtering the child pages, rather than seperate URLsPostPage- Functional translation ofEntry, but uses a stream field rather than having separate models for different types of content. These can be filtered by the content of the stream field blockNOTES:
DEPLOYMENT NOTES:
When deploying this, a
RoutableHomePageshould be created and the existingOutreachHomePageshould be moved under it to respect the new routing methods.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor