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.

Sign up for GitHub

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

Jump to bottom

AO3-5830 Tag search: Allow searching by fandom, searching for non-canonical tags, and sorting results #4144

Merged
merged 26 commits into from
Apr 13, 2022
Merged

AO3-5830 Tag search: Allow searching by fandom, searching for non-canonical tags, and sorting results #4144

merged 26 commits into from
Apr 13, 2022

Conversation

weeklies
Copy link
Contributor

@weeklies weeklies commented Feb 1, 2022 •
edited
Loading

Pull Request Checklist

Issue

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

Purpose

  1. Create new Tag Search form with sorting and searching by fandom
  2. Update and create new tests
  3. Remove old form and update Browse Tags page

Testing Instructions

  • Try searching by tag name, tag type, wrangling status
  • Try sorting by uses and kudos
  • Try filtering by fandom(s) (Test for single, multiple, non-existent and non-canonical fandoms)
  • Check the Browse Tags page for new description of tag cloud and link to Search Tags

References

Credit

weeklies (she/her)

@weeklies
Copy link
Contributor Author

weeklies commented Feb 1, 2022 •
edited
Loading

A note about autocomplete in the Fandoms field:

I believe aria-describedby appears to be conflicting with the autocomplete functionality, as adding both of them in results in errors. These were the two ways I tried:

<%= f.text_field :fandom_names, autocomplete_options("fandom"), "aria-describedby" => "fandom-field-description" %>

ArgumentError in Tags#search
unknown keyword: "aria-describedby"

<%= f.text_field :fandom_names, "aria-describedby" => "fandom-field-description", autocomplete_options("fandom") %>

/otwa/app/views/tags/_search_form.html.erb:15: syntax error, unexpected ')', expecting =>
/otwa/app/views/tags/_search_form.html.erb:65: syntax error, unexpected `end', expecting ')'
/otwa/app/views/tags/_search_form.html.erb:67: syntax error, unexpected `ensure', expecting ')'
/otwa/app/views/tags/_search_form.html.erb:69: syntax error, unexpected `end', expecting ')'
/otwa/app/views/tags/search.html.erb:5:in

I'm not sure about how to go about fixing this, so I've left only the aria-describedby in for the time being, sorry about that!

sarken
sarken requested changes Feb 14, 2022
View reviewed changes
@weeklies
Copy link
Contributor Author

Also, I hope it's okay to mark the comments that I think I've addressed in the changes as resolved, please let me know if I shouldn't!

@weeklies
AO3-5830 Make changes from feedback
bbd7cf3 
* Use fandoms instead of fandom_names

* Make fandom filter ignore non-existent fandoms

* Add new test scenarios for non-existent fandoms
@weeklies
Hound fixes
20f88cb
@redsummernight redsummernight mentioned this pull request Mar 12, 2022
Merged
5 tasks
@weeklies
Copy link
Contributor Author

I don't think this was in the specification, but should I let the sorting direction be configurable? Right now, it's defaulting to descending order.

@weeklies
Copy link
Contributor Author

weeklies commented Mar 15, 2022 •
edited
Loading

I'm not sure why sorting by uses in ascending order isn't working — if someone could point me in the right direction, that would be great.

@tickinginstant
Copy link
Contributor

The taggings_count_cache column in the database (which is used to set the uses field in ES) is only updated when Tag.write_redis_to_database is called. Unfortunately, Tag.write_redis_to_database avoids tag callbacks on write (presumably for speed reasons, since it's called once every 2 minutes and updates all tags that have had their count viewed recently, not just those that have had their count changed), which means that the uses field in ES is not kept up-to-date. That's going to have to change if you want to allow users to sort on the uses field.

@sarken
Copy link
Collaborator

sarken commented Mar 15, 2022

Let's just do sorting by name and date created.

@weeklies
Copy link
Contributor Author

Aw, that's unfortunate. Will make the changes.

redsummernight
redsummernight reviewed Mar 16, 2022
View reviewed changes
@redsummernight redsummernight added Needs Reindex Contains changes that will require Elasticsearch reindexing and removed Needs Reindex Contains changes that will require Elasticsearch reindexing labels Mar 17, 2022
redsummernight
redsummernight reviewed Mar 19, 2022
View reviewed changes
Copy link
Member

@redsummernight redsummernight left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! Just one last minor thing.

redsummernight
redsummernight reviewed Mar 19, 2022
View reviewed changes
app/models/search/tag_search_form.rb Outdated
def sort_options
[
%w[Name name],
%w["Date Created", "created_at"]
Copy link
Member

Choose a reason for hiding this comment

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

This should be ["Date Created", "created_at"], without the %w.

@redsummernight redsummernight merged commit 7f5816d into otwcode:master Apr 13, 2022
@weeklies weeklies mentioned this pull request Apr 14, 2022
Merged
5 tasks
@metallicity metallicity mentioned this pull request Jan 16, 2025
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sarken sarken sarken approved these changes

@redsummernight redsummernight redsummernight approved these changes

Assignees
No one assigned
Labels
Reviewed: Ready to Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@weeklies @tickinginstant @sarken @redsummernight