-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,15 +5,15 @@ | |
# the COPYRIGHT file. | ||
|
||
module StreamHelper | ||
def next_page_path(opts ={}) | ||
def next_page_path(_opts={}) | ||
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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't have to but it doesn't hurt |
||
elsif controller.instance_of?(StreamsController) | ||
next_stream_path | ||
else | ||
raise 'in order to use pagination for this new controller, update next_page_path in stream helper' | ||
raise "in order to use pagination for this new controller, update next_page_path in stream helper" | ||
end | ||
end | ||
|
||
|
@@ -22,6 +22,7 @@ def reshare?(post) | |
end | ||
|
||
private | ||
|
||
# rubocop:disable Rails/HelperInstanceVariable | ||
def next_stream_path | ||
if current_page?(:stream) | ||
|
@@ -50,7 +51,7 @@ def next_stream_path | |
|
||
def time_for_scroll(stream) | ||
if stream.stream_posts.empty? | ||
(Time.now() + 1).to_i | ||
(Time.zone.now + 1).to_i | ||
else | ||
stream.stream_posts.last.send(stream.order.to_sym).to_i | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,18 +45,40 @@ def self.guids_for_author(person) | |
Post.connection.select_values(Post.where(:author_id => person.id).select('posts.guid').to_sql) | ||
end | ||
|
||
def self.any_user_tag_stream(user, tag_ids) | ||
owned_or_visible_by_user(user).any_tag_stream(tag_ids) | ||
end | ||
|
||
def self.user_tag_stream(user, tag_ids) | ||
owned_or_visible_by_user(user).tag_stream(tag_ids) | ||
owned_or_visible_by_user(user).all_tag_stream(tag_ids) | ||
end | ||
|
||
def self.public_all_tag_stream(tag_ids) | ||
all_public.select("DISTINCT #{table_name}.*").all_tag_stream(tag_ids) | ||
end | ||
|
||
def self.public_tag_stream(tag_ids) | ||
all_public.select("DISTINCT #{table_name}.*").tag_stream(tag_ids) | ||
def self.public_any_tag_stream(tag_ids) | ||
all_public.select("DISTINCT #{table_name}.*").any_tag_stream(tag_ids) | ||
end | ||
|
||
def self.tag_stream(tag_ids) | ||
def self.any_tag_stream(tag_ids) | ||
joins(:taggings).where("taggings.tag_id IN (?)", tag_ids) | ||
end | ||
|
||
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 commentThe 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 |
||
end | ||
|
||
joins("INNER JOIN ( | ||
SELECT taggable_id FROM taggings | ||
WHERE taggings.tag_id IN (#{tag_ids.join(',')}) AND taggings.taggable_type = 'Post' | ||
GROUP BY taggable_id | ||
HAVING COUNT(*) >= #{tag_ids.length}) | ||
taggable ON taggable.taggable_id = posts.id") | ||
end | ||
|
||
def nsfw | ||
!!(text.try(:match, /#nsfw/i) || super) # rubocop:disable Style/DoubleNegation | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,13 +5,14 @@ | |
%h1 | ||
= @stream.display_tag_name | ||
- if user_signed_in? | ||
- unless tag_followed? | ||
.btn.btn-success.tag_following_action{data: {name: @stream.tag_name}} | ||
= t(".follow", tag: @stream.tag_name) | ||
-# rubocop:disable Style/IdenticalConditionalBranches | ||
- if tag_followed? | ||
.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) | ||
Comment on lines
+10
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
-# rubocop:enable Style/IdenticalConditionalBranches | ||
#main-stream.stream | ||
= render 'shared/stream', :posts => @stream.stream_posts | ||
= render 'shared/stream_more_button' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,12 @@ def title | |
|
||
# @return [ActiveRecord::Association<Post>] AR association of posts | ||
def posts | ||
@posts ||= StatusMessage.user_tag_stream(user, tag_ids) | ||
if tag_ids.empty? | ||
@posts = StatusMessage.none | ||
else | ||
@posts ||= StatusMessage.any_user_tag_stream(user, tag_ids) | ||
end | ||
Comment on lines
+19
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather move this condition under |
||
@posts | ||
end | ||
|
||
private | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,46 +5,43 @@ | |
# the COPYRIGHT file. | ||
|
||
class Stream::Tag < Stream::Base | ||
attr_accessor :tag_name, :people_page , :people_per_page | ||
attr_accessor :tag_names, :people_page, :people_per_page | ||
|
||
def initialize(user, tag_name, opts={}) | ||
self.tag_name = tag_name | ||
def initialize(user, tag_names, opts={}) | ||
self.tag_names = tag_names.downcase.gsub("#", "") | ||
self.people_page = opts[:page] || 1 | ||
self.people_per_page = 15 | ||
super(user, opts) | ||
end | ||
|
||
def tag | ||
@tag ||= ActsAsTaggableOn::Tag.named(tag_name).first | ||
def tags | ||
@tags ||= ActsAsTaggableOn::Tag.named_any(tag_names.split(" ")) | ||
end | ||
|
||
def display_tag_name | ||
@display_tag_name ||= "##{tag_name}" | ||
@display_tag_name ||= tag_names.split(" ").map {|t| "##{t}" }.join(" ") | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. |
||
end | ||
|
||
def tagged_people_count | ||
@people_count ||= ::Person.profile_tagged_with(tag_name).count | ||
@tagged_people_count ||= ::Person.profile_tagged_with(tag_names).count | ||
end | ||
|
||
def posts | ||
@posts ||= if user | ||
StatusMessage.user_tag_stream(user, tag.id) | ||
StatusMessage.user_tag_stream(user, tags.pluck(:id)) | ||
else | ||
StatusMessage.public_tag_stream(tag.id) | ||
StatusMessage.public_all_tag_stream(tags.pluck(:id)) | ||
end | ||
end | ||
|
||
def stream_posts | ||
return [] unless tag | ||
super | ||
end | ||
return [] unless tags | ||
|
||
def tag_name=(tag_name) | ||
@tag_name = tag_name.downcase.gsub('#', '') | ||
super | ||
end | ||
|
||
private | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,22 +41,22 @@ | |
|
||
describe ".tag_steam" do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 "returns status messages tagged with the tag" do | ||
tag_stream = StatusMessage.send(:tag_stream, [@tag_id]) | ||
tag_stream = StatusMessage.send(:all_tag_stream, [@tag_id]) | ||
expect(tag_stream).to include @status_message1 | ||
expect(tag_stream).to include @status_message2 | ||
end | ||
end | ||
|
||
describe ".public_tag_stream" do | ||
describe ".public_any_tag_stream" do | ||
it "returns public status messages tagged with the tag" do | ||
expect(StatusMessage.public_tag_stream([@tag_id])).to eq([@status_message1]) | ||
expect(StatusMessage.public_any_tag_stream([@tag_id])).to eq([@status_message1]) | ||
end | ||
|
||
it "returns a post with two tags only once" do | ||
status_message = FactoryBot.create(:status_message, text: "#hashtag #test", public: true) | ||
test_tag_id = ActsAsTaggableOn::Tag.where(name: "test").first.id | ||
|
||
expect(StatusMessage.public_tag_stream([@tag_id, test_tag_id])) | ||
expect(StatusMessage.public_any_tag_stream([@tag_id, test_tag_id])) | ||
.to match_array([@status_message1, status_message]) | ||
end | ||
end | ||
|
@@ -65,9 +65,9 @@ | |
it "returns tag stream thats owned or visible by" do | ||
relation = double | ||
expect(StatusMessage).to receive(:owned_or_visible_by_user).with(bob).and_return(relation) | ||
expect(relation).to receive(:tag_stream).with([@tag_id]) | ||
expect(relation).to receive(:any_tag_stream).with([@tag_id]) | ||
|
||
StatusMessage.user_tag_stream(bob, [@tag_id]) | ||
StatusMessage.any_user_tag_stream(bob, [@tag_id]) | ||
end | ||
end | ||
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.
🆗 this file doesn't seem to containt anything other than code style changes