Commit ef502d06 authored by Patrick Bajao's avatar Patrick Bajao

Include position for cache key of DiffDiscussion

In https://gitlab.com/gitlab-org/gitlab/-/issues/332967, we
introduced caching on discussions endpoint.

Upon testing it further while rolling it out, found a bug wherein
the diff note that should still be displayed won't display anymore
because position changed. The position stored in cache no longer
applies as it's outdated.

With this fix, we include the position of the first note in diff
discussion (as sha) in the cache_key. This way, when the position
changes, the cache key will be updated and will result to a cache
miss and will return updated data.
parent 1a321d99
......@@ -42,6 +42,13 @@ class DiffDiscussion < Discussion
)
end
def cache_key
[
super,
Digest::SHA1.hexdigest(position.to_json)
].join(':')
end
private
def get_params
......
......@@ -126,4 +126,13 @@ RSpec.describe DiffDiscussion do
end
end
end
describe '#cache_key' do
it 'returns the cache key with the position sha' do
notes_sha = Digest::SHA1.hexdigest("#{diff_note.id}")
position_sha = Digest::SHA1.hexdigest(diff_note.position.to_json)
expect(subject.cache_key).to eq("#{described_class::CACHE_VERSION}:#{diff_note.latest_cached_markdown_version}:#{subject.id}:#{notes_sha}:#{diff_note.updated_at}::#{position_sha}")
end
end
end
......@@ -158,6 +158,29 @@ RSpec.describe 'merge requests discussions' do
end
end
context 'when the diff note position changes' do
before do
# This replicates a position change wherein timestamps aren't updated
# which is why `Gitlab::Timeless.timeless` is utilized. This is the
# same approach being used in Discussions::UpdateDiffPositionService
# which is responsible for updating the positions of diff discussions
# when MR updates.
first_note.position = Gitlab::Diff::Position.new(
old_path: first_note.position.old_path,
new_path: first_note.position.new_path,
old_line: first_note.position.old_line,
new_line: first_note.position.new_line + 1,
diff_refs: first_note.position.diff_refs
)
Gitlab::Timeless.timeless(first_note, &:save)
end
it_behaves_like 'cache miss' do
let(:changed_notes) { [first_note, second_note] }
end
end
context 'when merge_request_discussion_cache is disabled' do
before do
stub_feature_flags(merge_request_discussion_cache: false)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment