Story 2378: Webpage Integration: Posts Feed - #2568
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
ycanales
changed the title
About the search issue discussed in yesterday's demo, here's a proposal. I want to hear your thoughts and if it's OK to include it in this PR: Accent-insensitive search (
|
| Input | Lexemes |
|---|---|
Rubén Pérez |
ruben perez |
ñandú |
nandu |
ação |
acao |
Straße Åström Łukasz |
strass astrom lukasz |
Đorđe Nguyễn Müller |
dord nguyen muller |
Ольга 北京 |
ольга 北京 (unchanged) |
Matching is symmetric, so an accented query still finds accented content:
| Query | english (today) |
english_unaccent |
|---|---|---|
perez against Rubén Pérez |
no match | match |
pérez against Rubén Pérez |
match | match |
acao against ação São Paulo |
no match | match |
Olga against Ольга |
no match | no match |
Limits worth stating up front
SEARCH_CONFIGis per-backend, not per-model, so this changes every Wagtail search on the site and the reindex covers everything.- Autocomplete is unaffected. modelsearch hardcodes
autocomplete_config = "simple"to disable stemming, andSEARCH_CONFIGdoes not reach it, so admin page search stays accent-sensitive. unaccentfolds Latin scripts from a fixed rules file. It does not transliterate Cyrillic, Greek or CJK, and extending the rules is not possible on managed Postgres.- Folding merges genuinely distinct words:
résuméandresumebecome one lexeme. Noise-level for this site, but a real narrowing of the index.
17ccd82 to
4003287
Compare
jlchilders11
left a comment
jlchilders11
left a comment
There was a problem hiding this comment.
I have one question before approving, but the code looks good.
|
|
||
|
|
||
| class EntryListView(V3Mixin, ListView): | ||
| class EntryListView(ListView): |
There was a problem hiding this comment.
While not opposed to this change, I think there is a larger discussion to be had on intent for the V3 soft launch overlap. My understanding was the intent was to run the Entry and Page flows in parallel for comparison, but if that isn't necessary this is a good call.
There was a problem hiding this comment.
+1 to this on team alignment and roll-out strategy!
There was a problem hiding this comment.
Consolidated back, thanks for the feedback both :)
There was a problem hiding this comment.
Hi @ycanales ! I've tested this and found the core feature work great, however I have a couple of requests that I'd love to hear your thoughts:
1/ On Figma, there's a new filter option for "Hot Posts", which is essential posts ranked by views. This is similar to how the "Posts from the Boost Community" card on Homepage & Community page are currently working. Can we add that filter option here as well?
Also, it seems like the new filter buttons on Figma looks a bit shorter than our current implementation. Seems like the design was updated at some point but I'm not sure if it was intentional 🤔. @henryajisegiri Would you mind helping us confirm this design change?
2/ Currently for the search input field, we have to hit "Enter" or click the arrow button to trigger the search; whereas on the Libraries page and the Algolia search modal, we have search debounce. Should we consider supporting that here? 🤔 To be fair, I've not noticed now that "What are you trying to find?" card on Homepage is also missing the debounce feature so this is just a nice-to-have I think. (cc @henryajisegiri – please let us know if you have a preference here)
3/ On the Post item, I think we've lost the "Blogpost" -> "Blog" mapping. Would you mind adding it back?
| @staticmethod | ||
| def _resolve_author(value): | ||
| if not value.isdigit(): | ||
| return None | ||
| return get_user_model().objects.filter(pk=value).first() |
There was a problem hiding this comment.
I think we should flag this to the team so that we can come back and improve this filtering once we settle on the username conversations and use that for author filtering on this Post page as well, instead of using index number. Also there's no design for setting ?author= query currently (cc @henryajisegiri )
|
|
||
|
|
||
| class EntryListView(V3Mixin, ListView): | ||
| class EntryListView(ListView): |
There was a problem hiding this comment.
+1 to this on team alignment and roll-out strategy!
|
Please see my response below:
|
d2e5c18 to
709d966
Compare
julhoang
left a comment
julhoang
left a comment
There was a problem hiding this comment.
Looks great to me, thank you so much for all the updates @ycanales !
I just have 2 non-blocking requests for some clean up on unused functions/context, but I definitely can approve as-is:
header_textonEntryListViewand its five subclasses is now orphaned innews/views.py:138(this PR replaced it withfeed_context)- The
post-filterdispatch branch is now unreachable innews/views.py:217
The page-tree fixture was local to test_commands.py. Register pages.tests.fixtures so the feed tests can reuse it, and add the two pieces they need on top: a default Site (pytest runs with --no-migrations, so Wagtail's initial data never exists and page.url returns None without one) and a PostPage factory. The factory indexes each page explicitly because indexing is queued with transaction.on_commit, which never runs under the django_db fixture.
…o the posts feed The feed rendered posts and post type pills, but the search box sat outside the form and the library dropdown was fed a context variable no view set, so neither did anything. The pills navigated with window.location.assign, discarding every other query parameter, and zero results rendered nothing at all. Search, pills, library and author are now one GET form writing to ?q=, ?type=, ?library= and ?author=, parsed and validated by PostFeedFilters. Filters run as plain queryset filters and, when a term is present, are handed to Wagtail search through a pk subquery: the backend rejects StreamField lookups outright and tag lookups without a FilterField, and returns SearchResults rather than a queryset. PostPage gains search_fields covering body text, summary, post type, tags and author name; body text is stripped of markup so a search for "p" does not match every rich text post. Zero results render an empty state, plus up to three posts from the selected library with the search term dropped. Submitting the form drops ?page=, which is what resets pagination. The header nav now points at the Wagtail feed. EntryListView keeps serving legacy Entry rows on the v2 template rather than sharing a context contract with it. Existing posts need ./manage.py update_index once per environment before they are searchable.
The dropdown dispatches field-change while setting `selected`, before Alpine has written it to the hidden input the form submits, so choosing a library submitted an empty `library=` and the feed came back unfiltered. Defer the submit a tick. Also pass the request to the nav's feed-page lookup: without it Page.url falls back to a fully qualified URL once a second Site exists, so the nav rendered an absolute href where every other link is a path. Both found by the E2E spec, neither visible to the view tests.
The v3 UI now has a single entry point for the feed. `EntryListView` and its
five type subclasses redirect to the PostIndexPage URL while the flag is
active, mapping each subclass's `filter_value` onto `?type=` so an inbound
link to /news/video/ arrives filtered.
The redirect is temporary rather than permanent, because the flag can be
switched back off and a cached 301 would strand v2 visitors on a 404. It is
skipped when no PostIndexPage exists, where `posts_feed_url` falls back to
this view's own URL and redirecting would loop.
`header_context` also exposes `posts_feed_url` as its own context key, so the
homepage's "View all posts" CTA resolves the feed the same way the header nav
does. It previously hardcoded {% url 'news' %}, sending v3 visitors to the
legacy list while the working feed sat unlinked.
With the flag off nothing changes: the legacy lists render as before and the
new feed stays 404.
Anchor the two assertions that could not fail: the library dropdown check now matches the option value together with the selected attribute and the Alpine seed, and the search check is scoped to the search input rather than to a page that already contains the term as a library slug. Move the feed header wording and precedence cases onto PostFeedFilters, which is where the property lives, and keep only the escaping case over HTTP. Drop the tests that duplicated a stronger assertion, exercised no feed logic, or stood in for browser behaviour, fold the out-of-range page into the unresolvable-values parametrize, narrow the logged out card to the copy this template owns, move the nav test to module level, and add a searched variant of the query count guard so the prefetches are covered on the pk-subquery path too.
The form-level Enter handler ran on every key press that bubbled up to it, so opening the dropdown or picking an option submitted the form instead. Skip the handler when the event comes from inside the dropdown, which binds Enter itself for both. Enter elsewhere still submits, which is what lets a user clear a search: the search field's own submit button is disabled while the box is empty, so implicit submission does nothing there.
Selecting a library submits the form, so the control the user was on is destroyed with the document and focus falls back to the body. Park the field name in sessionStorage and restore focus on the way back in. Not a query param: the URL gets shared and copied, and a cold visit to such a link should not have focus pulled off the page heading. Submit synchronously while here. field-change fires as the dropdown sets its own state, before Alpine has written the value to the hidden input the form serialises, which is why the submit was deferred a tick. Reading the value off the event removes the wait. The tick was not free: it is scheduled off Alpine's reactivity queue, and a submit parked behind one could sit unflushed until the next interaction happened to drain it, leaving a click on an option looking like it did nothing.
Arrowing from All to Blogs submits the form, so the pill the user had just landed on was destroyed with the document and focus fell back to the body, ending the keyboard run. Reuse the dropdown's mechanism: park the field name, then focus the checked radio on the way back in. Two things the pills need that the dropdown did not. Their ring hangs off :focus-visible, and that heuristic starts fresh in the new document, so a restored pill would hold focus with nothing on screen to show it, and the radio itself is visually hidden. Record whether the change came from the keyboard, and only then mark the pill for a matching rule in post-filter.css. A mouse user does not get an outline they never had. Also switch the component to $root. $el is whichever element Alpine is evaluating, so once these methods were called from a handler on a descendant it resolved to the filter div, and requestSubmit threw after the marker had already been stored.
Searching submits the form, so the box the user had just typed into went with the document and refining a term meant reaching for it again. Cover both ways it submits: Enter now routes through the same path the other controls use, and the arrow button is caught by a delegated click, since it is rendered by the field include and cannot be bound directly. That one only records, leaving the browser to submit as it already did. Split the recording out of submitFrom so the button can use it without submitting twice. Put the caret after the term as well, so a refinement carries on from the end instead of in front of what was typed. Enter keeps naming its own control rather than assuming the search box, so pressing it on a pill still comes back to that pill, and pressing it somewhere unnamed submits without recording, as before.
The feed was reachable only at the Wagtail index page's own URL, which every environment names for itself, so nothing in code could link to it without a lookup and the nav highlight had to hardcode one slug. Put it on the news route instead, the way every other v3 page in the site swaps templates behind the flag: off renders the legacy Entry list, on renders the feed over the PostPage tree. The per-type lists forward to it filtered, and where no index page has been created yet the legacy list stays rather than an empty page. Drops the nav path entry, the header lookup and the homepage CTA rewiring, all of which existed only to name the other URL.
78bb4d2 to
f8c94db
Compare
5 participants
Issue: #2378
Summary & Context
Makes the posts feed's controls actually work: search, the library filter, a header that reflects the filter state, URL-driven state, an empty state, and a user card wired to real data.
Stacked on #2562 (
jc/2376-post-detail-page), which movestagsontoPostPage. Review that one first.Changes
Search
search_fieldstoPostPagecovering body text,summary, post type, tag name and slug, and the author's display name.titleis already indexed byPage.search_fields.PostPage.search_body, which runs the StreamField throughget_searchable_content()and strips markup.Filters and feed state
pages/feed.pyholding the content-type table (moved out ofpages/models.py) andPostFeedFilters, a frozen dataclass that parses and validates?q=,?type=,?library=and?author=. Every value is resolved to a real object there, so nothing raw from the querystring can reach the feed header. Unknown values degrade to the unfiltered feed.POST_CONTENT_TYPES, resolving the# TODOthat was already in the file._PostContentTypegainslabel(plural, for the pill) andheader_label(singular, for the header), because neither matchescontent_type: the header would otherwise read "Blogpost Posts".disabled. They currently navigate to a bare URL, which silently behaves like "All".library_filter_options()intolibraries/utils.py, shared withEntryListView's old copy. Labelled withdisplay_nameso the dropdown matches the header wording ("Boost.Beast", not "Beast").Template
actiondrops the current querystring, which is what resets pagination:pageis not a form field, so any submit returns to page 1.window.location.assignblock. Submission is now declarative Alpine:field-changeand pillchangecallrequestSubmit(), and Enter submits explicitly (the search field's own submit button disables itself on an empty box, so without that a user could not clear a search).field-changesubmit is deferred with$nextTick. The dropdown dispatches the event as it setsselected, before Alpine has written it to the hidden input the form submits, so submitting immediately sent an emptylibrary=. Caught by the E2E spec, not by the unit tests.<noscript>Filter button and the native<select>fallback both still submit.?author=in a hidden input, so the first pill click does not silently drop it.Empty state
templates/v3/includes/_post_empty_state.htmland the{% else %}branch that_post_list_card.htmlnever had, plus.post-empty-statestyles ported from the library page's treatment.User card
badge_name='Bug Catcher'androle='Contributor', and dropbadge_icon_src, which_user_card.htmldoes not accept and so never rendered. Passrole=u.roleinstead.#. It now reads Sign Up Now and opens the signup page. Heading and copy come from the include's own defaults, which already match the AC verbatim.Routing
/news/, the legacyEntrylist, which rendered the same v3 template from a different context; the two would have had to share a context contract.EntryListViewdrops back to its v2 template and keeps serving legacy entries.posts_feed_url(request)passes the request through toget_urlso the nav renders a path rather than the fully qualified URLPage.urlfalls back to once a second Site exists.Tests
pages/: search per indexed field, each filter, every filter combination, the six header strings, pagination, the empty state and its fallback, URL-driven state, the user card in both auth states, and v3 flag gating. Plus a query-count guard, since each card readsitem.authoranditem.tag.pages/tests/fixtures.py(registered inconftest.py) with the page tree, a default Site and aPostPagefactory. The factory indexes each page explicitly: indexing is queued withtransaction.on_commit, which never runs under thedjango_dbfixture, so every search test would otherwise return zero rows with no error.Rejected alternatives
FilterFields so the tag filter survives.search(). The pk subquery needs none, and aFilterFieldon tags would make a post-search.filter()merely pass validation, reintroducing join duplicates.DISTINCTis not available as a fix either: it conflicts with the rankORDER BYthe backend injects.icontainsQ-filter instead of Wagtail search, aslibraries/api.pydoes. It needs no index rebuild, but gives no relevance ranking and cannot reach StreamField body text, which is the bulk of what people search for. The issue's dev note asks forqs.search(...)and that is what this does.tags__slugfor the library filter.tagged_items__tag__slugis used instead: same SQL, but it resolves against the concretePostPagetable under either tag arrangement, so it survives a merge with branches whereTaggedContent.content_objectstill points atwagtailcore.Page(theretags__slugraisesProgrammingError: column pages_postpage.id does not exist).LIMIT 1per request, and a cache would need invalidating on slug change and would leak between tests through the shared Redis. The query-count guard innews/tests/test_views.pymoves from 10 to 11 for it../manage.py update_indexmust run once per environment on deploy. Signals only index onpost_save, so everyPostPagecreated before this lands is invisible to search until it does./pages/posts/./news/still exists and still serves legacyEntryposts, now on the v2 template. Worth a second opinion on whether/news/should redirect.User.roleon this branch is still a hardcoded"Contributor"stub and there is no badge or org-affiliation source, so the card cannot show a real role, badge or affiliation today. It is wired tou.roleso it improves for free when Story 2443: User Profile Integration – User Roles #2527 lands.Boost.SQLiteas one lexeme, so searching the bareSQLitedoes not match it by title. Library-tagged posts are still reachable through the tag index. There is a test documenting this.Nonein dev, that cache is the first thing to flush.Peer-Testing Guidelines
Setup
jc/2376-post-detail-page) and rundocker compose exec web python manage.py migrate.v3waffle flag to Everyone at http://localhost:8000/admin/. This is also how the logged-out state is tested.docker compose exec web python manage.py convert_news_entries.docker compose exec web python manage.py update_index.Search
Results for "<term>".Filters
News Posts,Blog Posts,Video Posts,Link Posts, andAllshould clear it. Discussions, Achievements and Issues should be visibly disabled.Boost.<Library> Posts. Combine it with a post type: the header shows the post type only, by design.URL state
?type=nonsense,?library=nope,?author=abcand?page=999. Each should degrade quietly to a sensible feed, never a 500.Empty state and user card
Member Since <year>and a Create Post button. In a private window it shows "Create an account" and Sign Up Now.<select>and a visible Filter button appear, and submitting still applies search, pills and library.Screenshots
Self-review Checklist
Frontend