Merge branch '213136-move-features-to-core-design-management' into 'master'

Move features to Core: "Design Management""

Closes #213136

See merge request gitlab-org/gitlab!28589

changelogs/unreleased/213136-move-features-to-core-design-management.yml

+---
+title: Remove design management as a license feature
+merge_request: 28589
+author:
+type: added

doc/user/permissions.md

@@ -57,7 +57,7 @@ The following table depicts the various user permission levels in a project.
 | View Dependency list **(ULTIMATE)**               | ✓ (*1*) | ✓          | ✓           | ✓        | ✓      |
 | View License list **(ULTIMATE)**                  | ✓ (*1*) | ✓          | ✓           | ✓        | ✓      |
 | View licenses in Dependency list **(ULTIMATE)**   | ✓ (*1*) | ✓          | ✓           | ✓        | ✓      |
-| View [Design Management](project/issues/design_management.md) pages **(PREMIUM)** | ✓       | ✓          | ✓           | ✓        | ✓      |
+| View [Design Management](project/issues/design_management.md) pages | ✓       | ✓          | ✓           | ✓        | ✓      |
 | View project code                                 | ✓ (*1*) | ✓          | ✓           | ✓        | ✓      |
 | Pull project code                                 | ✓ (*1*) | ✓          | ✓           | ✓        | ✓      |
 | View GitLab Pages protected by [access control](project/pages/introduction.md#gitlab-pages-access-control-core) | ✓       | ✓          | ✓           | ✓        | ✓      |
@@ -87,7 +87,7 @@ The following table depicts the various user permission levels in a project.
 | Create/edit/delete [Releases](project/releases/index.md)|   |            | ✓           | ✓        | ✓      |
 | Pull from [Conan repository](packages/conan_repository/index.md), [Maven repository](packages/maven_repository/index.md), or [NPM registry](packages/npm_registry/index.md) **(PREMIUM)** |         | ✓          | ✓           | ✓        | ✓      |
 | Publish to [Conan repository](packages/conan_repository/index.md), [Maven repository](packages/maven_repository/index.md), or [NPM registry](packages/npm_registry/index.md) **(PREMIUM)** |         |            | ✓           | ✓        | ✓      |
-| Upload [Design Management](project/issues/design_management.md) files **(PREMIUM)** |         |            | ✓           | ✓        | ✓      |
+| Upload [Design Management](project/issues/design_management.md) files |         |            | ✓           | ✓        | ✓      |
 | Create new branches                               |         |            | ✓           | ✓        | ✓      |
 | Push to non-protected branches                    |         |            | ✓           | ✓        | ✓      |
 | Force push to non-protected branches              |         |            | ✓           | ✓        | ✓      |

doc/user/project/issues/design_management.md

-# Design Management **(PREMIUM)**
+# Design Management
 
 > [Introduced](https://gitlab.com/groups/gitlab-org/-/epics/660) in [GitLab Premium](https://about.gitlab.com/pricing/) 12.2.
 

doc/user/project/issues/index.md

@@ -126,7 +126,7 @@ associated label or assignee will change to match that of the new column. The en
 board can also be filtered to only include issues from a certain milestone or an overarching
 label.
 
-### Design Management **(PREMIUM)**
+### Design Management
 
 With [Design Management](design_management.md), you can upload design
 assets to issues and view them all together to easily share and

doc/user/project/settings/import_export.md

@@ -75,7 +75,7 @@ The following items will be exported:
 - Project configuration, including services
 - Issues with comments, merge requests with diffs and comments, labels, milestones, snippets,
   and other project entities
-- Design Management files and data **(PREMIUM)**
+- Design Management files and data
 - LFS objects
 - Issue boards
 - Pipelines history

ee/app/models/ee/project.rb

@@ -650,8 +650,7 @@ module EE
         # a few releases until we are able to understand the impact of the
         # hashed storage requirement for existing design management projects.
         # See https://gitlab.com/gitlab-org/gitlab/issues/13428#note_238729038
-        (hashed_storage?(:repository) || ::Feature.disabled?(:design_management_require_hashed_storage, self, default_enabled: true)) &&
-        feature_available?(:design_management)
+        (hashed_storage?(:repository) || ::Feature.disabled?(:design_management_require_hashed_storage, self, default_enabled: true))
     end
 
     def design_repository

ee/app/models/license.rb

@@ -63,7 +63,6 @@ class License < ApplicationRecord
     default_project_deletion_protection
     dependency_proxy
     deploy_board
-    design_management
     disable_name_update_for_users
     email_additional_text
     epics

ee/spec/models/project_spec.rb

@@ -2048,18 +2048,19 @@ describe Project do
   describe '#design_management_enabled?' do
     let(:project) { build(:project) }
 
-    where(:license_enabled, :lfs_enabled, :hashed_storage_enabled, :hash_storage_required, :expectation) do
-      false | false | false | false | false
-      true  | false | false | false | false
-      true  | true  | false | false | true
-      true  | true  | false | true  | false
-      true  | true  | true  | false | true
-      true  | true  | true  | true  | true
+    where(:lfs_enabled, :hashed_storage_enabled, :hash_storage_required, :expectation) do
+      false | false | false | false
+      false | true  | true  | false
+      false | true  | false | false
+      false | false | true  | false
+      true  | false | false | true
+      true  | true  | true  | true
+      true  | true  | false | true
+      true  | false | true  | false
     end
 
     with_them do
       before do
-        stub_licensed_features(design_management: license_enabled)
         stub_feature_flags(design_management_require_hashed_storage: hash_storage_required)
         expect(project).to receive(:lfs_enabled?).and_return(lfs_enabled)
         allow(project).to receive(:hashed_storage?).with(:repository).and_return(hashed_storage_enabled)

ee/spec/policies/design_management/design_policy_spec.rb

@@ -99,19 +99,9 @@ describe DesignManagement::DesignPolicy do
     it { is_expected.to be_disallowed(:create_design, :destroy_design) }
   end
 
-  context "when the license does not include the feature" do
+  context "when DesignManagement is not enabled" do
     before do
-      stub_licensed_features(design_management: false)
-      allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
-    end
-
-    it_behaves_like "design abilities not available"
-  end
-
-  context "when LFS is not enabled" do
-    before do
-      stub_licensed_features(design_management: true)
-      allow(Gitlab.config.lfs).to receive(:enabled).and_return(false)
+      enable_design_management(false)
     end
 
     it_behaves_like "design abilities not available"

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

@@ -45,25 +45,6 @@ describe 'Getting designs related to an issue' do
     Gitlab::UrlBuilder.build(design, ref: ref, size: size)
   end
 
-  context 'when the feature is not available' do
-    before do
-      stub_licensed_features(design_management: false)
-      stub_feature_flags(design_managment: false)
-    end
-
-    it_behaves_like 'a working graphql query' do
-      before do
-        post_graphql(query, current_user: current_user)
-      end
-    end
-
-    it 'returns no designs' do
-      post_graphql(query, current_user: current_user)
-
-      expect(design_collection).to be_nil
-    end
-  end
-
   context 'when the feature is available' do
     before do
       enable_design_management

ee/spec/services/design_management/save_designs_service_spec.rb

@@ -2,9 +2,11 @@
 require 'spec_helper'
 
 describe DesignManagement::SaveDesignsService do
-  let(:issue) { create(:issue, project: project) }
-  let(:project) { create(:project) }
-  let(:developer) { create(:user) }
+  include DesignManagementTestHelpers
+
+  let_it_be(:developer) { create(:user) }
+  let(:project) { issue.project }
+  let(:issue) { create(:issue) }
   let(:user) { developer }
   let(:files) { [rails_sample] }
   let(:design_repository) { EE::Gitlab::GlRepository::DESIGN.repository_resolver.call(project) }
@@ -70,7 +72,7 @@ describe DesignManagement::SaveDesignsService do
   describe '#execute' do
     context 'when the feature is not available' do
       before do
-        stub_licensed_features(design_management: false)
+        enable_design_management(false)
       end
 
       it_behaves_like 'a service error'
@@ -78,279 +80,269 @@ describe DesignManagement::SaveDesignsService do
 
     context 'when the feature is available' do
       before do
-        stub_licensed_features(design_management: true)
+        enable_design_management(true)
       end
 
-      context 'when LFS is not enabled' do
-        it_behaves_like 'a service error'
-      end
+      describe 'repository existence' do
+        def repository_exists
+          # Expire the memoized value as the service creates it's own instance
+          design_repository.expire_exists_cache
+          design_repository.exists?
+        end
 
-      context 'when LFS is enabled' do
-        before do
-          allow(project).to receive(:lfs_enabled?).and_return(true)
+        it 'creates a design repository when it did not exist' do
+          expect { run_service }.to change { repository_exists }.from(false).to(true)
         end
+      end
 
-        describe 'repository existence' do
-          def repository_exists
-            # Expire the memoized value as the service creates it's own instance
-            design_repository.expire_exists_cache
-            design_repository.exists?
-          end
+      it 'updates the creation count' do
+        counter = Gitlab::UsageCounters::DesignsCounter
+        expect { run_service }.to change { counter.read(:create) }.by(1)
+      end
+
+      it 'creates a commit in the repository' do
+        run_service
+
+        expect(design_repository.commit).to have_attributes(
+          author: user,
+          message: include(rails_sample_name)
+        )
+      end
+
+      it 'causes diff_refs not to be nil' do
+        expect(response).to include(
+          designs: all(have_attributes(diff_refs: be_present))
+        )
+      end
+
+      it 'creates a design & a version for the filename if it did not exist' do
+        expect(issue.designs.size).to eq(0)
+
+        updated_designs = response[:designs]
+
+        expect(updated_designs.size).to eq(1)
+        expect(updated_designs.first.versions.size).to eq(1)
+      end
+
+      it 'saves the user as the author' do
+        updated_designs = response[:designs]
 
-          it 'creates a design repository when it did not exist' do
-            expect { run_service }.to change { repository_exists }.from(false).to(true)
+        expect(updated_designs.first.versions.first.author).to eq(user)
+      end
+
+      describe 'saving the file to LFS' do
+        before do
+          expect_next_instance_of(Lfs::FileTransformer) do |transformer|
+            expect(transformer).to receive(:lfs_file?).and_return(true)
           end
         end
 
-        it 'updates the creation count' do
-          counter = Gitlab::UsageCounters::DesignsCounter
-          expect { run_service }.to change { counter.read(:create) }.by(1)
+        it 'saves the design to LFS' do
+          expect { run_service }.to change { LfsObject.count }.by(1)
         end
 
-        it 'creates a commit in the repository' do
-          run_service
+        it 'saves the repository_type of the LfsObjectsProject as design' do
+          expect do
+            run_service
+          end.to change { project.lfs_objects_projects.count }.from(0).to(1)
 
-          expect(design_repository.commit).to have_attributes(
-            author: user,
-            message: include(rails_sample_name)
-          )
+          expect(project.lfs_objects_projects.first.repository_type).to eq('design')
         end
+      end
 
-        it 'causes diff_refs not to be nil' do
-          expect(response).to include(
-            designs: all(have_attributes(diff_refs: be_present))
-          )
+      context 'when a design is being updated' do
+        before do
+          run_service
+          touch_files
         end
 
-        it 'creates a design & a version for the filename if it did not exist' do
-          expect(issue.designs.size).to eq(0)
+        it 'creates a new version for the existing design and updates the file' do
+          expect(issue.designs.size).to eq(1)
+          expect(DesignManagement::Version.for_designs(issue.designs).size).to eq(1)
 
           updated_designs = response[:designs]
 
           expect(updated_designs.size).to eq(1)
-          expect(updated_designs.first.versions.size).to eq(1)
+          expect(updated_designs.first.versions.size).to eq(2)
         end
 
-        it 'saves the user as the author' do
-          updated_designs = response[:designs]
-
-          expect(updated_designs.first.versions.first.author).to eq(user)
+        it 'increments the update counter' do
+          counter = Gitlab::UsageCounters::DesignsCounter
+          expect { run_service }.to change { counter.read(:update) }.by 1
         end
 
-        describe 'saving the file to LFS' do
-          before do
-            expect_next_instance_of(Lfs::FileTransformer) do |transformer|
-              expect(transformer).to receive(:lfs_file?).and_return(true)
-            end
-          end
-
-          it 'saves the design to LFS' do
-            expect { run_service }.to change { LfsObject.count }.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
 
-          it 'saves the repository_type of the LfsObjectsProject as design' do
-            expect do
-              run_service
-            end.to change { project.lfs_objects_projects.count }.from(0).to(1)
+            updated_designs = run_service([dk_png])[:designs]
 
-            expect(project.lfs_objects_projects.first.repository_type).to eq('design')
+            expect(existing_design.versions.reload.size).to eq(1)
+            expect(updated_designs.size).to eq(1)
+            expect(updated_designs.first.versions.size).to eq(1)
           end
         end
 
-        context 'when a design is being updated' do
-          before do
-            run_service
-            touch_files
-          end
-
-          it 'creates a new version for the existing design and updates the file' do
-            expect(issue.designs.size).to eq(1)
-            expect(DesignManagement::Version.for_designs(issue.designs).size).to eq(1)
-
-            updated_designs = response[:designs]
+        it 'calls repository#log_geo_updated_event' do
+          expect(design_repository).to receive(:log_geo_updated_event)
 
-            expect(updated_designs.size).to eq(1)
-            expect(updated_designs.first.versions.size).to eq(2)
+          allow_next_instance_of(described_class) do |instance|
+            allow(instance).to receive(:repository).and_return(design_repository)
           end
 
-          it 'increments the update counter' do
-            counter = Gitlab::UsageCounters::DesignsCounter
-            expect { run_service }.to change { counter.read(:update) }.by 1
-          end
+          run_service
+        end
+      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
+      context 'when a design has not changed since its previous version' do
+        before do
+          run_service
+        end
 
-              updated_designs = run_service([dk_png])[:designs]
+        it 'does not create a new version' do
+          expect { run_service }.not_to change { issue.design_versions.count }
+        end
 
-              expect(existing_design.versions.reload.size).to eq(1)
-              expect(updated_designs.size).to eq(1)
-              expect(updated_designs.first.versions.size).to eq(1)
-            end
-          end
+        it 'returns the design in `skipped_designs` instead of `designs`' do
+          response = run_service
 
-          it 'calls repository#log_geo_updated_event' do
-            expect(design_repository).to receive(:log_geo_updated_event)
+          expect(response[:designs]).to be_empty
+          expect(response[:skipped_designs].size).to eq(1)
+        end
+      end
 
-            allow_next_instance_of(described_class) do |instance|
-              allow(instance).to receive(:repository).and_return(design_repository)
-            end
+      context 'when doing a mixture of updates and creations' do
+        let(:files) { [rails_sample, dk_png] }
 
-            run_service
-          end
+        before do
+          # Create just the first one, which we will later update.
+          run_service([files.first])
+          touch_files([files.first])
         end
 
-        context 'when a design has not changed since its previous version' do
-          before do
-            run_service
-          end
+        it 'counts one creation and one update' do
+          counter = Gitlab::UsageCounters::DesignsCounter
+          expect { run_service }
+            .to change { counter.read(:create) }.by(1)
+            .and change { counter.read(:update) }.by(1)
+        end
 
-          it 'does not create a new version' do
-            expect { run_service }.not_to change { issue.design_versions.count }
+        it 'creates a single commit' do
+          commit_count = -> do
+            design_repository.expire_all_method_caches
+            design_repository.commit_count
           end
 
-          it 'returns the design in `skipped_designs` instead of `designs`' do
-            response = run_service
-
-            expect(response[:designs]).to be_empty
-            expect(response[:skipped_designs].size).to eq(1)
-          end
+          expect { run_service }.to change { commit_count.call }.by(1)
         end
 
-        context 'when doing a mixture of updates and creations' do
-          let(:files) { [rails_sample, dk_png] }
+        it 'enqueues just one new version worker' do
+          expect { run_service }.to enqueue_worker
+        end
+      end
 
-          before do
-            # Create just the first one, which we will later update.
-            run_service([files.first])
-            touch_files([files.first])
-          end
+      context 'when uploading multiple files' do
+        let(:files) { [rails_sample, dk_png] }
 
-          it 'counts one creation and one update' do
-            counter = Gitlab::UsageCounters::DesignsCounter
-            expect { run_service }
-              .to change { counter.read(:create) }.by(1)
-              .and change { counter.read(:update) }.by(1)
-          end
+        it 'returns information about both designs in the response' do
+          expect(response).to include(designs: have_attributes(size: 2), status: :success)
+        end
 
-          it 'creates a single commit' do
-            commit_count = -> do
-              design_repository.expire_all_method_caches
-              design_repository.commit_count
-            end
+        it 'creates 2 designs with a single version' do
+          expect { run_service }.to change { issue.designs.count }.from(0).to(2)
 
-            expect { run_service }.to change { commit_count.call }.by(1)
-          end
+          expect(DesignManagement::Version.for_designs(issue.designs).size).to eq(1)
+        end
 
-          it 'enqueues just one new version worker' do
-            expect { run_service }.to enqueue_worker
-          end
+        it 'increments the creation count by 2' do
+          counter = Gitlab::UsageCounters::DesignsCounter
+          expect { run_service }.to change { counter.read(:create) }.by 2
         end
 
-        context 'when uploading multiple files' do
-          let(:files) { [rails_sample, dk_png] }
+        it 'enqueues a new version worker' do
+          expect { run_service }.to enqueue_worker
+        end
 
-          it 'returns information about both designs in the response' do
-            expect(response).to include(designs: have_attributes(size: 2), status: :success)
+        it 'creates a single commit' do
+          commit_count = -> do
+            design_repository.expire_all_method_caches
+            design_repository.commit_count
           end
 
-          it 'creates 2 designs with a single version' do
-            expect { run_service }.to change { issue.designs.count }.from(0).to(2)
+          expect { run_service }.to change { commit_count.call }.by(1)
+        end
 
-            expect(DesignManagement::Version.for_designs(issue.designs).size).to eq(1)
-          end
+        it 'only does 5 gitaly calls', :request_store, :sidekiq_might_not_need_inline do
+          service = described_class.new(project, user, issue: issue, files: files)
+          # Some unrelated calls that are usually cached or happen only once
+          service.__send__(:repository).create_if_not_exists
+          service.__send__(:repository).has_visible_content?
 
-          it 'increments the creation count by 2' do
-            counter = Gitlab::UsageCounters::DesignsCounter
-            expect { run_service }.to change { counter.read(:create) }.by 2
-          end
+          request_count = -> { Gitlab::GitalyClient.get_request_count }
 
-          it 'enqueues a new version worker' do
-            expect { run_service }.to enqueue_worker
-          end
+          # An exists?, a check for existing blobs, default branch, an after_commit
+          # callback on LfsObjectsProject, and the creation of commits
+          expect { service.execute }.to change(&request_count).by(5)
+        end
 
-          it 'creates a single commit' do
-            commit_count = -> do
-              design_repository.expire_all_method_caches
-              design_repository.commit_count
-            end
+        context 'when uploading too many files' do
+          let(:files) { Array.new(DesignManagement::SaveDesignsService::MAX_FILES + 1) { dk_png } }
 
-            expect { run_service }.to change { commit_count.call }.by(1)
+          it 'returns the correct error' do
+            expect(response[:message]).to match(/only \d+ files are allowed simultaneously/i)
           end
+        end
+      end
 
-          it 'only does 5 gitaly calls', :request_store, :sidekiq_might_not_need_inline do
-            service = described_class.new(project, user, issue: issue, files: files)
-            # Some unrelated calls that are usually cached or happen only once
-            service.__send__(:repository).create_if_not_exists
-            service.__send__(:repository).has_visible_content?
-
-            request_count = -> { Gitlab::GitalyClient.get_request_count }
+      context 'when the user is not allowed to upload designs' do
+        let(:user) { create(:user) }
 
-            # An exists?, a check for existing blobs, default branch, an after_commit
-            # callback on LfsObjectsProject, and the creation of commits
-            expect { service.execute }.to change(&request_count).by(5)
-          end
+        it_behaves_like 'a service error'
+      end
 
-          context 'when uploading too many files' do
-            let(:files) { Array.new(DesignManagement::SaveDesignsService::MAX_FILES + 1) { dk_png } }
+      describe 'failure modes' do
+        let(:service) { described_class.new(project, user, issue: issue, files: files) }
+        let(:response) { service.execute }
 
-            it 'returns the correct error' do
-              expect(response[:message]).to match(/only \d+ files are allowed simultaneously/i)
-            end
-          end
+        before do
+          expect(service).to receive(:run_actions).and_raise(some_error)
         end
 
-        context 'when the user is not allowed to upload designs' do
-          let(:user) { create(:user) }
+        context 'when creating the commit fails' do
+          let(:some_error) { Gitlab::Git::BaseError }
 
-          it_behaves_like 'a service error'
+          it_behaves_like 'an execution error'
         end
 
-        describe 'failure modes' do
-          let(:service) { described_class.new(project, user, issue: issue, files: files) }
-          let(:response) { service.execute }
-
-          before do
-            expect(service).to receive(:run_actions).and_raise(some_error)
-          end
-
-          context 'when creating the commit fails' do
-            let(:some_error) { Gitlab::Git::BaseError }
-
-            it_behaves_like 'an execution error'
-          end
-
-          context 'when creating the versions fails' do
-            let(:some_error) { ActiveRecord::RecordInvalid }
+        context 'when creating the versions fails' do
+          let(:some_error) { ActiveRecord::RecordInvalid }
 
-            it_behaves_like 'a service error'
-          end
+          it_behaves_like 'a service error'
         end
+      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 }
+      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: filename).full_path)
-            design_repository.create_if_not_exists
-            design_repository.create_file(user, path, 'something fake',
-                                          branch_name: 'master',
-                                          message: 'Somehow created without being tracked in db')
-          end
+        before do
+          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',
+                                        message: 'Somehow created without being tracked in db')
+        end
 
-          it 'creates the design and a new version for it' do
-            first_updated_design = response[:designs].first
+        it 'creates the design and a new version for it' do
+          first_updated_design = response[:designs].first
 
-            expect(first_updated_design.filename).to eq(filename)
-            expect(first_updated_design.versions.size).to eq(1)
-          end
+          expect(first_updated_design.filename).to eq(filename)
+          expect(first_updated_design.versions.size).to eq(1)
         end
       end
 
-      describe 'scalability' do
+      describe 'scalability', skip: 'See: https://gitlab.com/gitlab-org/gitlab/-/issues/213169' do
         before do
-          run_service([dk_png]) # ensure project, issue, etc are created
+          run_service([sample_image('banana_sample.gif')]) # ensure project, issue, etc are created
         end
 
         it 'runs the same queries for all requests, regardless of number of files' do

ee/spec/support/helpers/design_management_test_helpers.rb

@@ -2,7 +2,6 @@
 
 module DesignManagementTestHelpers
   def enable_design_management(enabled = true, ref_filter = true)
-    stub_licensed_features(design_management: enabled)
     stub_lfs_setting(enabled: enabled)
     stub_feature_flags(design_management_reference_filter_gfm_pipeline: ref_filter)
   end