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

Support to search for multiple tags #8284

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 6 additions & 5 deletions app/helpers/stream_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@
# the COPYRIGHT file.

Copy link
Member

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

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))

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))

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.

Copy link
Member Author

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?

Copy link
Member

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

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

Expand All @@ -22,6 +22,7 @@ def reshare?(post)
end

private

# rubocop:disable Rails/HelperInstanceVariable
def next_stream_path
if current_page?(:stream)
Expand Down Expand Up @@ -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
Expand Down
30 changes: 26 additions & 4 deletions app/models/status_message.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Member

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

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
Expand Down
6 changes: 3 additions & 3 deletions app/presenters/tag_stream_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ def title

def metas_attributes
{
keywords: {name: "keywords", content: tag_name},
keywords: {name: "keywords", content: tag_names},
description: {name: "description", content: description},
og_url: {property: "og:url", content: url},
og_title: {property: "og:title", content: title},
Expand All @@ -18,10 +18,10 @@ def metas_attributes
private

def description
I18n.t("streams.tags.title", tags: tag_name)
I18n.t("streams.tags.title", tags: tag_names)
end

def url
tag_url tag_name
tag_url tag_names
end
end
13 changes: 7 additions & 6 deletions app/views/tags/show.mobile.haml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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

image
image

-# rubocop:enable Style/IdenticalConditionalBranches
#main-stream.stream
= render 'shared/stream', :posts => @stream.stream_posts
= render 'shared/stream_more_button'
Expand Down
2 changes: 1 addition & 1 deletion lib/evil_query.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ def aspects_post_ids!

def followed_tags_posts!
logger.debug("[EVIL-QUERY] followed_tags_posts!")
StatusMessage.public_tag_stream(@user.followed_tag_ids).excluding_hidden_content(@user)
StatusMessage.public_any_tag_stream(@user.followed_tag_ids).excluding_hidden_content(@user)
end

def mentioned_posts
Expand Down
7 changes: 6 additions & 1 deletion lib/stream/followed_tag.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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

@posts
end

private
Expand Down
27 changes: 12 additions & 15 deletions lib/stream/tag.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

image

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
Expand Down
4 changes: 2 additions & 2 deletions spec/integration/api/streams_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,8 @@
params: {access_token: access_token_public_only_read_only}
)
expect(response.status).to eq(200)
post = response_body_data(response)
expect(post.length).to eq(1)
posts = response_body_data(response)
expect(posts.any? {|post| post["public"] == true }).to be_truthy
end

it "fails with invalid credentials" do
Expand Down
4 changes: 2 additions & 2 deletions spec/lib/stream/tag_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,12 @@
describe '#tag_name=' do
it 'downcases the tag' do
stream = Stream::Tag.new(nil, "WHAT")
expect(stream.tag_name).to eq('what')
expect(stream.tag_names).to eq("what")
end

it 'removes #es' do
stream = Stream::Tag.new(nil, "#WHAT")
expect(stream.tag_name).to eq('what')
expect(stream.tag_names).to eq("what")
end
end

Expand Down
12 changes: 6 additions & 6 deletions spec/models/status_message_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,22 +41,22 @@

describe ".tag_steam" do
Copy link
Member

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 "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
Expand All @@ -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
Expand Down