Support to search for multiple tags #8284
Conversation
| if controller.instance_of?(TagsController) | ||
| tag_path(:name => @stream.tag_name, :max_time => time_for_scroll(@stream)) | ||
| tag_path(name: @stream.tag_names, max_time: time_for_scroll(@stream)) |
There was a problem hiding this comment.
Rails/HelperInstanceVariable: Do not use instance variables in helpers.
| elsif controller.instance_of?(PeopleController) | ||
| local_or_remote_person_path(@person, :max_time => time_for_scroll(@stream)) | ||
| local_or_remote_person_path(@person, max_time: time_for_scroll(@stream)) |
There was a problem hiding this comment.
Rails/HelperInstanceVariable: Do not use instance variables in helpers.
There was a problem hiding this comment.
Its an old module - should this be changed?
There was a problem hiding this comment.
It doesn't have to but it doesn't hurt
There was a problem hiding this comment.
The code looks good to me and the search for multiple tags works fine.
Some parts of the overall feature look a bit incomplete - profile search and follow tags button in the multi tag case.
I don't know what's our policy for the case like that - are we okay shipping it like that and then improving in some follow up PRs?
| def self.all_tag_stream(tag_ids) | ||
| if tag_ids.empty? | ||
| # A empty list means an unknown tag in Taggings list | ||
| tag_ids = [-1] |
There was a problem hiding this comment.
Doesn't it make the request to return an empty result all the time? If so, couldn't we just return none ? I think that could reduce some unnecessery load on the DB
| elsif controller.instance_of?(PeopleController) | ||
| local_or_remote_person_path(@person, :max_time => time_for_scroll(@stream)) | ||
| local_or_remote_person_path(@person, max_time: time_for_scroll(@stream)) |
There was a problem hiding this comment.
It doesn't have to but it doesn't hurt
| @@ -5,15 +5,15 @@ | |||
| # the COPYRIGHT file. | |||
|
|
|||
There was a problem hiding this comment.
🆗 this file doesn't seem to containt anything other than code style changes
| .btn.btn-danger.tag_following_action{data: {name: @stream.tag_names}} | ||
| = t(".stop_following", tag: @stream.tag_names) | ||
| - else | ||
| .btn.btn-danger.tag_following_action{data: {name: @stream.tag_name}} | ||
| = t(".stop_following", tag: @stream.tag_name) | ||
|
|
||
| .btn.btn-success.tag_following_action{data: {name: @stream.tag_names}} | ||
| = t(".follow", tag: @stream.tag_names) |
There was a problem hiding this comment.
This code is for mobile, but the problem I describe exist for both mobile and desktop implementations.
So when I search for "#photo #cats", what I see there on the button to follow and stop following the tags. If I interact with the button it subscribes me to the combined tag of #photocats which is probably not what the user would expect to happen
| if tag_ids.empty? | ||
| @posts = StatusMessage.none | ||
| else | ||
| @posts ||= StatusMessage.any_user_tag_stream(user, tag_ids) | ||
| end |
There was a problem hiding this comment.
I'd rather move this condition under any_user_tag_stream. We probably want such behavior in any usage of any_user_tag_stream
| end | ||
|
|
||
| def tagged_people | ||
| @people ||= ::Person.profile_tagged_with(tag_name).paginate(:page => people_page, :per_page => people_per_page) | ||
| @tagged_people ||= ::Person.profile_tagged_with(tag_names).paginate(page: people_page, per_page: people_per_page) |
There was a problem hiding this comment.
This doesn't really work with multiple tag names, is it expected?
The search results shows nothing, even if there is some profiles with multiple tags.
| @@ -41,22 +41,22 @@ | |||
|
|
|||
| describe ".tag_steam" do | |||
There was a problem hiding this comment.
The tests changes you're making here are 🆗
But I don't see any tests for the new functionality you're adding here.
I see your other pull requests add tests so I assume you just didn't want to invest time in the tests because you weren't sure it will get merged soon?
It allows to search for more than one tag in the search-field.
No frills, just search for "#cat #photos" or "#cat #painting" to see the feature.
Can be tested on https://societas.online