Skip to content

Conversation

@Cubostar
Copy link
Contributor

Pull Request Checklist

Issue

https://otwarchive.atlassian.net/browse/AO3-7119

Purpose

This PR fixes a bookmark indexing issue by changing the Elasticsearch bookmark analyzer from simple to standard.

Credit

Cubostar, he/him

@anmazz
Copy link
Contributor

anmazz commented Nov 25, 2025

(Not an official volunteer, so please defer to them if they disagree with me!)

I’d recommend adding a Cucumber .feature test here so the functionality is verified. You can probably just adapt the testing steps outlined in the ticket.

tag: {
type: "text",
analyzer: "simple"
analyzer: "standard"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may be able to remove this line, the standard analyzer should be the default, but I would wait until an official volunteer confirms before removing. Looking through the code there are some explicit instances of analyzer: "standard", so might be totally fine as is!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I'll wait until there's confirmation. Thanks for reviewing my PR!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine to keep explicit. Generally, I would say that we have no set style preference for this situation.

Personally, I like when a decision like this has an easy to follow git blame for why it is the way it is, so keeping this line makes it easier to git blame -> find this pr -> find the related test and jira issue

Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@Bilka2 Bilka2 added Reviewed: Ready to Merge Needs Reindex Contains changes that will require Elasticsearch reindexing and removed Coder Has Actioned Review labels Nov 30, 2025
@Cubostar
Copy link
Contributor Author

No problem, thanks for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Reindex Contains changes that will require Elasticsearch reindexing Reviewed: Ready to Merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants