-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Support to search for multiple tags #8284
base: develop
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rails/HelperInstanceVariable: Do not use instance variables in helpers.
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.
Its an old module - should this be changed?
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.
It doesn't have to but it doesn't hurt
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't have to but it doesn't hurt
@@ -5,15 +5,15 @@ | |||
# the COPYRIGHT file. | |||
|
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -41,22 +41,22 @@ | |||
|
|||
describe ".tag_steam" do |
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.
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