-
Notifications
You must be signed in to change notification settings - Fork 909
Exclude draft cards from search index #2418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
2e04432 to
17e47e2
Compare
|
Would a script to remove any already-indexed ones be pertinent? |
kevinmcconnell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, @flavorjones!
I initially thought we should just filter the query on the card status, since we end up needing to fetch the cards to show the results anyway. I can't tell off-hand whether there's any practical difference in performance between the two approaches because of that.
But, the more I thought about it, I think what you're doing here is a more elegant approach in any case. There's no point in indexing something you aren't going to return! Neat idea.
| if searchable? | ||
| search_record_class.upsert!(search_record_attributes) | ||
| else | ||
| remove_from_search_index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no way for a card to get "unpublished" back to draft, right? Because if we ever did that, I think any comments on the card would remain in the search index, is that right?
I think this doesn't matter in practice because we can't create comments until a card is published, and we can't un-publish a card. But I'm just checking :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevinmcconnell You're correct, the UI does not allow setting cards back to draft once they're published. It doesn't look like it's possible via the API, either, since status is not a permitted parameter for the CardsController.
A related interesting edge case is that it's currently possible to add comments to a draft card via the API -- but I've got a separate branch that will address that gap shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR to prevent comments on draft cards: #2423
17e47e2 to
bbce3ee
Compare
|
@northeastprince Good idea, I've pushed an additional commit with a script to clean that up. |
Only index cards and comments if their card is `published?`, using a new method `searchable?` that is implemented on both Card and Comment. This prevents draft cards and their comments from being indexed. When drafts are published, the existing `update_in_search_index` callback creates the record via `upsert!`, or else ensures unpublished records are removed from the index. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
One-time script to clean up any search records that were indexed before the searchable? guard was added. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
bbce3ee to
a9c6bc2
Compare
Summary
Prevents draft cards from appearing in search results by guarding the search indexing callbacks. This is an alternative approach to #2408 — instead of filtering drafts at query time, this prevents them from being indexed in the first place.
Details
searchable?to Card (returnspublished?) and Comment (returnstrue)create_in_search_indexandupdate_in_search_indexwithsearchable?The first commit is simply updating the existing search tests to use published cards. The second commit 17e47e2 represents the behavior change and includes new tests.
ref: https://app.fizzy.do/5986089/cards/3829