From 172932eec8f234fbb665489c00f73d644fa0413e Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Tue, 30 May 2017 11:24:55 +0100
Subject: Fix /unsubscribe slash command creating extra todos

The /unsubscribe slash command means that we check if the current user is
subscribed to the issuable without having an explicit subscription. That means
that we use the UserParser to find references to them in the notes.

The UserParser (and all parsers inheriting from BaseParser) use RequestStore to
cache ActiveRecord objects, so that we don't need to load the User object each
time, if we're parsing references a bunch of times in the same request.

However, it was always returning _all_ of the previously cached items, not just
the ones matching the IDs passed. This would mean that we did two runs through
with UserParser if you were mentioned in a comment, and then mentioned someone
else in your comment while using /unsubscribe:

1. Because /unsubscribe was used, we see if you were mentioned in any comments.
2. Because you mentioned someone, we find them - but we would also get back your
   user, even if you didn't mention yourself. This would have the effect of
   creating a mention or directly addressed todo for yourself incorrectly.

The fix is simple: only return values from the cache matching the IDs passed.
---
 ...ressed-yourself-todo-when-using-unsubscribe.yml |  5 +++++
 lib/banzai/reference_parser/base_parser.rb         |  5 +++--
 .../banzai/reference_parser/user_parser_spec.rb    | 25 +++++++++++++++++++++-
 3 files changed, 32 insertions(+), 3 deletions(-)
 create mode 100644 changelogs/unreleased/33032-invalid-you-directly-addressed-yourself-todo-when-using-unsubscribe.yml

diff --git a/changelogs/unreleased/33032-invalid-you-directly-addressed-yourself-todo-when-using-unsubscribe.yml b/changelogs/unreleased/33032-invalid-you-directly-addressed-yourself-todo-when-using-unsubscribe.yml
new file mode 100644
index 00000000000..1eaa0d0124e
--- /dev/null
+++ b/changelogs/unreleased/33032-invalid-you-directly-addressed-yourself-todo-when-using-unsubscribe.yml
@@ -0,0 +1,5 @@
+---
+title: Fix /unsubscribe slash command creating extra todos when you were already mentioned
+  in an issue
+merge_request:
+author:
diff --git a/lib/banzai/reference_parser/base_parser.rb b/lib/banzai/reference_parser/base_parser.rb
index c2503fa2adc..d99a3bfa625 100644
--- a/lib/banzai/reference_parser/base_parser.rb
+++ b/lib/banzai/reference_parser/base_parser.rb
@@ -163,14 +163,15 @@ module Banzai
       # been queried the object is returned from the cache.
       def collection_objects_for_ids(collection, ids)
         if RequestStore.active?
+          ids = ids.map(&:to_i)
           cache = collection_cache[collection_cache_key(collection)]
-          to_query = ids.map(&:to_i) - cache.keys
+          to_query = ids - cache.keys
 
           unless to_query.empty?
             collection.where(id: to_query).each { |row| cache[row.id] = row }
           end
 
-          cache.values
+          cache.values_at(*ids)
         else
           collection.where(id: ids)
         end
diff --git a/spec/lib/banzai/reference_parser/user_parser_spec.rb b/spec/lib/banzai/reference_parser/user_parser_spec.rb
index 4ec998efe53..592ed0d2b98 100644
--- a/spec/lib/banzai/reference_parser/user_parser_spec.rb
+++ b/spec/lib/banzai/reference_parser/user_parser_spec.rb
@@ -42,6 +42,29 @@ describe Banzai::ReferenceParser::UserParser, lib: true do
 
         expect(subject.referenced_by([link])).to eq([user])
       end
+
+      context 'when RequestStore is active' do
+        let(:other_user) { create(:user) }
+
+        before do
+          RequestStore.begin!
+        end
+
+        after do
+          RequestStore.end!
+          RequestStore.clear!
+        end
+
+        it 'does not return users from the first call in the second' do
+          link['data-user'] = user.id.to_s
+
+          expect(subject.referenced_by([link])).to eq([user])
+
+          link['data-user'] = other_user.id.to_s
+
+          expect(subject.referenced_by([link])).to eq([other_user])
+        end
+      end
     end
 
     context 'when the link has a data-project attribute' do
@@ -74,7 +97,7 @@ describe Banzai::ReferenceParser::UserParser, lib: true do
     end
   end
 
-  describe '#nodes_visible_to_use?' do
+  describe '#nodes_visible_to_user' do
     context 'when the link has a data-group attribute' do
       context 'using an existing group ID' do
         before do
-- 
cgit v1.2.1