From cefe80c77b4f9e99509e67952bf7332253749717 Mon Sep 17 00:00:00 2001 From: Joseph Snyder Date: Fri, 9 Jun 2023 08:54:41 -0400 Subject: [PATCH 1/7] Switch Groups UX to emails_enabled Change the frontend experience for the groups to use emails_enabled instead of emails_disabled. Properly "ignore" the emails_disabled column in the model Changelog: added EE: true --- app/assets/javascripts/boards/constants.js | 1 + app/assets/javascripts/boards/index.js | 2 +- .../sidebar/stores/sidebar_store.js | 1 + app/models/group.rb | 1 - app/models/namespace.rb | 14 +---- app/models/namespace_setting.rb | 1 + app/services/groups/update_service.rb | 5 +- .../groups/settings/_permissions.html.haml | 8 +-- .../assets/javascripts/epic_boards/index.js | 2 +- .../epic_boards/epic_boards_sidebar_spec.rb | 2 +- .../group_attributes_transformer.rb | 2 +- locale/gitlab.pot | 10 ++-- spec/features/groups/group_settings_spec.rb | 10 +++- ...group_notification_settings_finder_spec.rb | 6 +- spec/frontend/sidebar/mock_data.js | 7 ++- spec/graphql/types/group_type_spec.rb | 2 +- .../group_attributes_transformer_spec.rb | 4 +- spec/models/group_spec.rb | 17 ++++-- spec/models/namespace_setting_spec.rb | 30 ++++++++++ spec/models/namespace_spec.rb | 56 ++----------------- spec/models/project_setting_spec.rb | 2 +- spec/presenters/issue_presenter_spec.rb | 32 +++++++++++ spec/services/groups/update_service_spec.rb | 12 ++-- .../notification_service_shared_examples.rb | 4 +- 24 files changed, 132 insertions(+), 99 deletions(-) diff --git a/app/assets/javascripts/boards/constants.js b/app/assets/javascripts/boards/constants.js index 84564560fe3321..c12e514f27feeb 100644 --- a/app/assets/javascripts/boards/constants.js +++ b/app/assets/javascripts/boards/constants.js @@ -124,6 +124,7 @@ export const listIssuablesQueries = { closedAt: null, dueDate: null, emailsDisabled: false, + emailsEnabled: false, hidden: false, humanTimeEstimate: null, humanTotalTimeSpent: null, diff --git a/app/assets/javascripts/boards/index.js b/app/assets/javascripts/boards/index.js index 0d8882cf57e097..02c57fe1d2c1e0 100644 --- a/app/assets/javascripts/boards/index.js +++ b/app/assets/javascripts/boards/index.js @@ -55,7 +55,7 @@ function mountBoardApp(el) { releasesFetchPath: el.dataset.releasesFetchPath, timeTrackingLimitToHours: parseBoolean(el.dataset.timeTrackingLimitToHours), issuableType: TYPE_ISSUE, - emailsDisabled: parseBoolean(el.dataset.emailsDisabled), + emailsDisabled: !parseBoolean(el.dataset.emailsEnabled), hasMissingBoards: parseBoolean(el.dataset.hasMissingBoards), weights: el.dataset.weights ? JSON.parse(el.dataset.weights) : [], isIssueBoard: true, diff --git a/app/assets/javascripts/sidebar/stores/sidebar_store.js b/app/assets/javascripts/sidebar/stores/sidebar_store.js index ea3b3633ea7671..bda670dfa698d2 100644 --- a/app/assets/javascripts/sidebar/stores/sidebar_store.js +++ b/app/assets/javascripts/sidebar/stores/sidebar_store.js @@ -32,6 +32,7 @@ export default class SidebarStore { this.isLockDialogOpen = false; this.participants = []; this.projectEmailsDisabled = false; + this.projectEmailsEnabled = true; this.subscribeDisabledDescription = ''; this.subscribed = null; this.changing = false; diff --git a/app/models/group.rb b/app/models/group.rb index 30ba16354bd5d4..6ad1ca5e1e4f3f 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -350,7 +350,6 @@ def preset_root_ancestor_for(groups) # column is set to false anywhere in the ancestor hierarchy. def ids_with_disabled_email(groups) inner_groups = Group.where('id = namespaces_with_emails_disabled.id') - inner_query = inner_groups .self_and_ancestors .joins(:namespace_settings) diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 511f03517adbe1..c0586591682f6d 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -25,6 +25,8 @@ class Namespace < ApplicationRecord cross_database_ignore_tables %w[routes redirect_routes], url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/424277' + ignore_column :emails_disabled, remove_with: '16.8', remove_after: '2024-01-18' + # Tells ActiveRecord not to store the full class name, in order to save some space # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/69794 self.store_full_sti_class = false @@ -166,7 +168,6 @@ class Namespace < ApplicationRecord :lock_math_rendering_limits_enabled?, to: :namespace_settings - before_save :update_new_emails_created_column, if: -> { emails_disabled_changed? } before_create :sync_share_with_group_lock_with_parent before_update :sync_share_with_group_lock_with_parent, if: :parent_changed? after_update :force_share_with_group_lock_on_descendants, if: -> { saved_change_to_share_with_group_lock? && share_with_group_lock? } @@ -712,17 +713,6 @@ def cross_project_reference?(from) end end - def update_new_emails_created_column - return if namespace_settings.nil? - return if namespace_settings.emails_enabled == !emails_disabled - - if namespace_settings.persisted? - namespace_settings.update!(emails_enabled: !emails_disabled) - elsif namespace_settings - namespace_settings.emails_enabled = !emails_disabled - end - end - def cluster_enabled_granted? (Gitlab.com? || Gitlab.dev_or_test_env?) && root_ancestor.cluster_enabled_grant.present? end diff --git a/app/models/namespace_setting.rb b/app/models/namespace_setting.rb index 7970fbc40ba565..64634c03f677d3 100644 --- a/app/models/namespace_setting.rb +++ b/app/models/namespace_setting.rb @@ -36,6 +36,7 @@ class NamespaceSetting < ApplicationRecord chronic_duration_attr :project_runner_token_expiration_interval_human_readable, :project_runner_token_expiration_interval NAMESPACE_SETTINGS_PARAMS = %i[ + emails_enabled default_branch_name resource_access_token_creation_allowed prevent_sharing_groups_outside_hierarchy diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index 8c47feb813f2fc..26fdb480376b09 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -135,7 +135,10 @@ def reject_parent_id! # overridden in EE def remove_unallowed_params - params.delete(:emails_disabled) unless can?(current_user, :set_emails_disabled, group) + unless can?(current_user, :set_emails_disabled, group) + params.delete(:emails_disabled) + params.delete(:emails_enabled) + end unless can?(current_user, :update_default_branch_protection, group) params.delete(:default_branch_protection) diff --git a/app/views/groups/settings/_permissions.html.haml b/app/views/groups/settings/_permissions.html.haml index 15e245c6fb43bc..ff2aa1e32bc6d7 100644 --- a/app/views/groups/settings/_permissions.html.haml +++ b/app/views/groups/settings/_permissions.html.haml @@ -18,10 +18,10 @@ help_text: share_with_group_lock_help_text(@group) .form-group.gl-mb-3 - = f.gitlab_ui_checkbox_component :emails_disabled, - s_('GroupSettings|Email notifications are disabled'), - checkbox_options: { checked: @group.emails_disabled?, disabled: !can_disable_group_emails?(@group) }, - help_text: s_('GroupSettings|Overrides user notification preferences for all members of the group, subgroups, and projects.') + = f.gitlab_ui_checkbox_component :emails_enabled, + s_('GroupSettings|Enable email notifications'), + checkbox_options: { checked: @group.emails_enabled?, disabled: !can_disable_group_emails?(@group) }, + help_text: s_('GroupSettings|Enable sending email notifications for this group and all its subgroups and projects') .form-group.gl-mb-3 = f.gitlab_ui_checkbox_component :mentions_disabled, diff --git a/ee/app/assets/javascripts/epic_boards/index.js b/ee/app/assets/javascripts/epic_boards/index.js index f5cb1d8c8ee19f..14dcf2ca803bc1 100644 --- a/ee/app/assets/javascripts/epic_boards/index.js +++ b/ee/app/assets/javascripts/epic_boards/index.js @@ -57,7 +57,7 @@ function mountBoardApp(el) { timeTrackingLimitToHours: parseBoolean(el.dataset.timeTrackingLimitToHours), boardWeight: el.dataset.boardWeight ? parseInt(el.dataset.boardWeight, 10) : null, issuableType: TYPE_EPIC, - emailsDisabled: parseBoolean(el.dataset.emailsDisabled), + emailsDisabled: !parseBoolean(el.dataset.emailsEnabled), hasMissingBoards: parseBoolean(el.dataset.hasMissingBoards), weights: JSON.parse(el.dataset.weights), isIssueBoard: false, diff --git a/ee/spec/features/epic_boards/epic_boards_sidebar_spec.rb b/ee/spec/features/epic_boards/epic_boards_sidebar_spec.rb index fdbed10e06f772..500f8be5f980e3 100644 --- a/ee/spec/features/epic_boards/epic_boards_sidebar_spec.rb +++ b/ee/spec/features/epic_boards/epic_boards_sidebar_spec.rb @@ -207,7 +207,7 @@ context 'when notifications have been disabled' do before do - group.update_attribute(:emails_disabled, true) + group.update_attribute(:emails_enabled, false) refresh_and_click_first_card end diff --git a/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb b/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb index 85b52117dbc129..9afaf4f59ac915 100644 --- a/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb +++ b/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb @@ -23,7 +23,7 @@ def transform(context, data) 'path' => uniquify(namespace, path, :path), 'description' => data['description'], 'lfs_enabled' => data['lfs_enabled'], - 'emails_disabled' => data['emails_disabled'], + 'emails_enabled' => !data['emails_disabled'], 'mentions_disabled' => data['mentions_disabled'], 'share_with_group_lock' => data['share_with_group_lock'] } diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 6a09185bc77eb9..b0c676a50a7d38 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -24584,15 +24584,18 @@ msgstr "" msgid "GroupSettings|Default to Auto DevOps pipeline for all projects within this group" msgstr "" -msgid "GroupSettings|Email notifications are disabled" +msgid "GroupSettings|Enable caching of hierarchical objects (subgroups and projects) to improve the performance of group-level features within a large group." msgstr "" -msgid "GroupSettings|Enable caching of hierarchical objects (subgroups and projects) to improve the performance of group-level features within a large group." +msgid "GroupSettings|Enable email notifications" msgstr "" msgid "GroupSettings|Enable overview background aggregation for Value Streams Dashboard" msgstr "" +msgid "GroupSettings|Enable sending email notifications for this group and all its subgroups and projects" +msgstr "" + msgid "GroupSettings|Enabling these features is your acceptance of the %{link_start}GitLab Testing Agreement%{link_end}." msgstr "" @@ -24641,9 +24644,6 @@ msgstr "" msgid "GroupSettings|Organizations and contacts can be created and associated with issues." msgstr "" -msgid "GroupSettings|Overrides user notification preferences for all members of the group, subgroups, and projects." -msgstr "" - msgid "GroupSettings|Pipeline settings was updated for the group" msgstr "" diff --git a/spec/features/groups/group_settings_spec.rb b/spec/features/groups/group_settings_spec.rb index fd69ea7a260a93..75ed390e021411 100644 --- a/spec/features/groups/group_settings_spec.rb +++ b/spec/features/groups/group_settings_spec.rb @@ -212,17 +212,21 @@ end context 'disable email notifications' do + before do + group.namespace_settings.update!({ emails_enabled: false }) + end + it 'is visible' do visit edit_group_path(group) - expect(page).to have_selector('#group_emails_disabled', visible: true) + expect(page).to have_selector('#group_emails_enabled', visible: true) end it 'accepts the changed state' do visit edit_group_path(group) - check 'group_emails_disabled' + check 'group_emails_enabled' - expect { save_permissions_group }.to change { updated_emails_disabled? }.to(true) + expect { save_permissions_group }.to change { updated_emails_disabled? }.to(false) end end diff --git a/spec/finders/user_group_notification_settings_finder_spec.rb b/spec/finders/user_group_notification_settings_finder_spec.rb index 83d0d343c04450..7c4a1752dd86b5 100644 --- a/spec/finders/user_group_notification_settings_finder_spec.rb +++ b/spec/finders/user_group_notification_settings_finder_spec.rb @@ -134,10 +134,12 @@ def attributes(&proc) let_it_be(:sub_sub_group) { create(:group, parent: sub_group) } let_it_be(:another_root_group) { create(:group) } - let_it_be(:sub_group_with_emails_disabled) { create(:group, emails_enabled: false, parent: another_root_group) } + let_it_be(:sub_group_with_emails_disabled_settings) { create(:namespace_settings) } + let_it_be(:sub_group_with_emails_disabled) { create(:group, namespace_settings: sub_group_with_emails_disabled_settings, emails_enabled: false, parent: another_root_group) } let_it_be(:another_sub_sub_group) { create(:group, parent: sub_group_with_emails_disabled) } - let_it_be(:root_group_with_emails_disabled) { create(:group, emails_enabled: false) } + let_it_be(:root_group_with_emails_disabled_settings) { create(:namespace_settings) } + let_it_be(:root_group_with_emails_disabled) { create(:group, namespace_settings: root_group_with_emails_disabled_settings, emails_enabled: false) } let_it_be(:group) { create(:group, parent: root_group_with_emails_disabled) } let(:groups) { Group.where(id: [sub_sub_group, another_sub_sub_group, group]) } diff --git a/spec/frontend/sidebar/mock_data.js b/spec/frontend/sidebar/mock_data.js index 9d8392ad5f0db4..c326ae4fccc9e0 100644 --- a/spec/frontend/sidebar/mock_data.js +++ b/spec/frontend/sidebar/mock_data.js @@ -317,7 +317,11 @@ export const issueReferenceResponse = (reference) => ({ }, }); -export const issueSubscriptionsResponse = (subscribed = false, emailsDisabled = false) => ({ +export const issueSubscriptionsResponse = ( + subscribed = false, + emailsDisabled = false, + emailsEnabled = true, +) => ({ data: { workspace: { id: '1', @@ -327,6 +331,7 @@ export const issueSubscriptionsResponse = (subscribed = false, emailsDisabled = id: 'gid://gitlab/Issue/4', subscribed, emailsDisabled, + emailsEnabled, }, }, }, diff --git a/spec/graphql/types/group_type_spec.rb b/spec/graphql/types/group_type_spec.rb index dd23dbb0370fbd..6c7d1c246cff49 100644 --- a/spec/graphql/types/group_type_spec.rb +++ b/spec/graphql/types/group_type_spec.rb @@ -18,7 +18,7 @@ web_url avatar_url share_with_group_lock project_creation_level descendant_groups_count group_members_count projects_count subgroup_creation_level require_two_factor_authentication - two_factor_grace_period auto_devops_enabled emails_disabled + two_factor_grace_period auto_devops_enabled emails_disabled emails_enabled mentions_disabled parent boards milestones group_members merge_requests container_repositories container_repositories_count packages dependency_proxy_setting dependency_proxy_manifests diff --git a/spec/lib/bulk_imports/groups/transformers/group_attributes_transformer_spec.rb b/spec/lib/bulk_imports/groups/transformers/group_attributes_transformer_spec.rb index 69d5997cf96d33..5730071b694d6f 100644 --- a/spec/lib/bulk_imports/groups/transformers/group_attributes_transformer_spec.rb +++ b/spec/lib/bulk_imports/groups/transformers/group_attributes_transformer_spec.rb @@ -51,7 +51,7 @@ 'visibility_level' => Gitlab::VisibilityLevel.string_options[data['visibility']], 'project_creation_level' => Gitlab::Access.project_creation_string_options[data['project_creation_level']], 'subgroup_creation_level' => Gitlab::Access.subgroup_creation_string_options[data['subgroup_creation_level']], - 'emails_disabled' => true, + 'emails_enabled' => false, 'lfs_enabled' => false, 'mentions_disabled' => true, 'share_with_group_lock' => false, @@ -78,7 +78,7 @@ 'description' => 'Source Group Description', 'parent_id' => destination_group.id, 'share_with_group_lock' => nil, - 'emails_disabled' => nil, + 'emails_enabled' => true, 'lfs_enabled' => nil, 'mentions_disabled' => nil }) diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 316830d5ecec55..347bbaa7f78386 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -3076,7 +3076,9 @@ def define_cache_expectations(cache_key) end describe '#first_owner' do - let(:group) { build(:group) } + # Namespace setting needed due to "notifiable" looking for email state + let_it_be(:group_settings) { create(:namespace_settings) } + let(:group) { build(:group, namespace_settings: group_settings) } context 'the group has owners' do it 'is the first owner' do @@ -3094,7 +3096,8 @@ def define_cache_expectations(cache_key) end context 'the group has a parent' do - let(:parent) { build(:group) } + let_it_be(:parent_settings) { create(:namespace_settings) } + let(:parent) { build(:group, namespace_settings: parent_settings) } before do group.parent = parent @@ -3191,13 +3194,17 @@ def define_cache_expectations(cache_key) end describe '.ids_with_disabled_email' do - let_it_be(:parent_1) { create(:group) } + let_it_be(:group_settings_1) { create(:namespace_settings, emails_enabled: false) } + let_it_be(:group_settings_2) { create(:namespace_settings, emails_enabled: true) } + let_it_be(:group_settings_other) { create(:namespace_settings, emails_enabled: true) } + + let_it_be(:parent_1) { create(:group, namespace_settings: group_settings_1) } let_it_be(:child_1) { create(:group, parent: parent_1) } - let_it_be(:parent_2) { create(:group) } + let_it_be(:parent_2) { create(:group, namespace_settings: group_settings_2) } let_it_be(:child_2) { create(:group, parent: parent_2) } - let_it_be(:other_group) { create(:group) } + let_it_be(:other_group) { create(:group, namespace_settings: group_settings_other) } shared_examples 'returns namespaces with disabled email' do subject(:group_ids_where_email_is_disabled) { described_class.ids_with_disabled_email([child_1, child_2, other_group]) } diff --git a/spec/models/namespace_setting_spec.rb b/spec/models/namespace_setting_spec.rb index 5358446c333aa4..5bf79f691732ba 100644 --- a/spec/models/namespace_setting_spec.rb +++ b/spec/models/namespace_setting_spec.rb @@ -184,6 +184,36 @@ end end + describe '#emails_enabled?' do + context 'when a group has no parent' + let(:settings) { create(:namespace_settings, emails_enabled: true) } + let(:grandparent) { create(:group) } + let(:parent) { create(:group, parent: grandparent) } + let(:group) { create(:group, parent: parent, namespace_settings: settings) } + + context 'when the groups setting is changed' do + it 'returns false when the attribute is false' do + group.update_attribute(:emails_enabled, false) + + expect(group.emails_enabled?).to be_falsey + end + end + + context 'when a group has a parent' do + it 'returns true when no parent has disabled emails' do + expect(group.emails_enabled?).to be_truthy + end + + context 'when ancestor emails are disabled' do + it 'returns false' do + grandparent.update_attribute(:emails_enabled, false) + + expect(group.emails_enabled?).to be_falsey + end + end + end + end + context 'when a group has parent groups' do let(:grandparent) { create(:group, namespace_settings: settings) } let(:parent) { create(:group, parent: grandparent) } diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 55f08a310dc952..2a1415d24cbac7 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -1957,64 +1957,18 @@ end describe '#emails_disabled?' do - context 'when not a subgroup' do - let(:group) { create(:group) } - - it 'returns false' do - group.update_attribute(:emails_enabled, true) + let(:settings) { create(:namespace_settings, emails_enabled: true) } + let(:group) { create(:group, namespace_settings: settings) } + context 'is the opposite of emails_enabled setting' do + it 'returns false when emails are enabled' do expect(group.emails_disabled?).to be_falsey end - it 'returns true' do + it 'returns false when emails are enabled' do group.update_attribute(:emails_enabled, false) - expect(group.emails_disabled?).to be_truthy end - - it 'does not query the db when there is no parent group' do - group.update_attribute(:emails_enabled, false) - - expect { group.emails_disabled? }.not_to exceed_query_limit(0) - end - end - - context 'when a subgroup' do - let(:grandparent) { create(:group) } - let(:parent) { create(:group, parent: grandparent) } - let(:group) { create(:group, parent: parent) } - - it 'returns false' do - expect(group.emails_disabled?).to be_falsey - end - - context 'when ancestor emails are disabled' do - it 'returns true' do - grandparent.update_attribute(:emails_disabled, true) - - expect(group.emails_disabled?).to be_truthy - end - end - end - end - - describe '#emails_enabled?' do - context 'without a persisted namespace_setting object' do - let(:group_settings) { create(:namespace_settings) } - let(:group) { build(:group, emails_disabled: false, namespace_settings: group_settings) } - - it "is the opposite of emails_disabled" do - expect(group.emails_enabled?).to be_truthy - end - end - - context 'with a persisted namespace_setting object' do - let(:namespace_settings) { create(:namespace_settings, emails_enabled: true) } - let(:group) { build(:group, emails_disabled: false, namespace_settings: namespace_settings) } - - it "is the opposite of emails_disabled" do - expect(group.emails_enabled?).to be_truthy - end end end diff --git a/spec/models/project_setting_spec.rb b/spec/models/project_setting_spec.rb index 8ad232b7e0c58c..1344032840be3d 100644 --- a/spec/models/project_setting_spec.rb +++ b/spec/models/project_setting_spec.rb @@ -206,7 +206,7 @@ def valid_target_platform_combinations context 'when emails have been disabled in parent group' do it 'returns false' do - group.update_attribute(:emails_disabled, true) + group.update_attribute(:emails_enabled, false) expect(project.emails_enabled?).to be_falsey end diff --git a/spec/presenters/issue_presenter_spec.rb b/spec/presenters/issue_presenter_spec.rb index 6c971a55e7455a..9ca0ab954b8ea3 100644 --- a/spec/presenters/issue_presenter_spec.rb +++ b/spec/presenters/issue_presenter_spec.rb @@ -105,6 +105,38 @@ end end + describe '#parent_emails_enabled?' do + subject { presenter.parent_emails_enabled? } + + it 'returns false when emails notifications is enabled for project' do + is_expected.to be(true) + end + + context 'when emails notifications is enabled for project' do + before do + allow(project).to receive(:emails_enabled?).and_return(false) + end + + it { is_expected.to be(false) } + end + + context 'for group-level issue' do + let(:presented_issue) { create(:issue, :group_level, namespace: group) } + + it 'returns false when email notifications are enabled for group' do + is_expected.to be(true) + end + + context 'when email notifications are enabled for group' do + before do + allow(group).to receive(:emails_enabled?).and_return(false) + end + + it { is_expected.to be(false) } + end + end + end + describe '#service_desk_reply_to' do context 'when issue is not a service desk issue' do subject { presenter.service_desk_reply_to } diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 7ac3382672dde2..ec9e9d6f643ba9 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -374,19 +374,23 @@ end end - context 'when updating #emails_disabled' do - let(:service) { described_class.new(internal_group, user, emails_disabled: true) } + context 'when updating #emails_enabled' do + let(:service) { described_class.new(internal_group, user, emails_enabled: false) } + + before do + internal_group.namespace_settings.update!({ emails_enabled: true }) + end it 'updates the attribute' do internal_group.add_member(user, Gitlab::Access::OWNER) - expect { service.execute }.to change { internal_group.emails_disabled }.to(true) + expect { service.execute }.to change { internal_group.emails_enabled }.to(false) end it 'does not update when not group owner' do internal_group.add_member(user, Gitlab::Access::MAINTAINER) - expect { service.execute }.not_to change { internal_group.emails_disabled } + expect { service.execute }.not_to change { internal_group.emails_enabled } end end diff --git a/spec/support/shared_examples/services/notification_service_shared_examples.rb b/spec/support/shared_examples/services/notification_service_shared_examples.rb index c53872ca4bc402..44076d144a9cd1 100644 --- a/spec/support/shared_examples/services/notification_service_shared_examples.rb +++ b/spec/support/shared_examples/services/notification_service_shared_examples.rb @@ -49,7 +49,7 @@ end it 'sends no emails with group emails disabled' do - target_group.update_attribute(:emails_disabled, true) + target_group.update_attribute(:emails_enabled, false) notification_trigger @@ -57,7 +57,7 @@ end it 'sends emails to someone' do - target_group.update_attribute(:emails_disabled, false) + target_group.update_attribute(:emails_enabled, true) notification_trigger -- GitLab From 54db4e6501a291c76912e2012c8814c24f8af1ac Mon Sep 17 00:00:00 2001 From: Terri Chu Date: Wed, 14 Feb 2024 13:48:42 +0000 Subject: [PATCH 2/7] Apply 1 suggestion(s) to 1 file(s) --- app/models/namespace.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/namespace.rb b/app/models/namespace.rb index c0586591682f6d..4c2b799cba06a2 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -25,7 +25,7 @@ class Namespace < ApplicationRecord cross_database_ignore_tables %w[routes redirect_routes], url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/424277' - ignore_column :emails_disabled, remove_with: '16.8', remove_after: '2024-01-18' + ignore_column :emails_disabled, remove_with: '17.0', remove_after: '2024-04-24' # Tells ActiveRecord not to store the full class name, in order to save some space # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/69794 -- GitLab From e1ff5af52f9bec43156c35818ad51f36d276856a Mon Sep 17 00:00:00 2001 From: Joseph Snyder Date: Fri, 16 Feb 2024 09:31:11 -0500 Subject: [PATCH 3/7] Additional review fixes and updates - Switch to useing projectEmailsEnabled - Make default value of board value for emailsEnabled be correct - Fix testing emails_enabled block in namespace settings spec - Change group settings spec to follow default values (True at first, and uncheck to disabled) - Remove variables with emailsDisabled that are no longer used --- app/assets/javascripts/boards/constants.js | 3 +- app/assets/javascripts/boards/index.js | 2 +- .../subscriptions/subscriptions.vue | 12 +-- .../sidebar/stores/sidebar_store.js | 1 - spec/features/groups/group_settings_spec.rb | 14 +-- .../subscriptions/subscriptions_spec.js | 2 +- spec/models/namespace_setting_spec.rb | 97 +++++++------------ 7 files changed, 49 insertions(+), 82 deletions(-) diff --git a/app/assets/javascripts/boards/constants.js b/app/assets/javascripts/boards/constants.js index c12e514f27feeb..5fdac79e5cd860 100644 --- a/app/assets/javascripts/boards/constants.js +++ b/app/assets/javascripts/boards/constants.js @@ -123,8 +123,7 @@ export const listIssuablesQueries = { confidential: false, closedAt: null, dueDate: null, - emailsDisabled: false, - emailsEnabled: false, + emailsEnabled: true, hidden: false, humanTimeEstimate: null, humanTotalTimeSpent: null, diff --git a/app/assets/javascripts/boards/index.js b/app/assets/javascripts/boards/index.js index 02c57fe1d2c1e0..6f9a587b0c9e1f 100644 --- a/app/assets/javascripts/boards/index.js +++ b/app/assets/javascripts/boards/index.js @@ -55,7 +55,7 @@ function mountBoardApp(el) { releasesFetchPath: el.dataset.releasesFetchPath, timeTrackingLimitToHours: parseBoolean(el.dataset.timeTrackingLimitToHours), issuableType: TYPE_ISSUE, - emailsDisabled: !parseBoolean(el.dataset.emailsEnabled), + emailsEnabled: parseBoolean(el.dataset.emailsEnabled), hasMissingBoards: parseBoolean(el.dataset.hasMissingBoards), weights: el.dataset.weights ? JSON.parse(el.dataset.weights) : [], isIssueBoard: true, diff --git a/app/assets/javascripts/sidebar/components/subscriptions/subscriptions.vue b/app/assets/javascripts/sidebar/components/subscriptions/subscriptions.vue index b13f594603b797..3245f49a298eef 100644 --- a/app/assets/javascripts/sidebar/components/subscriptions/subscriptions.vue +++ b/app/assets/javascripts/sidebar/components/subscriptions/subscriptions.vue @@ -25,10 +25,10 @@ export default { required: false, default: false, }, - projectEmailsDisabled: { + projectEmailsEnabled: { type: Boolean, required: false, - default: false, + default: true, }, subscribeDisabledDescription: { type: String, @@ -57,19 +57,19 @@ export default { return this.subscribed === null; }, notificationIcon() { - if (this.projectEmailsDisabled) { + if (!this.projectEmailsEnabled) { return ICON_OFF; } return this.subscribed ? ICON_ON : ICON_OFF; }, notificationTooltip() { - if (this.projectEmailsDisabled) { + if (!this.projectEmailsEnabled) { return this.subscribeDisabledDescription; } return this.subscribed ? LABEL_ON : LABEL_OFF; }, notificationText() { - if (this.projectEmailsDisabled) { + if (!this.projectEmailsEnabled) { return this.subscribeDisabledDescription; } return __('Notifications'); @@ -118,7 +118,7 @@ export default { {{ notificationText }} { beforeEach(() => { mountComponent({ subscribed: false, - projectEmailsDisabled: true, + projectEmailsEnabled: false, subscribeDisabledDescription, }); }); diff --git a/spec/models/namespace_setting_spec.rb b/spec/models/namespace_setting_spec.rb index 5bf79f691732ba..cf99f6db0938d0 100644 --- a/spec/models/namespace_setting_spec.rb +++ b/spec/models/namespace_setting_spec.rb @@ -184,36 +184,6 @@ end end - describe '#emails_enabled?' do - context 'when a group has no parent' - let(:settings) { create(:namespace_settings, emails_enabled: true) } - let(:grandparent) { create(:group) } - let(:parent) { create(:group, parent: grandparent) } - let(:group) { create(:group, parent: parent, namespace_settings: settings) } - - context 'when the groups setting is changed' do - it 'returns false when the attribute is false' do - group.update_attribute(:emails_enabled, false) - - expect(group.emails_enabled?).to be_falsey - end - end - - context 'when a group has a parent' do - it 'returns true when no parent has disabled emails' do - expect(group.emails_enabled?).to be_truthy - end - - context 'when ancestor emails are disabled' do - it 'returns false' do - grandparent.update_attribute(:emails_enabled, false) - - expect(group.emails_enabled?).to be_falsey - end - end - end - end - context 'when a group has parent groups' do let(:grandparent) { create(:group, namespace_settings: settings) } let(:parent) { create(:group, parent: grandparent) } @@ -238,52 +208,55 @@ end describe '#emails_enabled?' do - context 'when a group has no parent' - let(:settings) { create(:namespace_settings, emails_enabled: true) } - let(:grandparent) { create(:group) } - let(:parent) { create(:group, parent: grandparent) } - let(:group) { create(:group, parent: parent, namespace_settings: settings) } + context 'when a group has no parent' do + let(:settings) { create(:namespace_settings, emails_enabled: true) } + let(:group) { create(:group, namespace_settings: settings) } - context 'when the groups setting is changed' do - it 'returns false when the attribute is false' do - group.update_attribute(:emails_enabled, false) + context 'when the groups setting is changed' do + it 'returns false when the attribute is false' do + group.update_attribute(:emails_enabled, false) - expect(group.emails_enabled?).to be_falsey + expect(group.emails_enabled?).to be_falsey + end end - end - context 'when a group has a parent' do - it 'returns true when no parent has disabled emails' do - expect(group.emails_enabled?).to be_truthy - end + context 'when a group has a parent' do + let(:grandparent) { create(:group, namespace_settings: settings) } + let(:parent) { create(:group, parent: grandparent) } + let(:group) { create(:group, parent: parent) } - context 'when ancestor emails are disabled' do - it 'returns false' do - grandparent.update_attribute(:emails_enabled, false) + it 'returns true when no parent has disabled emails' do + expect(group.emails_enabled?).to be_truthy + end - expect(group.emails_enabled?).to be_falsey + context 'when ancestor emails are disabled' do + it 'returns false' do + grandparent.update_attribute(:emails_enabled, false) + + expect(group.emails_enabled?).to be_falsey + end end end - end - context 'when a group has parent groups' do - let(:grandparent) { create(:group, namespace_settings: settings) } - let(:parent) { create(:group, parent: grandparent) } - let!(:group) { create(:group, parent: parent) } + context 'when a group has parent groups' do + let(:grandparent) { create(:group, namespace_settings: settings) } + let(:parent) { create(:group, parent: grandparent) } + let!(:group) { create(:group, parent: parent) } - context "when a parent group has emails disabled" do - let(:settings) { create(:namespace_settings, emails_enabled: false) } + context "when a parent group has emails disabled" do + let(:settings) { create(:namespace_settings, emails_enabled: false) } - it 'returns false' do - expect(group.emails_enabled?).to be_falsey + it 'returns false' do + expect(group.emails_enabled?).to be_falsey + end end - end - context 'when all parent groups have emails enabled' do - let(:settings) { create(:namespace_settings, emails_enabled: true) } + context 'when all parent groups have emails enabled' do + let(:settings) { create(:namespace_settings, emails_enabled: true) } - it 'returns true' do - expect(group.emails_enabled?).to be_truthy + it 'returns true' do + expect(group.emails_enabled?).to be_truthy + end end end end -- GitLab From de95f79d1a36e7d87ef03fd051c018dbd4ed8c59 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Mon, 19 Feb 2024 13:36:07 +0000 Subject: [PATCH 4/7] Apply suggestions from review - Update descriptions of text - Remove creations of namespace setting objects - Flatten test nesting levels - Fix mis-aligned end statements --- app/services/groups/update_service.rb | 5 +- spec/models/namespace_setting_spec.rb | 61 ++++++++++--------------- spec/models/namespace_spec.rb | 18 ++++---- spec/presenters/issue_presenter_spec.rb | 10 ++-- 4 files changed, 38 insertions(+), 56 deletions(-) diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index 26fdb480376b09..61712279a9d2d7 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -135,10 +135,7 @@ def reject_parent_id! # overridden in EE def remove_unallowed_params - unless can?(current_user, :set_emails_disabled, group) - params.delete(:emails_disabled) - params.delete(:emails_enabled) - end + params.delete(:emails_enabled) unless can?(current_user, :set_emails_disabled, group) unless can?(current_user, :update_default_branch_protection, group) params.delete(:default_branch_protection) diff --git a/spec/models/namespace_setting_spec.rb b/spec/models/namespace_setting_spec.rb index cf99f6db0938d0..b8c20c4d591ef7 100644 --- a/spec/models/namespace_setting_spec.rb +++ b/spec/models/namespace_setting_spec.rb @@ -208,55 +208,42 @@ end describe '#emails_enabled?' do - context 'when a group has no parent' do - let(:settings) { create(:namespace_settings, emails_enabled: true) } - let(:group) { create(:group, namespace_settings: settings) } + let_it_be_with_refind(:group) { create(:group) } - context 'when the groups setting is changed' do - it 'returns false when the attribute is false' do - group.update_attribute(:emails_enabled, false) + it 'returns true when the attribute is true' do + group.emails_enabled = true - expect(group.emails_enabled?).to be_falsey - end - end + expect(group.emails_enabled?).to be_truthy + end - context 'when a group has a parent' do - let(:grandparent) { create(:group, namespace_settings: settings) } - let(:parent) { create(:group, parent: grandparent) } - let(:group) { create(:group, parent: parent) } + it 'returns false when the attribute is false' do + group.emails_enabled = false - it 'returns true when no parent has disabled emails' do - expect(group.emails_enabled?).to be_truthy - end + expect(group.emails_enabled?).to be_falsey + end - context 'when ancestor emails are disabled' do - it 'returns false' do - grandparent.update_attribute(:emails_enabled, false) + context 'when a group has parent groups' do + let_it_be(:grandparent) { create(:group) } + let_it_be(:parent) { create(:group, parent: grandparent) } + let_it_be_with_refind(:group) { create(:group, parent: parent) } - expect(group.emails_enabled?).to be_falsey - end - end + it 'returns true when no parent has disabled emails' do + expect(group.emails_enabled?).to be_truthy end - context 'when a group has parent groups' do - let(:grandparent) { create(:group, namespace_settings: settings) } - let(:parent) { create(:group, parent: grandparent) } - let!(:group) { create(:group, parent: parent) } - - context "when a parent group has emails disabled" do - let(:settings) { create(:namespace_settings, emails_enabled: false) } + context 'when grandparent emails are disabled' do + it 'returns false' do + grandparent.update!(emails_enabled: false) - it 'returns false' do - expect(group.emails_enabled?).to be_falsey - end + expect(group.emails_enabled?).to be_falsey end + end - context 'when all parent groups have emails enabled' do - let(:settings) { create(:namespace_settings, emails_enabled: true) } + context "when parent emails are disabled" do + it 'returns false' do + parent.update!(emails_enabled: false) - it 'returns true' do - expect(group.emails_enabled?).to be_truthy - end + expect(group.emails_enabled?).to be_falsey end end end diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 2a1415d24cbac7..8cf924b0eec60b 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -1957,18 +1957,16 @@ end describe '#emails_disabled?' do - let(:settings) { create(:namespace_settings, emails_enabled: true) } - let(:group) { create(:group, namespace_settings: settings) } + let_it_be_with_refind(:group) { create(:group) } - context 'is the opposite of emails_enabled setting' do - it 'returns false when emails are enabled' do - expect(group.emails_disabled?).to be_falsey - end + it 'returns false when emails are enabled' do + expect(group.emails_disabled?).to be_falsey + end - it 'returns false when emails are enabled' do - group.update_attribute(:emails_enabled, false) - expect(group.emails_disabled?).to be_truthy - end + it 'returns true when emails are disabled' do + group.emails_enabled = false + + expect(group.emails_disabled?).to be_truthy end end diff --git a/spec/presenters/issue_presenter_spec.rb b/spec/presenters/issue_presenter_spec.rb index 9ca0ab954b8ea3..3391ec6d8ca60b 100644 --- a/spec/presenters/issue_presenter_spec.rb +++ b/spec/presenters/issue_presenter_spec.rb @@ -108,11 +108,11 @@ describe '#parent_emails_enabled?' do subject { presenter.parent_emails_enabled? } - it 'returns false when emails notifications is enabled for project' do + it 'returns true when email notifications are enabled for the project' do is_expected.to be(true) end - context 'when emails notifications is enabled for project' do + context 'when email notifications are disabled for the project' do before do allow(project).to receive(:emails_enabled?).and_return(false) end @@ -120,14 +120,14 @@ it { is_expected.to be(false) } end - context 'for group-level issue' do + context 'when issue is group-level' do let(:presented_issue) { create(:issue, :group_level, namespace: group) } - it 'returns false when email notifications are enabled for group' do + it 'returns true when email notifications are enabled for the group' do is_expected.to be(true) end - context 'when email notifications are enabled for group' do + context 'when email notifications are disabled for the group' do before do allow(group).to receive(:emails_enabled?).and_return(false) end -- GitLab From aac0703aea03d701088c02c521d1f91991e7754c Mon Sep 17 00:00:00 2001 From: Joseph Snyder Date: Tue, 20 Feb 2024 16:16:53 -0500 Subject: [PATCH 5/7] Remove unneeded calls creating namespace_settings Rely on the generation of the namespace_setting that occurs in the factory if necessary. --- ...user_group_notification_settings_finder_spec.rb | 6 ++---- spec/models/group_spec.rb | 14 +++----------- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/spec/finders/user_group_notification_settings_finder_spec.rb b/spec/finders/user_group_notification_settings_finder_spec.rb index 7c4a1752dd86b5..83d0d343c04450 100644 --- a/spec/finders/user_group_notification_settings_finder_spec.rb +++ b/spec/finders/user_group_notification_settings_finder_spec.rb @@ -134,12 +134,10 @@ def attributes(&proc) let_it_be(:sub_sub_group) { create(:group, parent: sub_group) } let_it_be(:another_root_group) { create(:group) } - let_it_be(:sub_group_with_emails_disabled_settings) { create(:namespace_settings) } - let_it_be(:sub_group_with_emails_disabled) { create(:group, namespace_settings: sub_group_with_emails_disabled_settings, emails_enabled: false, parent: another_root_group) } + let_it_be(:sub_group_with_emails_disabled) { create(:group, emails_enabled: false, parent: another_root_group) } let_it_be(:another_sub_sub_group) { create(:group, parent: sub_group_with_emails_disabled) } - let_it_be(:root_group_with_emails_disabled_settings) { create(:namespace_settings) } - let_it_be(:root_group_with_emails_disabled) { create(:group, namespace_settings: root_group_with_emails_disabled_settings, emails_enabled: false) } + let_it_be(:root_group_with_emails_disabled) { create(:group, emails_enabled: false) } let_it_be(:group) { create(:group, parent: root_group_with_emails_disabled) } let(:groups) { Group.where(id: [sub_sub_group, another_sub_sub_group, group]) } diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 347bbaa7f78386..936667779fa8ef 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -3076,10 +3076,6 @@ def define_cache_expectations(cache_key) end describe '#first_owner' do - # Namespace setting needed due to "notifiable" looking for email state - let_it_be(:group_settings) { create(:namespace_settings) } - let(:group) { build(:group, namespace_settings: group_settings) } - context 'the group has owners' do it 'is the first owner' do user_1 = create(:user) @@ -3194,17 +3190,13 @@ def define_cache_expectations(cache_key) end describe '.ids_with_disabled_email' do - let_it_be(:group_settings_1) { create(:namespace_settings, emails_enabled: false) } - let_it_be(:group_settings_2) { create(:namespace_settings, emails_enabled: true) } - let_it_be(:group_settings_other) { create(:namespace_settings, emails_enabled: true) } - - let_it_be(:parent_1) { create(:group, namespace_settings: group_settings_1) } + let_it_be(:parent_1) { create(:group, emails_enabled: false) } let_it_be(:child_1) { create(:group, parent: parent_1) } - let_it_be(:parent_2) { create(:group, namespace_settings: group_settings_2) } + let_it_be(:parent_2) { create(:group, emails_enabled: true) } let_it_be(:child_2) { create(:group, parent: parent_2) } - let_it_be(:other_group) { create(:group, namespace_settings: group_settings_other) } + let_it_be(:other_group) { create(:group, emails_enabled: true) } shared_examples 'returns namespaces with disabled email' do subject(:group_ids_where_email_is_disabled) { described_class.ids_with_disabled_email([child_1, child_2, other_group]) } -- GitLab From 3787230485d665243d67a1695e037248414ef355 Mon Sep 17 00:00:00 2001 From: Joseph Snyder Date: Thu, 7 Mar 2024 09:08:54 -0500 Subject: [PATCH 6/7] Remove unneeded addition to mock data Since the GraphQL isn't deprecating the emailsDisabled attribute, we shouldn't need to pass emailsEnabled. --- spec/frontend/sidebar/mock_data.js | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/spec/frontend/sidebar/mock_data.js b/spec/frontend/sidebar/mock_data.js index c326ae4fccc9e0..9d8392ad5f0db4 100644 --- a/spec/frontend/sidebar/mock_data.js +++ b/spec/frontend/sidebar/mock_data.js @@ -317,11 +317,7 @@ export const issueReferenceResponse = (reference) => ({ }, }); -export const issueSubscriptionsResponse = ( - subscribed = false, - emailsDisabled = false, - emailsEnabled = true, -) => ({ +export const issueSubscriptionsResponse = (subscribed = false, emailsDisabled = false) => ({ data: { workspace: { id: '1', @@ -331,7 +327,6 @@ export const issueSubscriptionsResponse = ( id: 'gid://gitlab/Issue/4', subscribed, emailsDisabled, - emailsEnabled, }, }, }, -- GitLab From 92b672f4dabdf3c4bd8d928036ec27e4c371c06e Mon Sep 17 00:00:00 2001 From: Joseph Snyder Date: Thu, 28 Mar 2024 11:39:24 -0400 Subject: [PATCH 7/7] Remove unneeded test updates Remove places where the namespace_settings objects are not required to be created for the test to pass or be altered. Add some additional testing when necessary. --- spec/models/group_spec.rb | 16 ++++++++++------ spec/services/groups/update_service_spec.rb | 4 ---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 936667779fa8ef..d23b603c5f1fcf 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -3092,8 +3092,7 @@ def define_cache_expectations(cache_key) end context 'the group has a parent' do - let_it_be(:parent_settings) { create(:namespace_settings) } - let(:parent) { build(:group, namespace_settings: parent_settings) } + let(:parent) { build(:group) } before do group.parent = parent @@ -3190,21 +3189,26 @@ def define_cache_expectations(cache_key) end describe '.ids_with_disabled_email' do - let_it_be(:parent_1) { create(:group, emails_enabled: false) } + let_it_be(:parent_1) { create(:group) } let_it_be(:child_1) { create(:group, parent: parent_1) } - let_it_be(:parent_2) { create(:group, emails_enabled: true) } + let_it_be(:parent_2) { create(:group) } let_it_be(:child_2) { create(:group, parent: parent_2) } - let_it_be(:other_group) { create(:group, emails_enabled: true) } + let_it_be(:other_group) { create(:group) } shared_examples 'returns namespaces with disabled email' do subject(:group_ids_where_email_is_disabled) { described_class.ids_with_disabled_email([child_1, child_2, other_group]) } - it do + it "when a group's parent has disabled emails" do parent_1.update_attribute(:emails_enabled, false) is_expected.to eq(Set.new([child_1.id])) end + + it "when a group itself has disabled emails" do + child_2.update_attribute(:emails_enabled, false) + is_expected.to eq(Set.new([child_2.id])) + end end it_behaves_like 'returns namespaces with disabled email' diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index ec9e9d6f643ba9..ed1395a38e9130 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -377,10 +377,6 @@ context 'when updating #emails_enabled' do let(:service) { described_class.new(internal_group, user, emails_enabled: false) } - before do - internal_group.namespace_settings.update!({ emails_enabled: true }) - end - it 'updates the attribute' do internal_group.add_member(user, Gitlab::Access::OWNER) -- GitLab