Merge branch '13353-DesignType-notes_count' into 'master'

Expose DesignType#notes_count in GraphQL

See merge request gitlab-org/gitlab-ee!15433

app/services/base_count_service.rb

@@ -35,7 +35,7 @@ class BaseCountService
   end
 
   def cache_key
-    raise NotImplementedError, 'cache_key must be implemented and return a String'
+    raise NotImplementedError, 'cache_key must be implemented and return a String, Array, or Hash'
   end
 
   # subclasses can override to add any specific options, such as

ee/app/graphql/types/design_management/design_type.rb

@@ -14,6 +14,11 @@ module Types
       field :id, GraphQL::ID_TYPE, null: false
       field :project, Types::ProjectType, null: false
       field :issue, Types::IssueType, null: false
+      field :notes_count,
+            GraphQL::INT_TYPE,
+            null: false,
+            method: :user_notes_count,
+            description: 'The total count of user-created notes for this design'
       field :filename, GraphQL::STRING_TYPE, null: false
       field :full_path, GraphQL::STRING_TYPE, null: false
       field :image, GraphQL::STRING_TYPE, null: false, extras: [:parent]

ee/app/models/concerns/ee/noteable.rb

@@ -28,5 +28,13 @@ module EE
         super
       end
     end
+
+    def after_note_created(_note)
+      # no-op
+    end
+
+    def after_note_destroyed(_note)
+      # no-op
+    end
   end
 end

ee/app/models/design_management/design.rb

@@ -100,6 +100,16 @@ module DesignManagement
       project.design_repository
     end
 
+    def user_notes_count
+      user_notes_count_service.count
+    end
+
+    def after_note_changed(note)
+      user_notes_count_service.delete_cache unless note.system?
+    end
+    alias_method :after_note_created,   :after_note_changed
+    alias_method :after_note_destroyed, :after_note_changed
+
     private
 
     def head_version
@@ -114,5 +124,11 @@ module DesignManagement
         errors.add(:filename, message)
       end
     end
+
+    def user_notes_count_service
+      strong_memoize(:user_notes_count_service) do
+        DesignManagement::DesignUserNotesCountService.new(self) # rubocop: disable CodeReuse/ServiceClass
+      end
+    end
   end
 end

ee/app/models/ee/note.rb

@@ -12,6 +12,9 @@ module EE
       belongs_to :review, inverse_of: :notes
 
       scope :searchable, -> { where(system: false).includes(:noteable) }
+
+      after_commit :notify_after_create, on: :create
+      after_commit :notify_after_destroy, on: :destroy
     end
 
     # Original method in Elastic::ApplicationSearch
@@ -56,6 +59,14 @@ module EE
       for_epic? ? noteable.group : super
     end
 
+    def notify_after_create
+      noteable&.after_note_created(self)
+    end
+
+    def notify_after_destroy
+      noteable&.after_note_destroyed(self)
+    end
+
     private
 
     def banzai_context_params

ee/app/services/design_management/design_user_notes_count_service.rb

+# frozen_string_literal: true
+
+module DesignManagement
+  # Service class for counting and caching the number of unresolved
+  # notes of a Design
+  class DesignUserNotesCountService < ::BaseCountService
+    # The version of the cache format. This should be bumped whenever the
+    # underlying logic changes. This removes the need for explicitly flushing
+    # all caches.
+    VERSION = 1
+
+    def initialize(design)
+      @design = design
+    end
+
+    def relation_for_count
+      design.notes.user
+    end
+
+    def raw?
+      # Since we're storing simple integers we don't need all of the
+      # additional Marshal data Rails includes by default.
+      true
+    end
+
+    def cache_key
+      ['designs', 'notes_count', VERSION, design.id]
+    end
+
+    private
+
+    attr_reader :design
+  end
+end

ee/changelogs/unreleased/13353-DesignType-notes_count.yml

+---
+title: Expose a count of Notes for a Design in a new notes_count property of DesignType
+  in GraphQL
+merge_request: 15433
+author:
+type: added

ee/spec/graphql/types/design_management/design_type_spec.rb

@@ -9,16 +9,17 @@ describe GitlabSchema.types['Design'] do
 
   it 'exposes the expected fields' do
     expected_fields = %i[
-      id
-      project
-      issue
+      diff_refs
+      discussions
       filename
+      full_path
+      id
       image
-      versions
-      discussions
+      issue
       notes
-      diff_refs
-      full_path
+      notes_count
+      project
+      versions
     ]
 
     is_expected.to have_graphql_fields(*expected_fields)

ee/spec/models/design_management/design_spec.rb

@@ -285,4 +285,49 @@ describe DesignManagement::Design do
       )
     end
   end
+
+  describe '#user_notes_count', :use_clean_rails_memory_store_caching do
+    set(:design) { create(:design, :with_file) }
+
+    subject { design.user_notes_count }
+
+    # Note: Cache invalidation tests are in `design_user_notes_count_service_spec.rb`
+
+    it 'returns a count of user-generated notes' do
+      create(:diff_note_on_design, noteable: design, project: design.project)
+
+      is_expected.to eq(1)
+    end
+
+    it 'does not count notes on other designs' do
+      second_design = create(:design, :with_file)
+      create(:diff_note_on_design, noteable: second_design, project: second_design.project)
+
+      is_expected.to eq(0)
+    end
+
+    it 'does not count system notes' do
+      create(:diff_note_on_design, system: true, noteable: design, project: design.project)
+
+      is_expected.to eq(0)
+    end
+  end
+
+  describe '#after_note_changed' do
+    subject { build(:design) }
+
+    it 'calls #delete_cache on DesignUserNotesCountService' do
+      expect_next_instance_of(DesignManagement::DesignUserNotesCountService) do |service|
+        expect(service).to receive(:delete_cache)
+      end
+
+      subject.after_note_changed(build(:note))
+    end
+
+    it 'does not call #delete_cache on DesignUserNotesCountService when passed a system note' do
+      expect(DesignManagement::DesignUserNotesCountService).not_to receive(:new)
+
+      subject.after_note_changed(build(:note, :system))
+    end
+  end
 end

ee/spec/models/ee/note_spec.rb

@@ -9,6 +9,38 @@ describe Note do
     it { is_expected.to belong_to(:review).inverse_of(:notes) }
   end
 
+  describe 'callbacks' do
+    describe '#notify_after_create' do
+      it 'calls #after_note_created on the noteable' do
+        note = build(:note)
+
+        expect(note).to receive(:notify_after_create).and_call_original
+        expect(note.noteable).to receive(:after_note_created).with(note)
+
+        note.save!
+      end
+    end
+
+    describe '#notify_after_destroy' do
+      it 'calls #after_note_destroyed on the noteable' do
+        note = create(:note)
+
+        expect(note).to receive(:notify_after_destroy).and_call_original
+        expect(note.noteable).to receive(:after_note_destroyed).with(note)
+
+        note.destroy
+      end
+
+      it 'does not error if noteable is nil' do
+        note = create(:note)
+
+        expect(note).to receive(:notify_after_destroy).and_call_original
+        expect(note).to receive(:noteable).at_least(:once).and_return(nil)
+        expect { note.destroy }.not_to raise_error
+      end
+    end
+  end
+
   context 'object storage with background upload' do
     before do
       stub_uploads_object_storage(AttachmentUploader, background_upload: true)

ee/spec/requests/api/graphql/project/issue/designs/designs_spec.rb

@@ -204,5 +204,46 @@ describe "Getting designs related to an issue" do
         end
       end
     end
+
+    describe 'a design with note annotations' do
+      let!(:note) { create(:diff_note_on_design, noteable: design, project: design.project) }
+
+      let(:design_query) do
+        <<~NODE
+        designs {
+          edges {
+            node {
+              notesCount
+              notes {
+                edges {
+                  node {
+                    id
+                  }
+                }
+              }
+            }
+          }
+        }
+        NODE
+      end
+
+      let(:design_response) do
+        design_collection["designs"]["edges"].first["node"]
+      end
+
+      before do
+        post_graphql(query, current_user: current_user)
+      end
+
+      it 'returns the notes for the design' do
+        expect(design_response.dig("notes", "edges")).to eq(
+          ["node" => { "id" => note.to_global_id.to_s }]
+        )
+      end
+
+      it 'returns a note_count for the design' do
+        expect(design_response["notesCount"]).to eq(1)
+      end
+    end
   end
 end

ee/spec/services/design_management/design_user_notes_count_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe DesignManagement::DesignUserNotesCountService, :use_clean_rails_memory_store_caching do
+  set(:design) { create(:design, :with_file) }
+
+  subject { described_class.new(design) }
+
+  it_behaves_like 'a counter caching service'
+
+  describe '#count' do
+    it 'returns the count of notes' do
+      create_list(:diff_note_on_design, 3, noteable: design, project: design.project)
+
+      expect(subject.count).to eq(3)
+    end
+  end
+
+  describe '#cache_key' do
+    it 'contains the `VERSION` and `design.id`' do
+      expect(subject.cache_key).to eq(['designs', 'notes_count', DesignManagement::DesignUserNotesCountService::VERSION, design.id])
+    end
+  end
+
+  describe 'cache invalidation' do
+    it 'changes when a new note is created' do
+      new_note_attrs = attributes_for(:diff_note_on_design, noteable: design)
+
+      expect do
+        Notes::CreateService.new(design.project, create(:user), new_note_attrs).execute
+      end.to change { subject.count }.by(1)
+    end
+
+    it 'changes when a note is destroyed' do
+      note = create(:diff_note_on_design, noteable: design, project: design.project)
+
+      expect do
+        Notes::DestroyService.new(note.project, note.author).execute(note)
+      end.to change { subject.count }.by(-1)
+    end
+  end
+end

spec/services/projects/forks_count_service_spec.rb

@@ -2,15 +2,17 @@
 
 require 'spec_helper'
 
-describe Projects::ForksCountService do
+describe Projects::ForksCountService, :use_clean_rails_memory_store_caching do
+  let(:project) { build(:project) }
+  subject { described_class.new(project) }
+
+  it_behaves_like 'a counter caching service'
+
   describe '#count' do
     it 'returns the number of forks' do
-      project = build(:project, id: 42)
-      service = described_class.new(project)
-
-      allow(service).to receive(:uncached_count).and_return(1)
+      allow(subject).to receive(:uncached_count).and_return(1)
 
-      expect(service.count).to eq(1)
+      expect(subject.count).to eq(1)
     end
   end
 end

spec/services/projects/open_issues_count_service_spec.rb

@@ -2,10 +2,13 @@
 
 require 'spec_helper'
 
-describe Projects::OpenIssuesCountService do
-  describe '#count' do
-    let(:project) { create(:project) }
+describe Projects::OpenIssuesCountService, :use_clean_rails_memory_store_caching do
+  let(:project) { create(:project) }
+  subject { described_class.new(project) }
+
+  it_behaves_like 'a counter caching service'
 
+  describe '#count' do
     context 'when user is nil' do
       it 'does not include confidential issues in the issue count' do
         create(:issue, :opened, project: project)
@@ -53,9 +56,7 @@ describe Projects::OpenIssuesCountService do
       end
     end
 
-    context '#refresh_cache', :use_clean_rails_memory_store_caching do
-      let(:subject) { described_class.new(project) }
-
+    context '#refresh_cache' do
       before do
         create(:issue, :opened, project: project)
         create(:issue, :opened, project: project)

spec/services/projects/open_merge_requests_count_service_spec.rb

@@ -2,16 +2,21 @@
 
 require 'spec_helper'
 
-describe Projects::OpenMergeRequestsCountService do
+describe Projects::OpenMergeRequestsCountService, :use_clean_rails_memory_store_caching do
+  set(:project) { create(:project) }
+
+  subject { described_class.new(project) }
+
+  it_behaves_like 'a counter caching service'
+
   describe '#count' do
     it 'returns the number of open merge requests' do
-      project = create(:project)
       create(:merge_request,
              :opened,
              source_project: project,
              target_project: project)
 
-      expect(described_class.new(project).count).to eq(1)
+      expect(subject.count).to eq(1)
     end
   end
 end

spec/services/users/keys_count_service_spec.rb

@@ -4,7 +4,9 @@ require 'spec_helper'
 
 describe Users::KeysCountService, :use_clean_rails_memory_store_caching do
   let(:user) { create(:user) }
-  let(:service) { described_class.new(user) }
+  subject { described_class.new(user) }
+
+  it_behaves_like 'a counter caching service'
 
   describe '#count' do
     before do
@@ -12,53 +14,19 @@ describe Users::KeysCountService, :use_clean_rails_memory_store_caching do
     end
 
     it 'returns the number of SSH keys as an Integer' do
-      expect(service.count).to eq(1)
-    end
-
-    it 'caches the number of keys in Redis', :request_store do
-      service.delete_cache
-      control_count = ActiveRecord::QueryRecorder.new { service.count }.count
-      service.delete_cache
-
-      expect { 2.times { service.count } }.not_to exceed_query_limit(control_count)
-    end
-  end
-
-  describe '#refresh_cache' do
-    it 'refreshes the Redis cache' do
-      Rails.cache.write(service.cache_key, 10)
-      service.refresh_cache
-
-      expect(Rails.cache.fetch(service.cache_key, raw: true)).to be_zero
-    end
-  end
-
-  describe '#delete_cache' do
-    it 'removes the cache' do
-      service.count
-      service.delete_cache
-
-      expect(Rails.cache.fetch(service.cache_key, raw: true)).to be_nil
+      expect(subject.count).to eq(1)
     end
   end
 
   describe '#uncached_count' do
     it 'returns the number of SSH keys' do
-      expect(service.uncached_count).to be_zero
-    end
-
-    it 'does not cache the number of keys' do
-      recorder = ActiveRecord::QueryRecorder.new do
-        2.times { service.uncached_count }
-      end
-
-      expect(recorder.count).to be > 0
+      expect(subject.uncached_count).to be_zero
     end
   end
 
   describe '#cache_key' do
     it 'returns the cache key' do
-      expect(service.cache_key).to eq("users/key-count-service/#{user.id}")
+      expect(subject.cache_key).to eq("users/key-count-service/#{user.id}")
     end
   end
 end

spec/support/shared_examples/services/count_service_shared_examples.rb

+# frozen_string_literal: true
+
+# The calling spec should use `:use_clean_rails_memory_store_caching`
+# when including this shared example. E.g.:
+#
+#   describe MyCountService, :use_clean_rails_memory_store_caching do
+#     it_behaves_like 'a counter caching service'
+#   end
+shared_examples 'a counter caching service' do
+  describe '#count' do
+    it 'caches the count', :request_store do
+      subject.delete_cache
+      control_count = ActiveRecord::QueryRecorder.new { subject.count }.count
+      subject.delete_cache
+
+      expect { 2.times { subject.count } }.not_to exceed_query_limit(control_count)
+    end
+  end
+
+  describe '#refresh_cache' do
+    it 'refreshes the cache' do
+      original_count = subject.count
+      Rails.cache.write(subject.cache_key, original_count + 1, raw: subject.raw?)
+
+      subject.refresh_cache
+
+      expect(fetch_cache || 0).to eq(original_count)
+    end
+  end
+
+  describe '#delete_cache' do
+    it 'removes the cache' do
+      subject.count
+      subject.delete_cache
+
+      expect(fetch_cache).to be_nil
+    end
+  end
+
+  describe '#uncached_count' do
+    it 'does not cache the count' do
+      subject.delete_cache
+      subject.uncached_count
+
+      expect(fetch_cache).to be_nil
+    end
+  end
+
+  private
+
+  def fetch_cache
+    Rails.cache.read(subject.cache_key, raw: subject.raw?)
+  end
+end