Skip to content
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

AO3-6522 Use standard analyzer for bookmarker and creators on bookmark search #4647

Merged
merged 3 commits into from
Jun 17, 2024

Conversation

potpotkettle
Copy link
Contributor

Pull Request Checklist

Issue

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

Purpose

Change the Elasticsearch analyzer for boorkmarker and creators so that numbers in names (like "testy2") are not ignored when searching.

Testing Instructions

I think the instructions given in the Jira issue will work. When testing in a local environment, you might need to (re)index new users and bookmarks you might have to create, and activate resque.

References

n/a

Credit

Potpotkettle (they/them)

user_pseud = User.find_by(login: user).default_pseud
work1 = FactoryBot.create(:work, title: title)
FactoryBot.create(:bookmark,
bookmarkable_id: work1.id,
Copy link
Contributor Author

@potpotkettle potpotkettle Oct 29, 2023

Choose a reason for hiding this comment

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

This will make it inconsistent with other similar lines with FactoryBot.create in the same file. Should I change them as well? I think the substrings "bookmark" look aligned in the current (apparently illegal) style.

@potpotkettle
Copy link
Contributor Author

The high-level tests added here might be an overkill for the small change in analyzers.

A unit test might be preferable, but I couldn't figure out how to do that.

Copy link
Contributor

@ceithir ceithir left a comment

Choose a reason for hiding this comment

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

Out of curiosity, do you know the reason simple was used instead of standard in the first place?

user_pseud = User.find_by(login: user).default_pseud
work1 = FactoryBot.create(:work, title: title)
FactoryBot.create(:bookmark,
bookmarkable_id: work1.id,
Copy link
Contributor

@ceithir ceithir Oct 29, 2023

Choose a reason for hiding this comment

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

https://github.com/otwcode/otwarchive/wiki/Coding-Standards#user-content-Previewing_Hound_comments

You can (and should!) ignore anything that isn't related to the lines you've changed.

I thus guess there is no issue leaving the other calls as they are, for them to be fixed bit by bit in future PRs.

Also, this is not something Hound detects (dunno if that would be easy to add?), but the standard syntax for Cucumber expressions has changed, moving away from (explicit) regex:
https://github.com/otwcode/otwarchive/wiki/Automated-Testing#user-content-Our_Cucumber_Standards

@potpotkettle
Copy link
Contributor Author

Out of curiosity, do you know the reason simple was used instead of standard in the first place?

I have no idea but it was the simple analyzer at least in 2014 and has been basically kept that way since then: #1840

@potpotkettle potpotkettle changed the title AO3-6522 Use standard analyzer for bookmarker and creators AO3-6522 Use standard analyzer for bookmarker and creators on bookmark search Oct 30, 2023
Comment on lines 319 to 320
bookmarkable_id: work1.id,
pseud_id: user_pseud.id)
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: FactoryBot can automatically get the ID we just pass the object, which is IMO a bit clearer

Suggested change
bookmarkable_id: work1.id,
pseud_id: user_pseud.id)
bookmarkable: work1,
pseud: user_pseud)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this is now done.

@brianjaustin brianjaustin added Reviewed: Ready to Merge Needs Reindex Contains changes that will require Elasticsearch reindexing and removed Awaiting Review labels Nov 2, 2023
@brianjaustin brianjaustin merged commit 03369dc into otwcode:master Jun 17, 2024
24 checks passed
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