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.
This commit is contained in:
parent
f47e86feaa
commit
172932eec8
|
@ -0,0 +1,5 @@
|
||||||
|
---
|
||||||
|
title: Fix /unsubscribe slash command creating extra todos when you were already mentioned
|
||||||
|
in an issue
|
||||||
|
merge_request:
|
||||||
|
author:
|
|
@ -163,14 +163,15 @@ module Banzai
|
||||||
# been queried the object is returned from the cache.
|
# been queried the object is returned from the cache.
|
||||||
def collection_objects_for_ids(collection, ids)
|
def collection_objects_for_ids(collection, ids)
|
||||||
if RequestStore.active?
|
if RequestStore.active?
|
||||||
|
ids = ids.map(&:to_i)
|
||||||
cache = collection_cache[collection_cache_key(collection)]
|
cache = collection_cache[collection_cache_key(collection)]
|
||||||
to_query = ids.map(&:to_i) - cache.keys
|
to_query = ids - cache.keys
|
||||||
|
|
||||||
unless to_query.empty?
|
unless to_query.empty?
|
||||||
collection.where(id: to_query).each { |row| cache[row.id] = row }
|
collection.where(id: to_query).each { |row| cache[row.id] = row }
|
||||||
end
|
end
|
||||||
|
|
||||||
cache.values
|
cache.values_at(*ids)
|
||||||
else
|
else
|
||||||
collection.where(id: ids)
|
collection.where(id: ids)
|
||||||
end
|
end
|
||||||
|
|
|
@ -42,6 +42,29 @@ describe Banzai::ReferenceParser::UserParser, lib: true do
|
||||||
|
|
||||||
expect(subject.referenced_by([link])).to eq([user])
|
expect(subject.referenced_by([link])).to eq([user])
|
||||||
end
|
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
|
end
|
||||||
|
|
||||||
context 'when the link has a data-project attribute' do
|
context 'when the link has a data-project attribute' do
|
||||||
|
@ -74,7 +97,7 @@ describe Banzai::ReferenceParser::UserParser, lib: true do
|
||||||
end
|
end
|
||||||
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 'when the link has a data-group attribute' do
|
||||||
context 'using an existing group ID' do
|
context 'using an existing group ID' do
|
||||||
before do
|
before do
|
||||||
|
|
Loading…
Reference in New Issue