diff options
| -rw-r--r-- | app/models/namespaces/traversal/linear.rb | 20 | ||||
| -rw-r--r-- | app/models/notification_setting.rb | 2 | ||||
| -rw-r--r-- | app/services/notification_recipients/builder/base.rb | 44 | ||||
| -rw-r--r-- | config/feature_flags/development/notification_setting_recipient_refactor.yml | 8 | ||||
| -rw-r--r-- | doc/administration/auth/atlassian.md | 2 | ||||
| -rw-r--r-- | doc/administration/auth/authentiq.md | 2 | ||||
| -rw-r--r-- | doc/development/contributing/style_guides.md | 1 | ||||
| -rw-r--r-- | lefthook.yml | 5 | ||||
| -rwxr-xr-x | scripts/lint-doc.sh | 1 | ||||
| -rwxr-xr-x | scripts/lint-docs-metadata.sh | 24 | ||||
| -rw-r--r-- | spec/services/notification_recipients/builder/default_spec.rb | 146 |
11 files changed, 229 insertions, 26 deletions
diff --git a/app/models/namespaces/traversal/linear.rb b/app/models/namespaces/traversal/linear.rb index 294ef83b9b4..dd9ca8d9bea 100644 --- a/app/models/namespaces/traversal/linear.rb +++ b/app/models/namespaces/traversal/linear.rb @@ -86,25 +86,7 @@ module Namespaces raise UnboundedSearch.new('Must bound search by a top') unless top without_sti_condition - .traversal_ids_contains(latest_traversal_ids(top)) - end - - # traversal_ids are a cached value. - # - # The traversal_ids value in a loaded object can become stale when compared - # to the database value. For example, if you load a hierarchy and then move - # a group, any previously loaded descendant objects will have out of date - # traversal_ids. - # - # To solve this problem, we never depend on the object's traversal_ids - # value. We always query the database first with a sub-select for the - # latest traversal_ids. - # - # Note that ActiveRecord will cache query results. You can avoid this by - # using `Model.uncached { ... }` - def latest_traversal_ids(namespace = self) - without_sti_condition.where('id = (?)', namespace) - .select('traversal_ids as latest_traversal_ids') + .traversal_ids_contains("{#{top.id}}") end end end diff --git a/app/models/notification_setting.rb b/app/models/notification_setting.rb index 9c3058b4706..3d049336d44 100644 --- a/app/models/notification_setting.rb +++ b/app/models/notification_setting.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class NotificationSetting < ApplicationRecord + include FromUnion + enum level: { global: 3, watch: 2, participating: 1, mention: 4, disabled: 0, custom: 5 } default_value_for :level, NotificationSetting.levels[:global] diff --git a/app/services/notification_recipients/builder/base.rb b/app/services/notification_recipients/builder/base.rb index 81e6750a4ca..b41b969ad7c 100644 --- a/app/services/notification_recipients/builder/base.rb +++ b/app/services/notification_recipients/builder/base.rb @@ -100,6 +100,8 @@ module NotificationRecipients # Get project/group users with CUSTOM notification level # rubocop: disable CodeReuse/ActiveRecord def add_custom_notifications + return new_add_custom_notifications if Feature.enabled?(:notification_setting_recipient_refactor, project) + user_ids = [] # Users with a notification setting on group or project @@ -115,6 +117,48 @@ module NotificationRecipients add_recipients(user_scope.where(id: user_ids), :custom, nil) end + + def new_add_custom_notifications + notification_by_sources = related_notification_settings_sources(:custom) + + return if notification_by_sources.blank? + + user_ids = NotificationSetting.from_union(notification_by_sources).select(:user_id) + + add_recipients(user_scope.where(id: user_ids), :custom, nil) + end + + def related_notification_settings_sources(level) + sources = [project, group].compact + + sources.map do |source| + source + .notification_settings + .where(source_or_global_setting_by_level_query(level)).select(:user_id) + end + end + + def global_setting_by_level_query(level) + table = NotificationSetting.arel_table + aliased_table = table.alias + + table + .project('true') + .from(aliased_table) + .where( + aliased_table[:user_id].eq(table[:user_id]) + .and(aliased_table[:source_id].eq(nil)) + .and(aliased_table[:source_type].eq(nil)) + .and(aliased_table[:level].eq(level)) + ).exists + end + + def source_or_global_setting_by_level_query(level) + table = NotificationSetting.arel_table + table.grouping( + table[:level].eq(:global).and(global_setting_by_level_query(level)) + ).or(table[:level].eq(level)) + end # rubocop: enable CodeReuse/ActiveRecord def add_project_watchers diff --git a/config/feature_flags/development/notification_setting_recipient_refactor.yml b/config/feature_flags/development/notification_setting_recipient_refactor.yml new file mode 100644 index 00000000000..7379fe42166 --- /dev/null +++ b/config/feature_flags/development/notification_setting_recipient_refactor.yml @@ -0,0 +1,8 @@ +--- +name: notification_setting_recipient_refactor +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57688 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/327303 +milestone: '13.11' +type: development +group: group::code review +default_enabled: false diff --git a/doc/administration/auth/atlassian.md b/doc/administration/auth/atlassian.md index 9c5da558b0d..78fb709886f 100644 --- a/doc/administration/auth/atlassian.md +++ b/doc/administration/auth/atlassian.md @@ -12,7 +12,7 @@ To enable the Atlassian OmniAuth provider for passwordless authentication you mu ## Atlassian application registration 1. Go to <https://developer.atlassian.com/apps/> and sign-in with the Atlassian - account that will administer the application. + account to administer the application with. 1. Click **Create a new app**. diff --git a/doc/administration/auth/authentiq.md b/doc/administration/auth/authentiq.md index dbc5e446287..3a3b81fc263 100644 --- a/doc/administration/auth/authentiq.md +++ b/doc/administration/auth/authentiq.md @@ -9,7 +9,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w To enable the Authentiq OmniAuth provider for passwordless authentication you must register an application with Authentiq. -Authentiq will generate a Client ID and the accompanying Client Secret for you to use. +Authentiq generates a Client ID and the accompanying Client Secret for you to use. 1. Get your Client credentials (Client ID and Client Secret) at [Authentiq](https://www.authentiq.com/developers). diff --git a/doc/development/contributing/style_guides.md b/doc/development/contributing/style_guides.md index 444a067a780..0f4b69d3daf 100644 --- a/doc/development/contributing/style_guides.md +++ b/doc/development/contributing/style_guides.md @@ -62,6 +62,7 @@ Before you push your changes, Lefthook automatically runs the following checks: - SCSS lint: Run `yarn lint:stylelint` checks (with the [`.stylelintrc`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/.stylelintrc) configuration) on the modified `*.scss{,.css}` files. Tags: `stylesheet`, `css`, `style`. - RuboCop: Run `bundle exec rubocop` checks (with the [`.rubocop.yml`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/.rubocop.yml) configuration) on the modified `*.rb` files. Tags: `backend`, `style`. - Vale: Run `vale` checks (with the [`.vale.ini`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/.vale.ini) configuration) on the modified `*.md` files. Tags: `documentation`, `style`. +- Documentation metadata: Run checks for the absence of [documentation metadata](../documentation/index.md#metadata). In addition to the default configuration, you can define a [local configuration](https://github.com/Arkweid/lefthook/blob/master/docs/full_guide.md#local-config). diff --git a/lefthook.yml b/lefthook.yml index daab6e7f4a0..d29dce157db 100644 --- a/lefthook.yml +++ b/lefthook.yml @@ -38,3 +38,8 @@ pre-push: files: git diff --name-only --diff-filter=d $(git merge-base origin/master HEAD)..HEAD glob: 'doc/*.md' run: if command -v vale 2> /dev/null; then vale --config .vale.ini --minAlertLevel error {files}; else echo "Vale not found. Install Vale"; fi + docs-metadata: # https://docs.gitlab.com/ee/development/documentation/#metadata + tags: documentation style + files: git diff --name-only --diff-filter=d $(git merge-base origin/master HEAD)..HEAD + glob: '*.md' + run: scripts/lint-docs-metadata.sh "{files}" diff --git a/scripts/lint-doc.sh b/scripts/lint-doc.sh index 4d0c1ba99d2..e11ce60c17f 100755 --- a/scripts/lint-doc.sh +++ b/scripts/lint-doc.sh @@ -19,6 +19,7 @@ then fi # Documentation pages need front matter for tracking purposes. +# See also lint-docs-metadata.sh echo '=> Checking documentation for front matter...' echo no_frontmatter=$(find doc -name "*.md" -exec head -n1 {} \; | grep -v --count -- ---) diff --git a/scripts/lint-docs-metadata.sh b/scripts/lint-docs-metadata.sh new file mode 100755 index 00000000000..75e4b9c2418 --- /dev/null +++ b/scripts/lint-docs-metadata.sh @@ -0,0 +1,24 @@ +#!/usr/bin/env bash + +# Used in `lefthook.yml` +(( counter=0 )) + +for file in $1 +do + if [ "$(head -n1 "$file")" != "---" ] + then + printf "%sDocumentation metadata missing in %s%s\n" "$(tput setaf 1)" "$file" "$(tput sgr0)" + (( counter++ )) + else + printf "Documentation metadata found in %s.\n" "$file" + fi +done + +if [ "$counter" -gt 0 ] +then + printf "\n%sDocumentation metadata is missing in changed documentation files.%s For more information, see https://docs.gitlab.com/ee/development/documentation/#metadata.\n" "$(tput setaf 1)" "$(tput sgr0)" + false +else + printf "\n%sDocumentation metadata found in all changed documentation files.%s\n" "$(tput setaf 2)" "$(tput sgr0)" + true +fi diff --git a/spec/services/notification_recipients/builder/default_spec.rb b/spec/services/notification_recipients/builder/default_spec.rb index d25410235c2..994138ea828 100644 --- a/spec/services/notification_recipients/builder/default_spec.rb +++ b/spec/services/notification_recipients/builder/default_spec.rb @@ -6,7 +6,7 @@ RSpec.describe NotificationRecipients::Builder::Default do describe '#build!' do let_it_be(:group) { create(:group, :public) } let_it_be(:project) { create(:project, :public, group: group).tap { |p| p.add_developer(project_watcher) } } - let_it_be(:issue) { create(:issue, project: project) } + let_it_be(:target) { create(:issue, project: project) } let_it_be(:current_user) { create(:user) } let_it_be(:other_user) { create(:user) } @@ -17,11 +17,11 @@ RSpec.describe NotificationRecipients::Builder::Default do let_it_be(:notification_setting_project_w) { create(:notification_setting, source: project, user: project_watcher, level: 2) } let_it_be(:notification_setting_group_w) { create(:notification_setting, source: group, user: group_watcher, level: 2) } - subject { described_class.new(issue, current_user, action: :new).tap { |s| s.build! } } + subject { described_class.new(target, current_user, action: :new).tap { |s| s.build! } } context 'participants and project watchers' do before do - expect(issue).to receive(:participants).and_return([participant, current_user]) + expect(target).to receive(:participants).and_return([participant, current_user]) end it 'adds all participants and watchers' do @@ -34,11 +34,147 @@ RSpec.describe NotificationRecipients::Builder::Default do it 'adds all subscribers' do subscriber = create(:user) non_subscriber = create(:user) - create(:subscription, project: project, user: subscriber, subscribable: issue, subscribed: true) - create(:subscription, project: project, user: non_subscriber, subscribable: issue, subscribed: false) + create(:subscription, project: project, user: subscriber, subscribable: target, subscribed: true) + create(:subscription, project: project, user: non_subscriber, subscribable: target, subscribed: false) expect(subject.recipients.map(&:user)).to include(subscriber) end end + + context 'custom notifications' do + shared_examples 'custom notification recipients' do + let_it_be(:custom_notification_user) { create(:user) } + let_it_be(:another_group) { create(:group) } + let_it_be(:another_project) { create(:project, namespace: another_group) } + + context 'with project custom notification setting' do + before do + create(:notification_setting, source: project, user: custom_notification_user, level: :custom) + end + + it 'adds the user to the recipients' do + expect(subject.recipients.map(&:user)).to include(custom_notification_user) + end + end + + context 'with the project custom notification setting in another project' do + before do + create(:notification_setting, source: another_project, user: custom_notification_user, level: :custom) + end + + it 'does not add the user to the recipients' do + expect(subject.recipients.map(&:user)).not_to include(custom_notification_user) + end + end + + context 'with group custom notification setting' do + before do + create(:notification_setting, source: group, user: custom_notification_user, level: :custom) + end + + it 'adds the user to the recipients' do + expect(subject.recipients.map(&:user)).to include(custom_notification_user) + end + end + + context 'with the group custom notification setting in another group' do + before do + create(:notification_setting, source: another_group, user: custom_notification_user, level: :custom) + end + + it 'does not add the user to the recipients' do + expect(subject.recipients.map(&:user)).not_to include(custom_notification_user) + end + end + + context 'with project global custom notification setting' do + before do + create(:notification_setting, source: project, user: custom_notification_user, level: :global) + end + + context 'with global custom notification setting' do + before do + create(:notification_setting, source: nil, user: custom_notification_user, level: :custom) + end + + it 'adds the user to the recipients' do + expect(subject.recipients.map(&:user)).to include(custom_notification_user) + end + end + + context 'without global custom notification setting' do + it 'does not add the user to the recipients' do + expect(subject.recipients.map(&:user)).not_to include(custom_notification_user) + end + end + end + + context 'with group global custom notification setting' do + before do + create(:notification_setting, source: group, user: custom_notification_user, level: :global) + end + + context 'with global custom notification setting' do + before do + create(:notification_setting, source: nil, user: custom_notification_user, level: :custom) + end + + it 'adds the user to the recipients' do + expect(subject.recipients.map(&:user)).to include(custom_notification_user) + end + end + + context 'without global custom notification setting' do + it 'does not add the user to the recipients' do + expect(subject.recipients.map(&:user)).not_to include(custom_notification_user) + end + end + end + + context 'with group custom notification setting in deeply nested parent group' do + let(:grand_parent_group) { create(:group, :public) } + let(:parent_group) { create(:group, :public, parent: grand_parent_group) } + let(:group) { create(:group, :public, parent: parent_group) } + let(:project) { create(:project, :public, group: group).tap { |p| p.add_developer(project_watcher) } } + let(:target) { create(:issue, project: project) } + + before do + create(:notification_setting, source: grand_parent_group, user: custom_notification_user, level: :custom) + end + + it 'adds the user to the recipients' do + expect(subject.recipients.map(&:user)).to include(custom_notification_user) + end + end + + context 'without a project or group' do + let(:target) { create(:snippet) } + + before do + create(:notification_setting, source: nil, user: custom_notification_user, level: :custom) + end + + it 'does not add the user to the recipients' do + expect(subject.recipients.map(&:user)).not_to include(custom_notification_user) + end + end + end + + before do + stub_feature_flags(notification_setting_recipient_refactor: enabled) + end + + context 'with notification_setting_recipient_refactor enabled' do + let(:enabled) { true } + + it_behaves_like 'custom notification recipients' + end + + context 'with notification_setting_recipient_refactor disabled' do + let(:enabled) { false } + + it_behaves_like 'custom notification recipients' + end + end end end |