diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2022-07-27 12:08:34 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2022-07-27 12:08:34 +0300 |
commit | d0a683e342c9e59bec8bb83879f0fc959b79224e (patch) | |
tree | fac5bd8be0faee2016b6ad67c25206ccf42c3447 | |
parent | 0ab741639e9d9a83833661925167a98d96751135 (diff) |
Add latest changes from gitlab-org/gitlab@master
27 files changed, 269 insertions, 230 deletions
diff --git a/app/assets/javascripts/integrations/edit/components/dynamic_field.vue b/app/assets/javascripts/integrations/edit/components/dynamic_field.vue index b4ceec22822..fe687ea9767 100644 --- a/app/assets/javascripts/integrations/edit/components/dynamic_field.vue +++ b/app/assets/javascripts/integrations/edit/components/dynamic_field.vue @@ -133,9 +133,6 @@ export default { this.model = null; } }, - helpHtmlConfig: { - ADD_ATTR: ['target'], // allow external links, can be removed after https://gitlab.com/gitlab-org/gitlab-ui/-/issues/1427 is implemented - }, }; </script> @@ -147,7 +144,7 @@ export default { :state="valid" > <template v-if="!isCheckbox" #description> - <span v-safe-html:[$options.helpHtmlConfig]="help"></span> + <span v-safe-html="help"></span> </template> <template v-if="isCheckbox"> @@ -155,7 +152,7 @@ export default { <gl-form-checkbox :id="fieldId" v-model="model" :disabled="isInheriting"> {{ checkboxLabel || humanizedTitle }} <template #help> - <span v-safe-html:[$options.helpHtmlConfig]="help"></span> + <span v-safe-html="help"></span> </template> </gl-form-checkbox> </template> diff --git a/app/assets/javascripts/integrations/edit/components/integration_form.vue b/app/assets/javascripts/integrations/edit/components/integration_form.vue index f1f574c6424..7a6f1a953a8 100644 --- a/app/assets/javascripts/integrations/edit/components/integration_form.vue +++ b/app/assets/javascripts/integrations/edit/components/integration_form.vue @@ -192,11 +192,7 @@ export default { this.integrationActive = integrationActive; }, }, - descriptionHtmlConfig: { - ADD_ATTR: ['target'], // allow external links, can be removed after https://gitlab.com/gitlab-org/gitlab-ui/-/issues/1427 is implemented - }, helpHtmlConfig: { - ADD_ATTR: ['target'], // allow external links, can be removed after https://gitlab.com/gitlab-org/gitlab-ui/-/issues/1427 is implemented ADD_TAGS: ['use'], // to support icon SVGs FORBID_ATTR: [], // This is trusted input so we can override the default config to allow data-* attributes }, @@ -254,7 +250,7 @@ export default { {{ $options.billingPlanNames[section.plan] }} </gl-badge> </h4> - <p v-safe-html:[$options.descriptionHtmlConfig]="section.description"></p> + <p v-safe-html="section.description"></p> </div> <div class="col-lg-8"> diff --git a/app/assets/javascripts/whats_new/components/feature.vue b/app/assets/javascripts/whats_new/components/feature.vue index 90f6230ef72..9380a77c1fb 100644 --- a/app/assets/javascripts/whats_new/components/feature.vue +++ b/app/assets/javascripts/whats_new/components/feature.vue @@ -30,7 +30,6 @@ export default { return dateInWords(date); }, }, - safeHtmlConfig: { ADD_ATTR: ['target'] }, }; </script> @@ -76,10 +75,7 @@ export default { {{ packageName }} </gl-badge> </div> - <div - v-safe-html:[$options.safeHtmlConfig]="feature.body" - class="gl-pt-3 gl-line-height-20" - ></div> + <div v-safe-html="feature.body" class="gl-pt-3 gl-line-height-20"></div> <gl-button :href="feature.url" target="_blank" diff --git a/app/assets/javascripts/work_items/components/work_item_links/work_item_links.vue b/app/assets/javascripts/work_items/components/work_item_links/work_item_links.vue index 89f086cfca5..e65f0cfb1f2 100644 --- a/app/assets/javascripts/work_items/components/work_item_links/work_item_links.vue +++ b/app/assets/javascripts/work_items/components/work_item_links/work_item_links.vue @@ -43,6 +43,7 @@ export default { }; }, update(data) { + this.canUpdate = data.workItem.userPermissions.updateWorkItem; return ( data.workItem.widgets.find((widget) => widget.type === WIDGET_TYPE_HIERARCHY)?.children .nodes ?? [] @@ -51,6 +52,9 @@ export default { skip() { return !this.issuableId; }, + result({ data }) { + this.canUpdate = data.workItem.userPermissions.updateWorkItem; + }, }, }, data() { @@ -58,6 +62,7 @@ export default { isShownAddForm: false, isOpen: true, children: [], + canUpdate: false, }; }, computed: { @@ -117,7 +122,7 @@ export default { > <h5 class="gl-m-0 gl-line-height-32 gl-flex-grow-1">{{ $options.i18n.title }}</h5> <gl-button - v-if="!isShownAddForm" + v-if="!isShownAddForm && canUpdate" category="secondary" data-testid="toggle-add-form" @click="toggleAddForm" @@ -173,7 +178,12 @@ export default { $options.WORK_ITEM_STATUS_TEXT[child.state] }}</span> </gl-badge> - <work-item-links-menu :work-item-id="child.id" :parent-work-item-id="issuableGid" /> + <work-item-links-menu + v-if="canUpdate" + :work-item-id="child.id" + :parent-work-item-id="issuableGid" + data-testid="links-menu" + /> </div> </div> </template> diff --git a/app/assets/javascripts/work_items/graphql/work_item_links.query.graphql b/app/assets/javascripts/work_items/graphql/work_item_links.query.graphql index c2496f53cc8..921f75ccb0a 100644 --- a/app/assets/javascripts/work_items/graphql/work_item_links.query.graphql +++ b/app/assets/javascripts/work_items/graphql/work_item_links.query.graphql @@ -5,6 +5,9 @@ query workItemQuery($id: WorkItemID!) { id } title + userPermissions { + updateWorkItem + } widgets { type ... on WorkItemWidgetHierarchy { diff --git a/app/models/environment.rb b/app/models/environment.rb index b93f1158a90..855fb281d81 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -58,8 +58,7 @@ class Environment < ApplicationRecord length: { maximum: 255 }, allow_nil: true - validates :external_url, addressable_url: true, allow_nil: true, unless: :soft_validation_on_external_url_enabled? - validate :safe_external_url, if: :soft_validation_on_external_url_enabled? + validate :safe_external_url delegate :manual_actions, :other_manual_actions, to: :last_deployment, allow_nil: true delegate :auto_rollback_enabled?, to: :project @@ -491,10 +490,6 @@ class Environment < ApplicationRecord private - def soft_validation_on_external_url_enabled? - ::Feature.enabled?(:soft_validation_on_external_url, project) - end - # We deliberately avoid using AddressableUrlValidator to allow users to update their environments even if they have # misconfigured `environment:url` keyword. The external URL is presented as a clickable link on UI and not consumed # in GitLab internally, thus we sanitize the URL before the persistence to make sure the rendered link is XSS safe. diff --git a/app/services/work_items/update_service.rb b/app/services/work_items/update_service.rb index 98818fda263..2deb8c82741 100644 --- a/app/services/work_items/update_service.rb +++ b/app/services/work_items/update_service.rb @@ -26,8 +26,8 @@ module WorkItems private - def update(work_item) - execute_widgets(work_item: work_item, callback: :update, widget_params: @widget_params) + def before_update(work_item, skip_spam_check: false) + execute_widgets(work_item: work_item, callback: :before_update_callback, widget_params: @widget_params) super end diff --git a/app/services/work_items/widgets/base_service.rb b/app/services/work_items/widgets/base_service.rb index c695651361e..37ed2bf4b05 100644 --- a/app/services/work_items/widgets/base_service.rb +++ b/app/services/work_items/widgets/base_service.rb @@ -5,17 +5,18 @@ module WorkItems class BaseService < ::BaseService WidgetError = Class.new(StandardError) - attr_reader :widget, :current_user + attr_reader :widget, :work_item, :current_user def initialize(widget:, current_user:) @widget = widget + @work_item = widget.work_item @current_user = current_user end private - def can_admin_work_item? - can?(current_user, :admin_work_item, widget.work_item) + def has_permission?(permission) + can?(current_user, permission, widget.work_item) end end end diff --git a/app/services/work_items/widgets/description_service/update_service.rb b/app/services/work_items/widgets/description_service/update_service.rb index e63b6b2ee6c..fe591ba605e 100644 --- a/app/services/work_items/widgets/description_service/update_service.rb +++ b/app/services/work_items/widgets/description_service/update_service.rb @@ -4,10 +4,12 @@ module WorkItems module Widgets module DescriptionService class UpdateService < WorkItems::Widgets::BaseService - def update(params: {}) - return unless params.present? && params[:description] + def before_update_callback(params: {}) + return unless params.present? && params.key?(:description) + return unless has_permission?(:update_work_item) - widget.work_item.description = params[:description] + work_item.description = params[:description] + work_item.assign_attributes(last_edited_at: Time.current, last_edited_by: current_user) end end end diff --git a/app/services/work_items/widgets/hierarchy_service/base_service.rb b/app/services/work_items/widgets/hierarchy_service/base_service.rb index 05625cb5240..ca49597d7d6 100644 --- a/app/services/work_items/widgets/hierarchy_service/base_service.rb +++ b/app/services/work_items/widgets/hierarchy_service/base_service.rb @@ -29,13 +29,13 @@ module WorkItems def set_parent(parent) ::WorkItems::ParentLinks::CreateService - .new(parent, current_user, { target_issuable: widget.work_item }) + .new(parent, current_user, { target_issuable: work_item }) .execute end # rubocop: disable CodeReuse/ActiveRecord def remove_parent - link = ::WorkItems::ParentLink.find_by(work_item: widget.work_item) + link = ::WorkItems::ParentLink.find_by(work_item: work_item) return success unless link.present? ::WorkItems::ParentLinks::DestroyService.new(link, current_user).execute @@ -44,12 +44,12 @@ module WorkItems def update_work_item_children(children) ::WorkItems::ParentLinks::CreateService - .new(widget.work_item, current_user, { issuable_references: children }) + .new(work_item, current_user, { issuable_references: children }) .execute end def feature_flag_enabled? - Feature.enabled?(:work_items_hierarchy, widget.work_item&.project) + Feature.enabled?(:work_items_hierarchy, work_item&.project) end def incompatible_args?(params) diff --git a/app/workers/concerns/waitable_worker.rb b/app/workers/concerns/waitable_worker.rb index 9300c2a5790..336d60d46ac 100644 --- a/app/workers/concerns/waitable_worker.rb +++ b/app/workers/concerns/waitable_worker.rb @@ -7,7 +7,7 @@ module WaitableWorker # Schedules multiple jobs and waits for them to be completed. def bulk_perform_and_wait(args_list) # Short-circuit: it's more efficient to do small numbers of jobs inline - if args_list.size == 1 && !always_async_project_authorizations_refresh? + if args_list.size == 1 return bulk_perform_inline(args_list) end @@ -29,10 +29,6 @@ module WaitableWorker bulk_perform_async(failed) if failed.present? end - - def always_async_project_authorizations_refresh? - Feature.enabled?(:always_async_project_authorizations_refresh) - end end def perform(*args) diff --git a/config/feature_flags/development/always_async_project_authorizations_refresh.yml b/config/feature_flags/development/always_async_project_authorizations_refresh.yml deleted file mode 100644 index 233be4d930e..00000000000 --- a/config/feature_flags/development/always_async_project_authorizations_refresh.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: always_async_project_authorizations_refresh -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92333 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/367683 -milestone: '15.3' -type: development -group: group::workspace -default_enabled: false diff --git a/config/feature_flags/development/soft_validation_on_external_url.yml b/config/feature_flags/development/soft_validation_on_external_url.yml deleted file mode 100644 index 3df63a7bb96..00000000000 --- a/config/feature_flags/development/soft_validation_on_external_url.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: soft_validation_on_external_url -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91970 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/367206 -milestone: '15.2' -type: development -group: group::release -default_enabled: false diff --git a/doc/ci/environments/index.md b/doc/ci/environments/index.md index 9238f783004..7747f5e9b78 100644 --- a/doc/ci/environments/index.md +++ b/doc/ci/environments/index.md @@ -373,7 +373,8 @@ To retry or rollback a deployment: ### Environment URL -> [Fixed](https://gitlab.com/gitlab-org/gitlab/-/issues/337417) to persist arbitrary URLs in GitLab 15.2 [with a flag](../../administration/feature_flags.md) named `soft_validation_on_external_url`. Disabled by default. +> - [Fixed](https://gitlab.com/gitlab-org/gitlab/-/issues/337417) to persist arbitrary URLs in GitLab 15.2 [with a flag](../../administration/feature_flags.md) named `soft_validation_on_external_url`. Disabled by default. +> - [Generally available](https://gitlab.com/gitlab-org/gitlab/-/issues/337417) in GitLab 15.3. [Feature flag `soft_validation_on_external_url`](https://gitlab.com/gitlab-org/gitlab/-/issues/367206) removed. The [environment URL](../yaml/index.md#environmenturl) is displayed in a few places in GitLab: diff --git a/doc/development/documentation/site_architecture/global_nav.md b/doc/development/documentation/site_architecture/global_nav.md index e1e0da03abc..5ec4415a206 100644 --- a/doc/development/documentation/site_architecture/global_nav.md +++ b/doc/development/documentation/site_architecture/global_nav.md @@ -22,7 +22,7 @@ At the highest level, our global nav is workflow-based. Navigation needs to help The levels under each of the higher workflow-based topics are the names of features. For example: -**Use GitLab** (_workflow_) **> Build your application** (_workflow_) **> CI/CD** (_feature_) **> Pipelines** (_feature) +**Use GitLab** (_workflow_) **> Build your application** (_workflow_) **> CI/CD** (_feature_) **> Pipelines** (_feature_) ## Choose the right words for your navigation entry @@ -39,20 +39,35 @@ as helpful as **Get started with runners**. ## Add a navigation entry -All topics should be included in the left nav. - To add a topic to the global nav, edit [`navigation.yaml`](https://gitlab.com/gitlab-org/gitlab-docs/blob/main/content/_data/navigation.yaml) and add your item. -All new pages need a navigation item. Without a navigation, the page becomes "orphaned." That -is: +Without a navigation entry: + +- The navigation closes when the page is opened, and the reader loses their place. +- The page isn't visible in a group with other pages. + +### Pages you don't need to add + +Exclude these pages from the global nav: + +- Legal notices. +- Pages in the `architecture/blueprints` directory. + +The following pages should probably be in the global nav, but the technical writers +do not actively work to add them: + +- Pages in the `/development` directory. +- Pages authored by the support team, which are under the `doc/administration/troubleshooting` directory. + +Sometimes pages for deprecated features are not in the global nav, depending on how long ago the feature was deprecated. -- The navigation shuts when the page is opened, and the reader loses their place. -- The page doesn't belong in a group with other pages. +All other pages should be in the global nav. -This means the decision to create a new page is a decision to create new navigation item and vice -versa. +The technical writing team runs a report to determine which pages are not in the nav. +For now this report is manual, but [an issue exists](https://gitlab.com/gitlab-org/gitlab-docs/-/issues/1212) +to automate it. ### Where to add diff --git a/spec/frontend/integrations/edit/components/dynamic_field_spec.js b/spec/frontend/integrations/edit/components/dynamic_field_spec.js index ee2f6541b03..5af0e272285 100644 --- a/spec/frontend/integrations/edit/components/dynamic_field_spec.js +++ b/spec/frontend/integrations/edit/components/dynamic_field_spec.js @@ -204,7 +204,7 @@ describe('DynamicField', () => { }); expect(findGlFormGroup().find('small').html()).toContain( - '[<code>1</code> <a>3</a> <a target="_blank" href="foo">4</a>]', + '[<code>1</code> <a>3</a> <a href="foo">4</a>]', ); }); }); diff --git a/spec/frontend/work_items/components/work_item_links/work_item_links_spec.js b/spec/frontend/work_items/components/work_item_links/work_item_links_spec.js index 2ec9b1ec0ac..3b780bcc5bc 100644 --- a/spec/frontend/work_items/components/work_item_links/work_item_links_spec.js +++ b/spec/frontend/work_items/components/work_item_links/work_item_links_spec.js @@ -6,7 +6,11 @@ import createMockApollo from 'helpers/mock_apollo_helper'; import waitForPromises from 'helpers/wait_for_promises'; import WorkItemLinks from '~/work_items/components/work_item_links/work_item_links.vue'; import getWorkItemLinksQuery from '~/work_items/graphql/work_item_links.query.graphql'; -import { workItemHierarchyResponse, workItemHierarchyEmptyResponse } from '../../mock_data'; +import { + workItemHierarchyResponse, + workItemHierarchyEmptyResponse, + workItemHierarchyNoUpdatePermissionResponse, +} from '../../mock_data'; Vue.use(VueApollo); @@ -29,6 +33,7 @@ describe('WorkItemLinks', () => { const findEmptyState = () => wrapper.findByTestId('links-empty'); const findToggleAddFormButton = () => wrapper.findByTestId('toggle-add-form'); const findAddLinksForm = () => wrapper.findByTestId('add-links-form'); + const findFirstLinksMenu = () => wrapper.findByTestId('links-menu'); beforeEach(async () => { await createComponent(); @@ -82,5 +87,20 @@ describe('WorkItemLinks', () => { expect(children).toHaveLength(4); expect(children.at(0).findComponent(GlBadge).text()).toBe('Open'); + expect(findFirstLinksMenu().exists()).toBe(true); + }); + + describe('when no permission to update', () => { + beforeEach(async () => { + await createComponent({ response: workItemHierarchyNoUpdatePermissionResponse }); + }); + + it('does not display button to toggle Add form', () => { + expect(findToggleAddFormButton().exists()).toBe(false); + }); + + it('does not display link menu on children', () => { + expect(findFirstLinksMenu().exists()).toBe(false); + }); }); }); diff --git a/spec/frontend/work_items/mock_data.js b/spec/frontend/work_items/mock_data.js index a76407931d4..aa0f7317bd0 100644 --- a/spec/frontend/work_items/mock_data.js +++ b/spec/frontend/work_items/mock_data.js @@ -307,6 +307,9 @@ export const workItemHierarchyEmptyResponse = { __typename: 'WorkItemType', }, title: 'New title', + userPermissions: { + updateWorkItem: false, + }, widgets: [ { type: 'DESCRIPTION', @@ -327,6 +330,49 @@ export const workItemHierarchyEmptyResponse = { }, }; +export const workItemHierarchyNoUpdatePermissionResponse = { + data: { + workItem: { + id: 'gid://gitlab/WorkItem/1', + workItemType: { + id: 'gid://gitlab/WorkItems::Type/6', + __typename: 'WorkItemType', + }, + title: 'New title', + userPermissions: { + updateWorkItem: false, + }, + widgets: [ + { + type: 'DESCRIPTION', + __typename: 'WorkItemWidgetDescription', + }, + { + type: 'HIERARCHY', + parent: null, + children: { + nodes: [ + { + id: 'gid://gitlab/WorkItem/2', + workItemType: { + id: 'gid://gitlab/WorkItems::Type/5', + __typename: 'WorkItemType', + }, + title: 'xyz', + state: 'OPEN', + __typename: 'WorkItem', + }, + ], + __typename: 'WorkItemConnection', + }, + __typename: 'WorkItemWidgetHierarchy', + }, + ], + __typename: 'WorkItem', + }, + }, +}; + export const workItemHierarchyResponse = { data: { workItem: { @@ -336,6 +382,9 @@ export const workItemHierarchyResponse = { __typename: 'WorkItemType', }, title: 'New title', + userPermissions: { + updateWorkItem: true, + }, widgets: [ { type: 'DESCRIPTION', diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 8577cad1011..31a5bc737ba 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -153,6 +153,11 @@ RSpec.describe Gitlab::GitAccess, :aggregate_failures do end it 'logs' do + allow(Gitlab::AppJsonLogger).to receive(:info).with( + hash_including( + "class" => "AuthorizedProjectUpdate::ProjectRecalculatePerUserWorker" + ) + ) expect(Gitlab::AppJsonLogger).to receive(:info).with( message: 'Actor was :ci', project_id: project.id @@ -746,6 +751,11 @@ RSpec.describe Gitlab::GitAccess, :aggregate_failures do it 'logs' do expect(Gitlab::AppJsonLogger).to receive(:info).with( + hash_including( + "class" => "AuthorizedProjectUpdate::ProjectRecalculatePerUserWorker" + ) + ).once + expect(Gitlab::AppJsonLogger).to receive(:info).with( message: 'Actor was :ci', project_id: project.id ).once diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index 26883fc57b8..16e56f92b6a 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -82,50 +82,6 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do end end end - - context 'when soft_validation_on_external_url feature flag is disabled' do - before do - stub_feature_flags(soft_validation_on_external_url: false) - end - - where(:source_external_url, :expected_error_message) do - nil | nil - 'http://example.com' | nil - 'example.com' | 'is blocked: Only allowed schemes are http, https' - 'www.example.io' | 'is blocked: Only allowed schemes are http, https' - 'http://$URL' | 'is blocked: Hostname or IP address invalid' - 'http://$(URL)' | 'is blocked: Hostname or IP address invalid' - 'custom://example.com' | 'is blocked: Only allowed schemes are http, https' - '1.1.1.1' | 'is blocked: Only allowed schemes are http, https' - '$BASE_URL/${CI_COMMIT_REF_NAME}' | 'is blocked: Only allowed schemes are http, https' - '$ENVIRONMENT_URL' | 'is blocked: Only allowed schemes are http, https' - 'https://$SUB.$MAIN' | 'is blocked: Hostname or IP address invalid' - 'https://$SUB-$REGION.$MAIN' | 'is blocked: Hostname or IP address invalid' - 'https://example.com?param={()}' | nil - 'http://XSS?x=<script>alert(1)</script>' | nil - 'https://user:${VARIABLE}@example.io' | nil - 'https://example.com/test?param={data}' | nil - 'http://${URL}' | 'is blocked: URI is invalid' - 'https://${URL}.example/test' | 'is blocked: URI is invalid' - 'http://test${CI_MERGE_REQUEST_IID}.example.com' | 'is blocked: URI is invalid' - 'javascript:alert("hello")' | 'is blocked: Only allowed schemes are http, https' - end - with_them do - it 'sets an external URL or an error' do - environment.external_url = source_external_url - - environment.valid? - - if expected_error_message - expect(environment.errors[:external_url].first).to eq(expected_error_message) - else - expect(environment.errors[:external_url]).to be_empty, - "There were unexpected errors: #{environment.errors.full_messages}" - expect(environment.external_url).to eq(source_external_url) - end - end - end - end end describe '.before_save' do diff --git a/spec/services/merge_requests/reload_diffs_service_spec.rb b/spec/services/merge_requests/reload_diffs_service_spec.rb index ed89a02b570..3d5b65207e6 100644 --- a/spec/services/merge_requests/reload_diffs_service_spec.rb +++ b/spec/services/merge_requests/reload_diffs_service_spec.rb @@ -34,10 +34,7 @@ RSpec.describe MergeRequests::ReloadDiffsService, :use_clean_rails_memory_store_ context 'cache clearing' do it 'clears the cache for older diffs on the merge request' do - redis = instance_double(Redis) - expect(Gitlab::Redis::Cache).to receive(:with).and_yield(redis) - - expect(redis).to receive(:del).once + expect_any_instance_of(Redis).to receive(:del).once.and_call_original expect(Rails.cache).to receive(:delete).once.and_call_original subject.execute diff --git a/spec/services/work_items/update_service_spec.rb b/spec/services/work_items/update_service_spec.rb index 9db4139c6d5..dd5924e7434 100644 --- a/spec/services/work_items/update_service_spec.rb +++ b/spec/services/work_items/update_service_spec.rb @@ -4,7 +4,8 @@ require 'spec_helper' RSpec.describe WorkItems::UpdateService do let_it_be(:developer) { create(:user) } - let_it_be(:project) { create(:project).tap { |proj| proj.add_developer(developer) } } + let_it_be(:guest) { create(:user) } + let_it_be(:project) { create(:project) } let_it_be(:parent) { create(:work_item, project: project) } let_it_be_with_reload(:work_item) { create(:work_item, project: project, assignees: [developer]) } @@ -13,6 +14,11 @@ RSpec.describe WorkItems::UpdateService do let(:opts) { {} } let(:current_user) { developer } + before do + project.add_developer(developer) + project.add_guest(guest) + end + describe '#execute' do let(:service) do described_class.new( @@ -102,7 +108,7 @@ RSpec.describe WorkItems::UpdateService do let(:supported_widgets) do [ - { klass: WorkItems::Widgets::DescriptionService::UpdateService, callback: :update, params: { description: 'foo' } }, + { klass: WorkItems::Widgets::DescriptionService::UpdateService, callback: :before_update_callback, params: { description: 'foo' } }, { klass: WorkItems::Widgets::HierarchyService::UpdateService, callback: :before_update_in_transaction, params: { parent: parent } } ] end @@ -126,7 +132,7 @@ RSpec.describe WorkItems::UpdateService do before do allow_next_instance_of(widget_service_class) do |instance| allow(instance) - .to receive(:update) + .to receive(:before_update_callback) .with(params: { description: 'changed' }).and_return(nil) end end @@ -143,6 +149,28 @@ RSpec.describe WorkItems::UpdateService do expect(work_item.description).to eq('changed') end + context 'with mentions', :mailer, :sidekiq_might_not_need_inline do + shared_examples 'creates the todo and sends email' do |attribute| + it 'creates a todo and sends email' do + expect { perform_enqueued_jobs { update_work_item } }.to change(Todo, :count).by(1) + expect(work_item.reload.attributes[attribute.to_s]).to eq("mention #{guest.to_reference}") + should_email(guest) + end + end + + context 'when description contains a user mention' do + let(:widget_params) { { description_widget: { description: "mention #{guest.to_reference}" } } } + + it_behaves_like 'creates the todo and sends email', :description + end + + context 'when title contains a user mention' do + let(:opts) { { title: "mention #{guest.to_reference}" } } + + it_behaves_like 'creates the todo and sends email', :title + end + end + context 'when work item validation fails' do let(:opts) { { title: '' } } diff --git a/spec/services/work_items/widgets/description_service/update_service_spec.rb b/spec/services/work_items/widgets/description_service/update_service_spec.rb index a2eceb97f09..37b3f03fc6a 100644 --- a/spec/services/work_items/widgets/description_service/update_service_spec.rb +++ b/spec/services/work_items/widgets/description_service/update_service_spec.rb @@ -3,32 +3,102 @@ require 'spec_helper' RSpec.describe WorkItems::Widgets::DescriptionService::UpdateService do - let_it_be(:user) { create(:user) } - let_it_be(:project) { create(:project) } - let_it_be_with_reload(:work_item) { create(:work_item, project: project, description: 'old description') } + let_it_be(:random_user) { create(:user) } + let_it_be(:author) { create(:user) } + let_it_be(:guest) { create(:user) } + let_it_be(:reporter) { create(:user) } + let_it_be(:project) { create(:project, :public) } + + let(:params) { { description: 'updated description' } } + let(:current_user) { author } + let(:work_item) do + create(:work_item, author: author, project: project, description: 'old description', + last_edited_at: Date.yesterday, last_edited_by: random_user + ) + end let(:widget) { work_item.widgets.find {|widget| widget.is_a?(WorkItems::Widgets::Description) } } describe '#update' do - subject { described_class.new(widget: widget, current_user: user).update(params: params) } # rubocop:disable Rails/SaveBang - - context 'when description param is present' do - let(:params) { { description: 'updated description' } } + subject { described_class.new(widget: widget, current_user: current_user).before_update_callback(params: params) } + shared_examples 'sets work item description' do it 'correctly sets work item description value' do subject - expect(work_item.description).to eq('updated description') + expect(work_item.description).to eq(params[:description]) + expect(work_item.last_edited_by).to eq(current_user) + expect(work_item.last_edited_at).to be_within(2.seconds).of(Time.current) end end - context 'when description param is not present' do - let(:params) { {} } - + shared_examples 'does not set work item description' do it 'does not change work item description value' do subject expect(work_item.description).to eq('old description') + expect(work_item.last_edited_by).to eq(random_user) + expect(work_item.last_edited_at).to eq(Date.yesterday) + end + end + + context 'when user has permission to update description' do + context 'when user is work item author' do + let(:current_user) { author } + + it_behaves_like 'sets work item description' + end + + context 'when user is a project reporter' do + let(:current_user) { reporter } + + before do + project.add_reporter(reporter) + end + + it_behaves_like 'sets work item description' + end + + context 'when description is nil' do + let(:current_user) { author } + let(:params) { { description: nil } } + + it_behaves_like 'sets work item description' + end + + context 'when description is empty' do + let(:current_user) { author } + let(:params) { { description: '' } } + + it_behaves_like 'sets work item description' + end + + context 'when description param is not present' do + let(:params) { {} } + + it_behaves_like 'does not set work item description' + end + end + + context 'when user does not have permission to update description' do + context 'when user is a project guest' do + let(:current_user) { guest } + + before do + project.add_guest(guest) + end + + it_behaves_like 'does not set work item description' + end + + context 'with private project' do + let_it_be(:project) { create(:project) } + + context 'when user is work item author' do + let(:current_user) { author } + + it_behaves_like 'does not set work item description' + end end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 921d6503099..47cd78873f8 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -208,7 +208,6 @@ RSpec.configure do |config| include StubFeatureFlags include StubSnowplow - include StubMember if ENV['CI'] || ENV['RETRIES'] # This includes the first try, i.e. tests will be run 4 times before failing. diff --git a/spec/support/helpers/stub_member.rb b/spec/support/helpers/stub_member.rb deleted file mode 100644 index b8fb7bce234..00000000000 --- a/spec/support/helpers/stub_member.rb +++ /dev/null @@ -1,8 +0,0 @@ -# frozen_string_literal: true - -module StubMember - def self.included(base) - GroupMember.prepend(StubbedMember::GroupMember) - ProjectMember.prepend(StubbedMember::ProjectMember) - end -end diff --git a/spec/support/helpers/stubbed_member.rb b/spec/support/helpers/stubbed_member.rb deleted file mode 100644 index bf700229656..00000000000 --- a/spec/support/helpers/stubbed_member.rb +++ /dev/null @@ -1,64 +0,0 @@ -# frozen_string_literal: true - -# Extend the ProjectMember & GroupMember class with the ability to -# to run project_authorizations refresh jobs inline. - -# This is needed so that calls like `group.add_member(user)` or `create(:project_member)` -# in the specs can be run without including `:sidekiq_inline` trait. -module StubbedMember - extend ActiveSupport::Concern - - module ClearDeduplicationData - private - - def clear_deduplication_data! - Gitlab::Redis::Queues.with do |redis| - redis.scan_each(match: '*duplicate*').each do |key| - redis.del(key) - end - end - end - end - - module GroupMember - include ClearDeduplicationData - - private - - def refresh_member_authorized_projects(blocking:) - return super unless blocking - - # First, we remove all the keys associated with deduplication from Redis. - # We can't perform a full flush with `Gitlab::Redis::Queues.with(&:flushdb)` - # because that is going to remove other, unrelated enqueued jobs as well, - # and that is going to fail some specs. - clear_deduplication_data! - - # then we run `super`, which will enqueue a project authorizations refresh job - super - - # then we drain (run) the jobs that were enqueued, but only for the worker class we are interested in. - AuthorizedProjectsWorker.drain - ensure - clear_deduplication_data! - end - end - - module ProjectMember - include ClearDeduplicationData - - private - - def refresh_member_authorized_projects(blocking:) - return super unless blocking - - clear_deduplication_data! - - super - - AuthorizedProjectUpdate::ProjectRecalculatePerUserWorker.drain - ensure - clear_deduplication_data! - end - end -end diff --git a/spec/workers/concerns/waitable_worker_spec.rb b/spec/workers/concerns/waitable_worker_spec.rb index bf156c3b8cb..f6d4cc4679d 100644 --- a/spec/workers/concerns/waitable_worker_spec.rb +++ b/spec/workers/concerns/waitable_worker_spec.rb @@ -30,33 +30,19 @@ RSpec.describe WaitableWorker do describe '.bulk_perform_and_wait' do context '1 job' do - it 'runs the jobs asynchronously' do - arguments = [[1]] - - expect(worker).to receive(:bulk_perform_async).with(arguments) - - worker.bulk_perform_and_wait(arguments) - end - - context 'when the feature flag `always_async_project_authorizations_refresh` is turned off' do - before do - stub_feature_flags(always_async_project_authorizations_refresh: false) - end - - it 'inlines the job' do - args_list = [[1]] - expect(worker).to receive(:bulk_perform_inline).with(args_list).and_call_original - expect(Gitlab::AppJsonLogger).to( - receive(:info).with(a_hash_including('message' => 'running inline', - 'class' => 'Gitlab::Foo::Bar::DummyWorker', - 'job_status' => 'running', - 'queue' => 'foo_bar_dummy')) - .once) - - worker.bulk_perform_and_wait(args_list) - - expect(worker.counter).to eq(1) - end + it 'inlines the job' do + args_list = [[1]] + expect(worker).to receive(:bulk_perform_inline).with(args_list).and_call_original + expect(Gitlab::AppJsonLogger).to( + receive(:info).with(a_hash_including('message' => 'running inline', + 'class' => 'Gitlab::Foo::Bar::DummyWorker', + 'job_status' => 'running', + 'queue' => 'foo_bar_dummy')) + .once) + + worker.bulk_perform_and_wait(args_list) + + expect(worker.counter).to eq(1) end end |