Merge branch '344553-update-registration-alert' into 'master'

Update registration enabled callout

See merge request gitlab-org/gitlab!80972

app/assets/javascripts/persistent_user_callout.js

@@ -13,23 +13,25 @@ export default class PersistentUserCallout {
     this.featureId = featureId;
     this.groupId = groupId;
     this.deferLinks = parseBoolean(deferLinks);
+    this.closeButtons = this.container.querySelectorAll('.js-close');
 
     this.init();
   }
 
   init() {
-    const closeButton = this.container.querySelector('.js-close');
     const followLink = this.container.querySelector('.js-follow-link');
 
-    if (closeButton) {
-      this.handleCloseButtonCallout(closeButton);
+    if (this.closeButtons.length) {
+      this.handleCloseButtonCallout();
     } else if (followLink) {
       this.handleFollowLinkCallout(followLink);
     }
   }
 
-  handleCloseButtonCallout(closeButton) {
-    closeButton.addEventListener('click', (event) => this.dismiss(event));
+  handleCloseButtonCallout() {
+    this.closeButtons.forEach((closeButton) => {
+      closeButton.addEventListener('click', this.dismiss);
+    });
 
     if (this.deferLinks) {
       this.container.addEventListener('click', (event) => {
@@ -47,7 +49,7 @@ export default class PersistentUserCallout {
     followLink.addEventListener('click', (event) => this.registerCalloutWithLink(event));
   }
 
-  dismiss(event, deferredLinkOptions = null) {
+  dismiss = (event, deferredLinkOptions = null) => {
     event.preventDefault();
 
     axios
@@ -57,6 +59,9 @@ export default class PersistentUserCallout {
       })
       .then(() => {
         this.container.remove();
+        this.closeButtons.forEach((closeButton) => {
+          closeButton.removeEventListener('click', this.dismiss);
+        });
 
         if (deferredLinkOptions) {
           const { href, target } = deferredLinkOptions;
@@ -70,7 +75,7 @@ export default class PersistentUserCallout {
           ),
         });
       });
-  }
+  };
 
   registerCalloutWithLink(event) {
     event.preventDefault();

app/helpers/users/callouts_helper.rb

@@ -10,6 +10,7 @@ module Users
     REGISTRATION_ENABLED_CALLOUT = 'registration_enabled_callout'
     UNFINISHED_TAG_CLEANUP_CALLOUT = 'unfinished_tag_cleanup_callout'
     SECURITY_NEWSLETTER_CALLOUT = 'security_newsletter_callout'
+    REGISTRATION_ENABLED_CALLOUT_ALLOWED_CONTROLLER_PATHS = [/^root/, /^dashboard\S*/, /^admin\S*/].freeze
 
     def show_gke_cluster_integration_callout?(project)
       active_nav_link?(controller: sidebar_operations_paths) &&
@@ -47,7 +48,8 @@ module Users
       !Gitlab.com? &&
         current_user&.admin? &&
         signup_enabled? &&
-        !user_dismissed?(REGISTRATION_ENABLED_CALLOUT)
+        !user_dismissed?(REGISTRATION_ENABLED_CALLOUT) &&
+        REGISTRATION_ENABLED_CALLOUT_ALLOWED_CONTROLLER_PATHS.any? { |path| controller.controller_path.match?(path) }
     end
 
     def dismiss_two_factor_auth_recovery_settings_check

app/views/layouts/header/_registration_enabled_callout.html.haml

 - return unless show_registration_enabled_user_callout?
 
 = render 'shared/global_alert',
-  title: _('Open registration is enabled on your instance.'),
+  title: _('Anyone can register for an account.'),
   variant: :warning,
   alert_class: 'js-registration-enabled-callout',
   alert_data: { feature_id: Users::CalloutsHelper::REGISTRATION_ENABLED_CALLOUT, dismiss_endpoint: callouts_path },
   close_button_data: { testid: 'close-registration-enabled-callout' } do
   .gl-alert-body
-    = html_escape(_('%{anchorOpen}Learn more%{anchorClose} about how you can customize / disable registration on your instance.')) % { anchorOpen: "<a href=\"#{help_page_path('user/admin_area/settings/sign_up_restrictions')}\" class=\"gl-link\">".html_safe, anchorClose: '</a>'.html_safe }
+    = _('Only allow anyone to register for accounts on GitLab instances that you intend to be used by anyone. Allowing anyone to register makes GitLab instances more vulnerable.')
   .gl-alert-actions
     = link_to general_admin_application_settings_path(anchor: 'js-signup-settings'), class: 'btn gl-alert-action btn-confirm btn-md gl-button' do
       %span.gl-button-text
-        = _('View setting')
+        = _('Turn off')
+    %button.btn.gl-alert-action.btn-default.btn-md.gl-button.js-close
+      %span.gl-button-text
+        = _('Acknowledge')

locale/gitlab.pot

@@ -462,9 +462,6 @@ msgstr ""
 msgid "%{address} is an invalid IP address range"
 msgstr ""
 
-msgid "%{anchorOpen}Learn more%{anchorClose} about how you can customize / disable registration on your instance."
-msgstr ""
-
 msgid "%{author_link} cloned %{original_issue} to %{new_issue}."
 msgstr ""
 
@@ -1986,6 +1983,9 @@ msgstr ""
 msgid "AccountValidation|you may %{unsubscribe_link} at any time."
 msgstr ""
 
+msgid "Acknowledge"
+msgstr ""
+
 msgid "Action"
 msgstr ""
 
@@ -4208,6 +4208,9 @@ msgstr ""
 msgid "Any namespace"
 msgstr ""
 
+msgid "Anyone can register for an account."
+msgstr ""
+
 msgid "App ID"
 msgstr ""
 
@@ -25632,6 +25635,9 @@ msgstr ""
 msgid "Only admins can delete project"
 msgstr ""
 
+msgid "Only allow anyone to register for accounts on GitLab instances that you intend to be used by anyone. Allowing anyone to register makes GitLab instances more vulnerable."
+msgstr ""
+
 msgid "Only effective when remote storage is enabled. Set to 0 for no size limit."
 msgstr ""
 
@@ -25704,9 +25710,6 @@ msgstr ""
 msgid "Open raw"
 msgstr ""
 
-msgid "Open registration is enabled on your instance."
-msgstr ""
-
 msgid "Open sidebar"
 msgstr ""
 
@@ -40609,9 +40612,6 @@ msgstr ""
 msgid "View seat usage"
 msgstr ""
 
-msgid "View setting"
-msgstr ""
-
 msgid "View supported languages and frameworks"
 msgstr ""
 

spec/features/callouts/registration_enabled_spec.rb

@@ -5,6 +5,8 @@ require 'spec_helper'
 RSpec.describe 'Registration enabled callout' do
   let_it_be(:admin) { create(:admin) }
   let_it_be(:non_admin) { create(:user) }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:callout_title) { _('Anyone can register for an account.') }
 
   context 'when "Sign-up enabled" setting is `true`' do
     before do
@@ -14,23 +16,42 @@ RSpec.describe 'Registration enabled callout' do
     context 'when an admin is logged in' do
       before do
         sign_in(admin)
+      end
+
+      it 'displays callout on admin and dashboard pages and root page' do
+        visit root_path
+
+        expect(page).to have_content callout_title
+        expect(page).to have_link _('Turn off'), href: general_admin_application_settings_path(anchor: 'js-signup-settings')
+
         visit root_dashboard_path
+
+        expect(page).to have_content callout_title
+
+        visit admin_root_path
+
+        expect(page).to have_content callout_title
       end
 
-      it 'displays callout' do
-        expect(page).to have_content 'Open registration is enabled on your instance.'
-        expect(page).to have_link 'View setting', href: general_admin_application_settings_path(anchor: 'js-signup-settings')
+      it 'does not display callout on pages other than root, admin, or dashboard' do
+        visit project_issues_path(project)
+
+        expect(page).not_to have_content callout_title
       end
 
       context 'when callout is dismissed', :js do
         before do
+          visit admin_root_path
+
           find('[data-testid="close-registration-enabled-callout"]').click
 
+          wait_for_requests
+
           visit root_dashboard_path
         end
 
         it 'does not display callout' do
-          expect(page).not_to have_content 'Open registration is enabled on your instance.'
+          expect(page).not_to have_content callout_title
         end
       end
     end
@@ -42,7 +63,7 @@ RSpec.describe 'Registration enabled callout' do
       end
 
       it 'does not display callout' do
-        expect(page).not_to have_content 'Open registration is enabled on your instance.'
+        expect(page).not_to have_content callout_title
       end
     end
   end

spec/frontend/persistent_user_callout_spec.js

@@ -21,7 +21,8 @@ describe('PersistentUserCallout', () => {
         data-feature-id="${featureName}"
         data-group-id="${groupId}"
       >
-        <button type="button" class="js-close"></button>
+        <button type="button" class="js-close js-close-primary"></button>
+        <button type="button" class="js-close js-close-secondary"></button>
       </div>
     `;
 
@@ -64,14 +65,15 @@ describe('PersistentUserCallout', () => {
   }
 
   describe('dismiss', () => {
-    let button;
+    const buttons = {};
     let mockAxios;
     let persistentUserCallout;
 
     beforeEach(() => {
       const fixture = createFixture();
       const container = fixture.querySelector('.container');
-      button = fixture.querySelector('.js-close');
+      buttons.primary = fixture.querySelector('.js-close-primary');
+      buttons.secondary = fixture.querySelector('.js-close-secondary');
       mockAxios = new MockAdapter(axios);
       persistentUserCallout = new PersistentUserCallout(container);
       jest.spyOn(persistentUserCallout.container, 'remove').mockImplementation(() => {});
@@ -81,29 +83,33 @@ describe('PersistentUserCallout', () => {
       mockAxios.restore();
     });
 
-    it('POSTs endpoint and removes container when clicking close', () => {
+    it.each`
+      button
+      ${'primary'}
+      ${'secondary'}
+    `('POSTs endpoint and removes container when clicking $button close', async ({ button }) => {
       mockAxios.onPost(dismissEndpoint).replyOnce(200);
 
-      button.click();
+      buttons[button].click();
 
-      return waitForPromises().then(() => {
-        expect(persistentUserCallout.container.remove).toHaveBeenCalled();
-        expect(mockAxios.history.post[0].data).toBe(
-          JSON.stringify({ feature_name: featureName, group_id: groupId }),
-        );
-      });
+      await waitForPromises();
+
+      expect(persistentUserCallout.container.remove).toHaveBeenCalled();
+      expect(mockAxios.history.post[0].data).toBe(
+        JSON.stringify({ feature_name: featureName, group_id: groupId }),
+      );
     });
 
-    it('invokes Flash when the dismiss request fails', () => {
+    it('invokes Flash when the dismiss request fails', async () => {
       mockAxios.onPost(dismissEndpoint).replyOnce(500);
 
-      button.click();
+      buttons.primary.click();
 
-      return waitForPromises().then(() => {
-        expect(persistentUserCallout.container.remove).not.toHaveBeenCalled();
-        expect(createFlash).toHaveBeenCalledWith({
-          message: 'An error occurred while dismissing the alert. Refresh the page and try again.',
-        });
+      await waitForPromises();
+
+      expect(persistentUserCallout.container.remove).not.toHaveBeenCalled();
+      expect(createFlash).toHaveBeenCalledWith({
+        message: 'An error occurred while dismissing the alert. Refresh the page and try again.',
       });
     });
   });
@@ -132,37 +138,37 @@ describe('PersistentUserCallout', () => {
       mockAxios.restore();
     });
 
-    it('defers loading of a link until callout is dismissed', () => {
+    it('defers loading of a link until callout is dismissed', async () => {
       const { href, target } = deferredLink;
       mockAxios.onPost(dismissEndpoint).replyOnce(200);
 
       deferredLink.click();
 
-      return waitForPromises().then(() => {
-        expect(windowSpy).toHaveBeenCalledWith(href, target);
-        expect(persistentUserCallout.container.remove).toHaveBeenCalled();
-        expect(mockAxios.history.post[0].data).toBe(JSON.stringify({ feature_name: featureName }));
-      });
+      await waitForPromises();
+
+      expect(windowSpy).toHaveBeenCalledWith(href, target);
+      expect(persistentUserCallout.container.remove).toHaveBeenCalled();
+      expect(mockAxios.history.post[0].data).toBe(JSON.stringify({ feature_name: featureName }));
     });
 
-    it('does not dismiss callout on non-deferred links', () => {
+    it('does not dismiss callout on non-deferred links', async () => {
       normalLink.click();
 
-      return waitForPromises().then(() => {
-        expect(windowSpy).not.toHaveBeenCalled();
-        expect(persistentUserCallout.container.remove).not.toHaveBeenCalled();
-      });
+      await waitForPromises();
+
+      expect(windowSpy).not.toHaveBeenCalled();
+      expect(persistentUserCallout.container.remove).not.toHaveBeenCalled();
     });
 
-    it('does not follow link when notification is closed', () => {
+    it('does not follow link when notification is closed', async () => {
       mockAxios.onPost(dismissEndpoint).replyOnce(200);
 
       button.click();
 
-      return waitForPromises().then(() => {
-        expect(windowSpy).not.toHaveBeenCalled();
-        expect(persistentUserCallout.container.remove).toHaveBeenCalled();
-      });
+      await waitForPromises();
+
+      expect(windowSpy).not.toHaveBeenCalled();
+      expect(persistentUserCallout.container.remove).toHaveBeenCalled();
     });
   });
 
@@ -187,30 +193,30 @@ describe('PersistentUserCallout', () => {
       mockAxios.restore();
     });
 
-    it('uses a link to trigger callout and defers following until callout is finished', () => {
+    it('uses a link to trigger callout and defers following until callout is finished', async () => {
       const { href } = link;
       mockAxios.onPost(dismissEndpoint).replyOnce(200);
 
       link.click();
 
-      return waitForPromises().then(() => {
-        expect(window.location.assign).toBeCalledWith(href);
-        expect(persistentUserCallout.container.remove).not.toHaveBeenCalled();
-        expect(mockAxios.history.post[0].data).toBe(JSON.stringify({ feature_name: featureName }));
-      });
+      await waitForPromises();
+
+      expect(window.location.assign).toBeCalledWith(href);
+      expect(persistentUserCallout.container.remove).not.toHaveBeenCalled();
+      expect(mockAxios.history.post[0].data).toBe(JSON.stringify({ feature_name: featureName }));
     });
 
-    it('invokes Flash when the dismiss request fails', () => {
+    it('invokes Flash when the dismiss request fails', async () => {
       mockAxios.onPost(dismissEndpoint).replyOnce(500);
 
       link.click();
 
-      return waitForPromises().then(() => {
-        expect(window.location.assign).not.toHaveBeenCalled();
-        expect(createFlash).toHaveBeenCalledWith({
-          message:
-            'An error occurred while acknowledging the notification. Refresh the page and try again.',
-        });
+      await waitForPromises();
+
+      expect(window.location.assign).not.toHaveBeenCalled();
+      expect(createFlash).toHaveBeenCalledWith({
+        message:
+          'An error occurred while acknowledging the notification. Refresh the page and try again.',
       });
     });
   });

spec/helpers/users/callouts_helper_spec.rb

@@ -103,6 +103,7 @@ RSpec.describe Users::CalloutsHelper do
         allow(helper).to receive(:current_user).and_return(admin)
         stub_application_setting(signup_enabled: true)
         allow(helper).to receive(:user_dismissed?).with(described_class::REGISTRATION_ENABLED_CALLOUT) { false }
+        allow(helper.controller).to receive(:controller_path).and_return("admin/users")
       end
 
       it { is_expected.to be false }
@@ -114,6 +115,7 @@ RSpec.describe Users::CalloutsHelper do
         allow(helper).to receive(:current_user).and_return(user)
         stub_application_setting(signup_enabled: true)
         allow(helper).to receive(:user_dismissed?).with(described_class::REGISTRATION_ENABLED_CALLOUT) { false }
+        allow(helper.controller).to receive(:controller_path).and_return("admin/users")
       end
 
       it { is_expected.to be false }
@@ -125,6 +127,7 @@ RSpec.describe Users::CalloutsHelper do
         allow(helper).to receive(:current_user).and_return(admin)
         stub_application_setting(signup_enabled: false)
         allow(helper).to receive(:user_dismissed?).with(described_class::REGISTRATION_ENABLED_CALLOUT) { false }
+        allow(helper.controller).to receive(:controller_path).and_return("admin/users")
       end
 
       it { is_expected.to be false }
@@ -136,17 +139,31 @@ RSpec.describe Users::CalloutsHelper do
         allow(helper).to receive(:current_user).and_return(admin)
         stub_application_setting(signup_enabled: true)
         allow(helper).to receive(:user_dismissed?).with(described_class::REGISTRATION_ENABLED_CALLOUT) { true }
+        allow(helper.controller).to receive(:controller_path).and_return("admin/users")
       end
 
       it { is_expected.to be false }
     end
 
-    context 'when not gitlab.com, `current_user` is an admin, signup is enabled, and user has not dismissed callout' do
+    context 'when controller path is not allowed' do
       before do
         allow(::Gitlab).to receive(:com?).and_return(false)
         allow(helper).to receive(:current_user).and_return(admin)
         stub_application_setting(signup_enabled: true)
         allow(helper).to receive(:user_dismissed?).with(described_class::REGISTRATION_ENABLED_CALLOUT) { false }
+        allow(helper.controller).to receive(:controller_path).and_return("projects/issues")
+      end
+
+      it { is_expected.to be false }
+    end
+
+    context 'when not gitlab.com, `current_user` is an admin, signup is enabled, user has not dismissed callout, and controller path is allowed' do
+      before do
+        allow(::Gitlab).to receive(:com?).and_return(false)
+        allow(helper).to receive(:current_user).and_return(admin)
+        stub_application_setting(signup_enabled: true)
+        allow(helper).to receive(:user_dismissed?).with(described_class::REGISTRATION_ENABLED_CALLOUT) { false }
+        allow(helper.controller).to receive(:controller_path).and_return("admin/users")
       end
 
       it { is_expected.to be true }