Merge branch '247489-update-create-column-from-to-also-copy-constraints-take-2' into 'master'

Add migration helpers for fetching and copying check constraints - Take 2

See merge request gitlab-org/gitlab!44777

.rubocop_todo.yml

@@ -1141,19 +1141,6 @@ Rails/SaveBang:
     - 'spec/services/notification_recipients/build_service_spec.rb'
     - 'spec/services/notification_service_spec.rb'
     - 'spec/services/packages/conan/create_package_file_service_spec.rb'
-    - 'spec/services/projects/after_rename_service_spec.rb'
-    - 'spec/services/projects/autocomplete_service_spec.rb'
-    - 'spec/services/projects/create_service_spec.rb'
-    - 'spec/services/projects/destroy_service_spec.rb'
-    - 'spec/services/projects/fork_service_spec.rb'
-    - 'spec/services/projects/hashed_storage/base_attachment_service_spec.rb'
-    - 'spec/services/projects/move_access_service_spec.rb'
-    - 'spec/services/projects/move_project_group_links_service_spec.rb'
-    - 'spec/services/projects/overwrite_project_service_spec.rb'
-    - 'spec/services/projects/propagate_service_template_spec.rb'
-    - 'spec/services/projects/unlink_fork_service_spec.rb'
-    - 'spec/services/projects/update_pages_service_spec.rb'
-    - 'spec/services/projects/update_service_spec.rb'
     - 'spec/services/reset_project_cache_service_spec.rb'
     - 'spec/services/resource_events/change_milestone_service_spec.rb'
     - 'spec/services/system_hooks_service_spec.rb'

app/assets/javascripts/design_management/pages/index.vue

@@ -3,6 +3,7 @@ import { GlLoadingIcon, GlButton, GlAlert } from '@gitlab/ui';
 import VueDraggable from 'vuedraggable';
 import { deprecatedCreateFlash as createFlash } from '~/flash';
 import { s__, sprintf } from '~/locale';
+import { getFilename } from '~/lib/utils/file_upload';
 import UploadButton from '../components/upload/button.vue';
 import DeleteButton from '../components/delete_button.vue';
 import Design from '../components/list/item.vue';
@@ -31,7 +32,7 @@ import {
   isValidDesignFile,
   moveDesignOptimisticResponse,
 } from '../utils/design_management_utils';
-import { getFilename } from '~/lib/utils/file_upload';
+import { trackDesignCreate, trackDesignUpdate } from '../utils/tracking';
 import { DESIGNS_ROUTE_NAME } from '../router/constants';
 
 const MAXIMUM_FILE_UPLOAD_LIMIT = 10;
@@ -186,6 +187,7 @@ export default {
       updateStoreAfterUploadDesign(store, designManagementUpload, this.projectQueryBody);
     },
     onUploadDesignDone(res) {
+      // display any warnings, if necessary
       const skippedFiles = res?.data?.designManagementUpload?.skippedDesigns || [];
       const skippedWarningMessage = designUploadSkippedWarning(this.filesToBeSaved, skippedFiles);
       if (skippedWarningMessage) {
@@ -196,7 +198,19 @@ export default {
       if (!this.isLatestVersion) {
         this.$router.push({ name: DESIGNS_ROUTE_NAME });
       }
+
+      // reset state
       this.resetFilesToBeSaved();
+      this.trackUploadDesign(res);
+    },
+    trackUploadDesign(res) {
+      (res?.data?.designManagementUpload?.designs || []).forEach(design => {
+        if (design.event === 'CREATION') {
+          trackDesignCreate();
+        } else if (design.event === 'MODIFICATION') {
+          trackDesignUpdate();
+        }
+      });
     },
     onUploadDesignError() {
       this.resetFilesToBeSaved();

app/assets/javascripts/design_management/utils/tracking.js

 import Tracking from '~/tracking';
 
 // Tracking Constants
-const DESIGN_TRACKING_CONTEXT_SCHEMA = 'iglu:com.gitlab/design_management_context/jsonschema/1-0-0';
-const DESIGN_TRACKING_PAGE_NAME = 'projects:issues:design';
-const DESIGN_TRACKING_EVENT_NAME = 'view_design';
+const DESIGN_TRACKING_CONTEXT_SCHEMAS = {
+  VIEW_DESIGN_SCHEMA: 'iglu:com.gitlab/design_management_context/jsonschema/1-0-0',
+};
+const DESIGN_TRACKING_EVENTS = {
+  VIEW_DESIGN: 'view_design',
+  CREATE_DESIGN: 'create_design',
+  UPDATE_DESIGN: 'update_design',
+};
+
+export const DESIGN_TRACKING_PAGE_NAME = 'projects:issues:design';
 
 export function trackDesignDetailView(
   referer = '',
@@ -11,10 +18,11 @@ export function trackDesignDetailView(
   designVersion = 1,
   latestVersion = false,
 ) {
-  Tracking.event(DESIGN_TRACKING_PAGE_NAME, DESIGN_TRACKING_EVENT_NAME, {
-    label: DESIGN_TRACKING_EVENT_NAME,
+  const eventName = DESIGN_TRACKING_EVENTS.VIEW_DESIGN;
+  Tracking.event(DESIGN_TRACKING_PAGE_NAME, eventName, {
+    label: eventName,
     context: {
-      schema: DESIGN_TRACKING_CONTEXT_SCHEMA,
+      schema: DESIGN_TRACKING_CONTEXT_SCHEMAS.VIEW_DESIGN_SCHEMA,
       data: {
         'design-version-number': designVersion,
         'design-is-current-version': latestVersion,
@@ -24,3 +32,11 @@ export function trackDesignDetailView(
     },
   });
 }
+
+export function trackDesignCreate() {
+  return Tracking.event(DESIGN_TRACKING_PAGE_NAME, DESIGN_TRACKING_EVENTS.CREATE_DESIGN);
+}
+
+export function trackDesignUpdate() {
+  return Tracking.event(DESIGN_TRACKING_PAGE_NAME, DESIGN_TRACKING_EVENTS.UPDATE_DESIGN);
+}

changelogs/unreleased/design-tracking-create-update.yml

+---
+title: Add product analytics for design created and modified events
+merge_request: 44129
+author:
+type: added

changelogs/unreleased/mb_rails_save_bang_fix4.yml

+---
+title: Fix Rails/SaveBang offenses in spec/services/projects/*
+merge_request: 44980
+author: matthewbried
+type: other

jest.config.base.js

@@ -13,6 +13,7 @@ module.exports = path => {
       'jest-junit',
       {
         outputName: './junit_jest.xml',
+        addFileAttribute: 'true',
       },
     ]);
   }

spec/frontend/design_management/mock_data/apollo_mock.js

@@ -51,6 +51,34 @@ export const designListQueryResponse = {
   },
 };
 
+export const designUploadMutationCreatedResponse = {
+  data: {
+    designManagementUpload: {
+      designs: [
+        {
+          id: '1',
+          event: 'CREATION',
+          filename: 'fox_1.jpg',
+        },
+      ],
+    },
+  },
+};
+
+export const designUploadMutationUpdatedResponse = {
+  data: {
+    designManagementUpload: {
+      designs: [
+        {
+          id: '1',
+          event: 'MODIFICATION',
+          filename: 'fox_1.jpg',
+        },
+      ],
+    },
+  },
+};
+
 export const permissionsQueryResponse = {
   data: {
     project: {

spec/frontend/design_management/pages/index_spec.js

@@ -4,6 +4,7 @@ import VueDraggable from 'vuedraggable';
 import VueRouter from 'vue-router';
 import { GlEmptyState } from '@gitlab/ui';
 import createMockApollo from 'jest/helpers/mock_apollo_helper';
+import { mockTracking, unmockTracking } from 'helpers/tracking_helper';
 import Index from '~/design_management/pages/index.vue';
 import uploadDesignQuery from '~/design_management/graphql/mutations/upload_design.mutation.graphql';
 import DesignDestroyer from '~/design_management/components/design_destroyer.vue';
@@ -21,6 +22,8 @@ import * as utils from '~/design_management/utils/design_management_utils';
 import { DESIGN_DETAIL_LAYOUT_CLASSLIST } from '~/design_management/constants';
 import {
   designListQueryResponse,
+  designUploadMutationCreatedResponse,
+  designUploadMutationUpdatedResponse,
   permissionsQueryResponse,
   moveDesignMutationResponse,
   reorderedDesigns,
@@ -29,6 +32,7 @@ import {
 import getDesignListQuery from '~/design_management/graphql/queries/get_design_list.query.graphql';
 import permissionsQuery from '~/design_management/graphql/queries/design_permissions.query.graphql';
 import moveDesignMutation from '~/design_management/graphql/mutations/move_design.mutation.graphql';
+import { DESIGN_TRACKING_PAGE_NAME } from '~/design_management/utils/tracking';
 
 jest.mock('~/flash.js');
 const mockPageEl = {
@@ -370,7 +374,7 @@ describe('Design management index page', () => {
       createComponent({ stubs: { GlEmptyState } });
       wrapper.setData({ filesToBeSaved: [{ name: 'test' }] });
 
-      wrapper.vm.onUploadDesignDone();
+      wrapper.vm.onUploadDesignDone(designUploadMutationCreatedResponse);
       return wrapper.vm.$nextTick().then(() => {
         expect(wrapper.vm.filesToBeSaved).toEqual([]);
         expect(wrapper.vm.isSaving).toBeFalsy();
@@ -482,6 +486,34 @@ describe('Design management index page', () => {
         expect(createFlash).toHaveBeenCalledWith(message);
       });
     });
+
+    describe('tracking', () => {
+      let trackingSpy;
+
+      beforeEach(() => {
+        trackingSpy = mockTracking('_category_', undefined, jest.spyOn);
+
+        createComponent({ stubs: { GlEmptyState } });
+      });
+
+      afterEach(() => {
+        unmockTracking();
+      });
+
+      it('tracks design creation', () => {
+        wrapper.vm.onUploadDesignDone(designUploadMutationCreatedResponse);
+
+        expect(trackingSpy).toHaveBeenCalledTimes(1);
+        expect(trackingSpy).toHaveBeenCalledWith(DESIGN_TRACKING_PAGE_NAME, 'create_design');
+      });
+
+      it('tracks design modification', () => {
+        wrapper.vm.onUploadDesignDone(designUploadMutationUpdatedResponse);
+
+        expect(trackingSpy).toHaveBeenCalledTimes(1);
+        expect(trackingSpy).toHaveBeenCalledWith(DESIGN_TRACKING_PAGE_NAME, 'update_design');
+      });
+    });
   });
 
   describe('on latest version when has designs', () => {

spec/services/projects/after_rename_service_spec.rb

@@ -243,7 +243,7 @@ RSpec.describe Projects::AfterRenameService do
   def service_execute
     # AfterRenameService is called by UpdateService after a successful model.update
     # the initialization will include before and after paths values
-    project.update(path: path_after_rename)
+    project.update!(path: path_after_rename)
 
     described_class.new(project, path_before: path_before_rename, full_path_before: full_path_before_rename).execute
   end

spec/services/projects/autocomplete_service_spec.rb

@@ -123,7 +123,7 @@ RSpec.describe Projects::AutocompleteService do
       let!(:subgroup_milestone) { create(:milestone, group: subgroup) }
 
       before do
-        project.update(namespace: subgroup)
+        project.update!(namespace: subgroup)
       end
 
       it 'includes project milestones and all acestors milestones' do

spec/services/projects/create_service_spec.rb

@@ -15,7 +15,7 @@ RSpec.describe Projects::CreateService, '#execute' do
   end
 
   it 'creates labels on Project creation if there are templates' do
-    Label.create(title: "bug", template: true)
+    Label.create!(title: "bug", template: true)
     project = create_project(user, opts)
 
     created_label = project.reload.labels.last

spec/services/projects/destroy_service_spec.rb

@@ -72,7 +72,7 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do
     context 'when project has remote mirrors' do
       let!(:project) do
         create(:project, :repository, namespace: user.namespace).tap do |project|
-          project.remote_mirrors.create(url: 'http://test.com')
+          project.remote_mirrors.create!(url: 'http://test.com')
         end
       end
 

spec/services/projects/fork_service_spec.rb

@@ -179,7 +179,7 @@ RSpec.describe Projects::ForkService do
 
         context "when origin has git depth specified" do
           before do
-            @from_project.update(ci_default_git_depth: 42)
+            @from_project.update!(ci_default_git_depth: 42)
           end
 
           it "inherits default_git_depth from the origin project" do
@@ -201,7 +201,7 @@ RSpec.describe Projects::ForkService do
       context "when project has restricted visibility level" do
         context "and only one visibility level is restricted" do
           before do
-            @from_project.update(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+            @from_project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
             stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::INTERNAL])
           end
 

spec/services/projects/hashed_storage/base_attachment_service_spec.rb

@@ -45,7 +45,7 @@ RSpec.describe Projects::HashedStorage::BaseAttachmentService do
   describe '#move_folder!' do
     context 'when old_path is not a directory' do
       it 'adds information to the logger and returns true' do
-        Tempfile.create do |old_path|
+        Tempfile.create do |old_path| # rubocop:disable Rails/SaveBang
           new_path = "#{old_path}-new"
 
           expect(subject.send(:move_folder!, old_path, new_path)).to be_truthy

spec/services/projects/move_access_service_spec.rb

@@ -17,9 +17,9 @@ RSpec.describe Projects::MoveAccessService do
     project_with_access.add_maintainer(maintainer_user)
     project_with_access.add_developer(developer_user)
     project_with_access.add_reporter(reporter_user)
-    project_with_access.project_group_links.create(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER)
-    project_with_access.project_group_links.create(group: developer_group, group_access: Gitlab::Access::DEVELOPER)
-    project_with_access.project_group_links.create(group: reporter_group, group_access: Gitlab::Access::REPORTER)
+    project_with_access.project_group_links.create!(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER)
+    project_with_access.project_group_links.create!(group: developer_group, group_access: Gitlab::Access::DEVELOPER)
+    project_with_access.project_group_links.create!(group: reporter_group, group_access: Gitlab::Access::REPORTER)
   end
 
   subject { described_class.new(target_project, user) }
@@ -97,7 +97,7 @@ RSpec.describe Projects::MoveAccessService do
       end
 
       it 'does not remove remaining group links' do
-        target_project.project_group_links.create(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER)
+        target_project.project_group_links.create!(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER)
 
         subject.execute(project_with_access, options)
 

spec/services/projects/move_project_group_links_service_spec.rb

@@ -14,9 +14,9 @@ RSpec.describe Projects::MoveProjectGroupLinksService do
 
   describe '#execute' do
     before do
-      project_with_groups.project_group_links.create(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER)
-      project_with_groups.project_group_links.create(group: developer_group, group_access: Gitlab::Access::DEVELOPER)
-      project_with_groups.project_group_links.create(group: reporter_group, group_access: Gitlab::Access::REPORTER)
+      project_with_groups.project_group_links.create!(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER)
+      project_with_groups.project_group_links.create!(group: developer_group, group_access: Gitlab::Access::DEVELOPER)
+      project_with_groups.project_group_links.create!(group: reporter_group, group_access: Gitlab::Access::REPORTER)
     end
 
     it 'moves the group links from one project to another' do
@@ -30,8 +30,8 @@ RSpec.describe Projects::MoveProjectGroupLinksService do
     end
 
     it 'does not move existent group links in the current project' do
-      target_project.project_group_links.create(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER)
-      target_project.project_group_links.create(group: developer_group, group_access: Gitlab::Access::DEVELOPER)
+      target_project.project_group_links.create!(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER)
+      target_project.project_group_links.create!(group: developer_group, group_access: Gitlab::Access::DEVELOPER)
 
       expect(project_with_groups.project_group_links.count).to eq 3
       expect(target_project.project_group_links.count).to eq 2
@@ -55,8 +55,8 @@ RSpec.describe Projects::MoveProjectGroupLinksService do
       let(:options) { { remove_remaining_elements: false } }
 
       it 'does not remove remaining project group links' do
-        target_project.project_group_links.create(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER)
-        target_project.project_group_links.create(group: developer_group, group_access: Gitlab::Access::DEVELOPER)
+        target_project.project_group_links.create!(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER)
+        target_project.project_group_links.create!(group: developer_group, group_access: Gitlab::Access::DEVELOPER)
 
         subject.execute(project_with_groups, options)
 

spec/services/projects/overwrite_project_service_spec.rb

@@ -111,9 +111,9 @@ RSpec.describe Projects::OverwriteProjectService do
           create_list(:deploy_keys_project, 2, project: project_from)
           create_list(:notification_setting, 2, source: project_from)
           create_list(:users_star_project, 2, project: project_from)
-          project_from.project_group_links.create(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER)
-          project_from.project_group_links.create(group: developer_group, group_access: Gitlab::Access::DEVELOPER)
-          project_from.project_group_links.create(group: reporter_group, group_access: Gitlab::Access::REPORTER)
+          project_from.project_group_links.create!(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER)
+          project_from.project_group_links.create!(group: developer_group, group_access: Gitlab::Access::DEVELOPER)
+          project_from.project_group_links.create!(group: reporter_group, group_access: Gitlab::Access::REPORTER)
           project_from.add_maintainer(maintainer_user)
           project_from.add_developer(developer_user)
           project_from.add_reporter(reporter_user)

spec/services/projects/unlink_fork_service_spec.rb

@@ -61,7 +61,7 @@ RSpec.describe Projects::UnlinkForkService, :use_clean_rails_memory_store_cachin
   context 'when the original project was deleted' do
     it 'does not fail when the original project is deleted' do
       source = forked_project.forked_from_project
-      source.destroy
+      source.destroy!
       forked_project.reload
 
       expect { subject.execute }.not_to raise_error

spec/services/projects/update_pages_service_spec.rb

@@ -95,14 +95,14 @@ RSpec.describe Projects::UpdatePagesService do
         expect(project.pages_deployed?).to be_truthy
         expect(Dir.exist?(File.join(project.pages_path))).to be_truthy
 
-        project.destroy
+        project.destroy!
 
         expect(Dir.exist?(File.join(project.pages_path))).to be_falsey
         expect(ProjectPagesMetadatum.find_by_project_id(project)).to be_nil
       end
 
       it 'fails if sha on branch is not latest' do
-        build.update(ref: 'feature')
+        build.update!(ref: 'feature')
 
         expect(execute).not_to eq(:success)
         expect(project.pages_metadatum).not_to be_deployed
@@ -191,7 +191,7 @@ RSpec.describe Projects::UpdatePagesService do
   it 'fails to remove project pages when no pages is deployed' do
     expect(PagesWorker).not_to receive(:perform_in)
     expect(project.pages_deployed?).to be_falsey
-    project.destroy
+    project.destroy!
   end
 
   it 'fails if no artifacts' do

spec/services/projects/update_service_spec.rb

@@ -141,7 +141,7 @@ RSpec.describe Projects::UpdateService do
         let(:group) { create(:group, visibility_level: Gitlab::VisibilityLevel::INTERNAL) }
 
         before do
-          project.update(namespace: group, visibility_level: group.visibility_level)
+          project.update!(namespace: group, visibility_level: group.visibility_level)
         end
 
         it 'does not update project visibility level' do
@@ -256,7 +256,7 @@ RSpec.describe Projects::UpdateService do
       end
 
       it 'handles empty project feature attributes' do
-        project.project_feature.update(wiki_access_level: ProjectFeature::DISABLED)
+        project.project_feature.update!(wiki_access_level: ProjectFeature::DISABLED)
 
         result = update_project(project, user, { name: 'test1' })
 
@@ -267,7 +267,7 @@ RSpec.describe Projects::UpdateService do
 
     context 'when enabling a wiki' do
       it 'creates a wiki' do
-        project.project_feature.update(wiki_access_level: ProjectFeature::DISABLED)
+        project.project_feature.update!(wiki_access_level: ProjectFeature::DISABLED)
         TestEnv.rm_storage_dir(project.repository_storage, project.wiki.path)
 
         result = update_project(project, user, project_feature_attributes: { wiki_access_level: ProjectFeature::ENABLED })
@@ -278,7 +278,7 @@ RSpec.describe Projects::UpdateService do
       end
 
       it 'logs an error and creates a metric when wiki can not be created' do
-        project.project_feature.update(wiki_access_level: ProjectFeature::DISABLED)
+        project.project_feature.update!(wiki_access_level: ProjectFeature::DISABLED)
 
         expect_any_instance_of(ProjectWiki).to receive(:wiki).and_raise(Wiki::CouldNotCreateWikiError)
         expect_any_instance_of(described_class).to receive(:log_error).with("Could not create wiki for #{project.full_name}")