taggables_with only returns first match not all taggables

[drewdeponte]

@slabounty discovered an issue at, https://github.com/cyphactor/tagalong/blob/master/lib/tagalong/tagger.rb#L104 , and e-mailed me about it. Therefore, I am creating an issues to house any discussion under it.

@russCloak was also CC'd on the original e-mail so I am including him in this ticket conversation as well.

[drewdeponte]

@slabounty it definitely looks like a bug.

I think the following simulates what would happen if we simply removed the return.

irb(main):019:0> def zar
irb(main):020:1> foo = [['a', 'b'], [1, 2, 3], ['hello', 'goodbye']]
irb(main):021:1> foo.each do |t|
irb(main):022:2* t
irb(main):023:2> end
irb(main):024:1> end
=> :zar
irb(main):025:0> zar
=> [["a", "b"], [1, 2, 3], ["hello", "goodbye"]]

Now the bigger question. Is that what we want? Or are we really looking for that thing flattened or something? What should be the expected result given the name of the method?

My initial instinct would be that it would return an array of taggables (any type of taggable object) that happen to be tagged with the specified tag name. Therefore, a flattened array of unique taggable objects.

We do already have this method documented in the README.md, https://github.com/cyphactor/tagalong#list-taggables-that-have-a-tag. It seems to describe exactly what my instinct describes.

So, looks like we need to rework that method to collect them all, flatten them, and filter them based on uniqueness before returning an array of taggables that are tagged with the given tag.

[slabounty]

I think you're right. When @russCloak and I were looking at some code yesterday, that's what we were expecting to see.

[slabounty]

It might be as easy as ...
self.tagalong_tags.where(:name => name).map { |t| t.taggables }.flatten
for the method.

[drewdeponte]

Only thing I think that might be missing is the uniqueness filter.

[slabounty]

Actually ... is there a way for the tagger to have more than one item returned from ...
self.tagalong_tags.where(:name => name)? It seems like this would mean that there could be more than one tag for a tagger that has the same name. I think it might be the case that the existing code is doing the right thing in a kind of strange way. It may be what we want is more like ...
self.tagalong_tags.where(:name => name).first.taggables

[russCloak]

@slabounty is right:

class TagalongTag < ::ActiveRecord::Base
  validates_uniqueness_of :name, :scope => [ :tagger_id, :tagger_type ]
  ...
end

So, we actually raise an exception when a tagalong_tag is created for a tagger with a name that is already in use (exclusively for that tagger):

> user = User.create!
# => true
> user.tagalong_tags.create!(:name => "test")
# => true
> user.tagalong_tags.create!(:name => "test")
# => ActiveRecord::RecordInvalid:
# =>       Validation failed: Name has already been taken

So, tagger.tagalong_tags.where(:name => name) should only ever return one or zero results.

I modified the test around this method to make it more clear, but this demonstrates that it works properly (with multiple taggable types):

describe "#taggables_with" do
  it "returns a collection of the taggables tagged with the given tag" do
    user = User.create!
    contact = Contact.create!
    business = Business.create!

    contact.tagalong_tags.create!(:name => "test", :tagger => user)
    user.tag(business, "test")

    expect(user.taggables_with("test")).to eq([contact, business])
  end
end