Merge branch 'usage-data-design-management' into 'master'

Usage data design management

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

ee/app/services/design_management/save_designs_service.rb

@@ -9,6 +9,7 @@ module DesignManagement
 
       @issue = params.fetch(:issue)
       @files = params.fetch(:files)
+      @success_callbacks = []
     end
 
     def execute
@@ -18,7 +19,7 @@ module DesignManagement
       save_designs!
 
       success({ designs: updated_designs })
-    rescue Gitlab::Git::BaseError, ActiveRecord::RecordInvalid => e
+    rescue ::Gitlab::Git::BaseError, ::ActiveRecord::RecordInvalid => e
       error(e.message)
     end
 
@@ -26,9 +27,21 @@ module DesignManagement
 
     attr_reader :files, :issue
 
+    def success(*_)
+      while cb = @success_callbacks.pop
+        cb.call
+      end
+
+      super
+    end
+
+    def on_success(&block)
+      @success_callbacks.push(block)
+    end
+
     def save_designs!
       commit_sha = create_and_commit_designs!
-      DesignManagement::Version.create_for_designs(updated_designs, commit_sha)
+      ::DesignManagement::Version.create_for_designs(updated_designs, commit_sha)
     end
 
     def create_and_commit_designs!
@@ -53,8 +66,11 @@ module DesignManagement
     end
 
     def build_repository_action(file, design)
+      action = new_file?(design) ? :create : :update
+      on_success { ::EE::Gitlab::UsageCounters::DesignsCounter.count(action) }
+
       {
-        action: new_file?(design) ? :create : :update,
+        action: action,
         file_path: design.full_path,
         content: file_content(file, design.full_path)
       }
@@ -105,9 +121,9 @@ module DesignManagement
     end
 
     def file_content(file, full_path)
-      return file.to_io if Feature.disabled?(:store_designs_in_lfs, default_enabled: true)
+      return file.to_io if ::Feature.disabled?(:store_designs_in_lfs, default_enabled: true)
 
-      transformer = Lfs::FileTransformer.new(project, repository, target_branch)
+      transformer = ::Lfs::FileTransformer.new(project, repository, target_branch)
       transformer.new_file(full_path, file.to_io).content
     end
 

ee/changelogs/unreleased/usage-data-design-management.yml

+---
+title: Count design usage, in order to meet SMAU OKR
+merge_request: 14779
+type: added

ee/lib/ee/gitlab/usage_data.rb

@@ -8,6 +8,11 @@ module EE
       class_methods do
         extend ::Gitlab::Utils::Override
 
+        override :usage_data_counters
+        def usage_data_counters
+          super + [::EE::Gitlab::UsageCounters::DesignsCounter]
+        end
+
         override :features_usage_data
         def features_usage_data
           super.merge(features_usage_data_ee)

ee/lib/gitlab/usage_counters/designs_counter.rb

+# frozen_string_literal: true
+
+module EE::Gitlab::UsageCounters
+  class DesignsCounter
+    extend ::Gitlab::UsageDataCounters::RedisCounter
+
+    KNOWN_EVENTS = %w[create update delete].map(&:freeze).freeze
+
+    UnknownEvent = Class.new(StandardError)
+
+    class << self
+      # Each event gets a unique Redis key
+      def redis_key(event)
+        raise UnknownEvent, event unless KNOWN_EVENTS.include?(event.to_s)
+
+        "USAGE_DESIGN_MANAGEMENT_DESIGNS_#{event}".upcase
+      end
+
+      def count(event)
+        increment(redis_key event)
+      end
+
+      def read(event)
+        total_count(redis_key event)
+      end
+
+      def totals
+        KNOWN_EVENTS.map { |e| ["design_management_designs_#{e}".to_sym, read(e)] }.to_h
+      end
+    end
+  end
+end

ee/spec/lib/ee/gitlab/usage_counters/designs_counter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EE::Gitlab::UsageCounters::DesignsCounter do
+  it_behaves_like 'a redis usage counter', 'Designs', :create
+  it_behaves_like 'a redis usage counter', 'Designs', :update
+  it_behaves_like 'a redis usage counter', 'Designs', :delete
+
+  it_behaves_like 'a redis usage counter with totals', :design_management_designs,
+    create: 5,
+    update: 3,
+    delete: 2
+end

ee/spec/lib/gitlab/usage_data_spec.rb

@@ -55,6 +55,12 @@ describe Gitlab::UsageData do
       ))
     end
 
+    it do
+      is_expected.to include(design_management_designs_create: a_kind_of(Integer),
+                             design_management_designs_update: a_kind_of(Integer),
+                             design_management_designs_delete: a_kind_of(Integer))
+    end
+
     it 'gathers usage counts' do
       expect(count_data[:boards]).to eq(1)
       expect(count_data[:projects]).to eq(3)

ee/spec/services/design_management/save_designs_service_spec.rb

@@ -5,9 +5,16 @@ describe DesignManagement::SaveDesignsService do
   let(:issue) { create(:issue, project: project) }
   let(:project) { create(:project) }
   let(:user) { project.owner }
-  let(:files) { [fixture_file_upload("spec/fixtures/rails_sample.jpg")] }
+  let(:files) { [rails_sample] }
   let(:design_repository) { EE::Gitlab::GlRepository::DESIGN.repository_accessor.call(project) }
   let(:design_collection) { DesignManagement::DesignCollection.new(issue) }
+  let(:rails_sample_name) { 'rails_sample.jpg' }
+  let(:rails_sample) { sample_image(rails_sample_name) }
+  let(:dk_png) { sample_image('dk.png') }
+
+  def sample_image(filename)
+    fixture_file_upload("spec/fixtures/#{filename}")
+  end
 
   subject(:service) { described_class.new(project, user, issue: issue, files: files) }
 
@@ -50,6 +57,11 @@ describe DesignManagement::SaveDesignsService do
           expect { service.execute }.to change { repository_exists.call }.from(false).to(true)
         end
 
+        it "updates the creation count" do
+          counter = EE::Gitlab::UsageCounters::DesignsCounter
+          expect { service.execute }.to change { counter.read(:create) }.by(1)
+        end
+
         it "creates a nice commit in the repository" do
           service.execute
 
@@ -57,7 +69,7 @@ describe DesignManagement::SaveDesignsService do
 
           expect(commit).not_to be_nil
           expect(commit.author).to eq(user)
-          expect(commit.message).to include("rails_sample.jpg")
+          expect(commit.message).to include(rails_sample_name)
         end
 
         it "creates a design & a version for the filename if it did not exist" do
@@ -104,7 +116,7 @@ describe DesignManagement::SaveDesignsService do
         context "when a design already exists" do
           before do
             # This makes sure the file is created in the repository.
-            # otherwise we"d have a database & repository that are not in sync.
+            # otherwise we'd have a database & repository that are not in sync.
             service.execute
           end
 
@@ -118,11 +130,16 @@ describe DesignManagement::SaveDesignsService do
             expect(updated_designs.first.versions.size).to eq(2)
           end
 
+          it 'increments the update counter' do
+            counter = EE::Gitlab::UsageCounters::DesignsCounter
+            expect { service.execute }.to change { counter.read(:update) }.by 1
+          end
+
           context "when uploading a new design" do
             it "does not link the new version to the existing design" do
               existing_design = issue.designs.first
 
-              updated_designs = described_class.new(project, user, issue: issue, files: [fixture_file_upload("spec/fixtures/dk.png")])
+              updated_designs = described_class.new(project, user, issue: issue, files: [dk_png])
                                   .execute[:designs]
 
               expect(existing_design.versions.reload.size).to eq(1)
@@ -132,13 +149,33 @@ describe DesignManagement::SaveDesignsService do
           end
         end
 
-        context "when uploading multiple files" do
-          let(:files) do
-            [
-              fixture_file_upload("spec/fixtures/rails_sample.jpg"),
-              fixture_file_upload("spec/fixtures/dk.png")
-            ]
+        context 'when doing a mixture of updates and creations' do
+          let(:files) { [rails_sample, dk_png] }
+
+          before do
+            # Create just the first one, which we will later update.
+            described_class.new(project, user, issue: issue, files: [files.first]).execute
+          end
+
+          it 'counts one creation and one update' do
+            counter = EE::Gitlab::UsageCounters::DesignsCounter
+            expect { service.execute }
+              .to change { counter.read(:create) }.by(1)
+              .and change { counter.read(:update) }.by(1)
+          end
+
+          it 'creates a single commit' do
+            commit_count = -> do
+              design_repository.expire_all_method_caches
+              design_repository.commit_count
+            end
+
+            expect { service.execute }.to change { commit_count.call }.by(1)
           end
+        end
+
+        context "when uploading multiple files" do
+          let(:files) { [rails_sample, dk_png] }
 
           it 'returns information about both designs in the response' do
             expect(service.execute).to include(designs: have_attributes(size: 2), status: :success)
@@ -149,6 +186,11 @@ describe DesignManagement::SaveDesignsService do
             expect(DesignManagement::Version.for_designs(issue.designs).size).to eq(1)
           end
 
+          it 'increments the creation count by 2' do
+            counter = EE::Gitlab::UsageCounters::DesignsCounter
+            expect { service.execute }.to change { counter.read(:create) }.by 2
+          end
+
           it "creates a single commit" do
             commit_count = -> do
               design_repository.expire_all_method_caches
@@ -169,7 +211,7 @@ describe DesignManagement::SaveDesignsService do
           end
 
           context "when uploading too many files" do
-            let(:files) { Array.new(DesignManagement::SaveDesignsService::MAX_FILES + 1) { fixture_file_upload("spec/fixtures/dk.png") } }
+            let(:files) { Array.new(DesignManagement::SaveDesignsService::MAX_FILES + 1) { dk_png } }
 
             it "returns the correct error" do
               expect(service.execute[:message]).to match(/only \d+ files are allowed simultaneously/i)
@@ -200,8 +242,9 @@ describe DesignManagement::SaveDesignsService do
         end
 
         context "when a design already existed in the repo but we didn't know about it in the database" do
+          let(:filename) { rails_sample_name }
           before do
-            path = File.join(build(:design, issue: issue, filename: "rails_sample.jpg").full_path)
+            path = File.join(build(:design, issue: issue, filename: filename).full_path)
             design_repository.create_if_not_exists
             design_repository.create_file(user, path, "something fake",
                                           branch_name: "master",
@@ -211,7 +254,7 @@ describe DesignManagement::SaveDesignsService do
           it "creates the design and a new version for it" do
             first_updated_design = service.execute[:designs].first
 
-            expect(first_updated_design.filename).to eq("rails_sample.jpg")
+            expect(first_updated_design.filename).to eq(filename)
             expect(first_updated_design.versions.size).to eq(1)
           end
         end

spec/lib/gitlab/usage_data_counters/wiki_page_counter_spec.rb

@@ -2,68 +2,13 @@
 
 require 'spec_helper'
 
-describe Gitlab::UsageDataCounters::WikiPageCounter, :clean_gitlab_redis_shared_state do
-  shared_examples :wiki_page_event do |event|
-    describe ".count(#{event})" do
-      it "increments the wiki page #{event} counter by 1" do
-        expect do
-          described_class.count(event)
-        end.to change { described_class.read(event) }.by 1
-      end
-    end
-
-    describe ".read(#{event})" do
-      event_count = 5
-
-      it "returns the total number of #{event} events" do
-        event_count.times do
-          described_class.count(event)
-        end
-
-        expect(described_class.read(event)).to eq(event_count)
-      end
-    end
-  end
-
-  include_examples :wiki_page_event, :create
-  include_examples :wiki_page_event, :update
-  include_examples :wiki_page_event, :delete
-
-  describe 'totals' do
-    creations = 5
-    edits = 3
-    deletions = 2
-
-    before do
-      creations.times do
-        described_class.count(:create)
-      end
-      edits.times do
-        described_class.count(:update)
-      end
-      deletions.times do
-        described_class.count(:delete)
-      end
-    end
-
-    it 'can report all totals' do
-      expect(described_class.totals).to include(
-        wiki_pages_update: edits,
-        wiki_pages_create: creations,
-        wiki_pages_delete: deletions
-      )
-    end
-  end
-
-  describe 'unknown events' do
-    error = described_class::UnknownEvent
-
-    it 'cannot increment' do
-      expect { described_class.count(:wibble) }.to raise_error error
-    end
-
-    it 'cannot read' do
-      expect { described_class.read(:wibble) }.to raise_error error
-    end
-  end
+describe Gitlab::UsageDataCounters::WikiPageCounter do
+  it_behaves_like 'a redis usage counter', 'Wiki Page', :create
+  it_behaves_like 'a redis usage counter', 'Wiki Page', :update
+  it_behaves_like 'a redis usage counter', 'Wiki Page', :delete
+
+  it_behaves_like 'a redis usage counter with totals', :wiki_pages,
+    create: 5,
+    update: 3,
+    delete: 2
 end

spec/support/shared_examples/lib/gitlab/usage_data_counters/a_redis_counter.rb

+# frozen_string_literal: true
+
+shared_examples 'a redis usage counter' do |thing, event|
+  describe ".count(#{event})", :clean_gitlab_redis_shared_state do
+    it "increments the #{thing} #{event} counter by 1" do
+      expect do
+        described_class.count(event)
+      end.to change { described_class.read(event) }.by 1
+    end
+  end
+
+  describe ".read(#{event})", :clean_gitlab_redis_shared_state do
+    event_count = 5
+
+    it "returns the total number of #{event} events" do
+      event_count.times do
+        described_class.count(event)
+      end
+
+      expect(described_class.read(event)).to eq(event_count)
+    end
+  end
+end
+
+shared_examples 'a redis usage counter with totals' do |prefix, events|
+  describe 'totals', :clean_gitlab_redis_shared_state do
+    before do
+      events.each do |k, n|
+        n.times do
+          described_class.count(k)
+        end
+      end
+    end
+
+    let(:expected_totals) do
+      events.transform_keys { |k| "#{prefix}_#{k}".to_sym }
+    end
+
+    it 'can report all totals' do
+      expect(described_class.totals).to include(expected_totals)
+    end
+  end
+
+  # Override these let-bindings to adjust the unknown events tests
+  let(:unknown_event) { described_class::UnknownEvent }
+  let(:bad_event) { :wibble }
+
+  describe 'unknown events' do
+    it 'cannot increment' do
+      expect { described_class.count(bad_event) }.to raise_error unknown_event
+    end
+
+    it 'cannot read' do
+      expect { described_class.read(bad_event) }.to raise_error unknown_event
+    end
+  end
+end