From 3a984c8ee7c010c1984050cc6ee983b98ca6396c Mon Sep 17 00:00:00 2001 From: Arit Amana <32520970+msarit@users.noreply.github.com> Date: Wed, 13 Jan 2021 13:54:39 -0500 Subject: [PATCH] [2020 EOY Issue] Modal alert when enabling Auth Provider without keys (#12135) * Start hooking things up * further generalize adminModal, hook up relevant code * Complete implementation * remove comments * Address QA suggestions * Address QA suggestions * Use object parameter for adminModal * starting the admin cypress tests * Code review suggestions * remove incomplete test * rename data attributes to make more sense * complete the work * use destructuring * edit warning modal text --- app/assets/stylesheets/admin.scss | 8 +- app/helpers/authentication_helper.rb | 7 +- app/javascript/admin/adminModal.js | 92 +++++---- .../admin/controllers/config_controller.js | 179 ++++++++++++++---- .../_apple_auth_provider_settings.html.erb | 10 +- .../configs/_auth_provider_settings.html.erb | 14 +- app/views/admin/configs/show.html.erb | 13 +- spec/helpers/authentication_helper_spec.rb | 25 +-- 8 files changed, 213 insertions(+), 135 deletions(-) diff --git a/app/assets/stylesheets/admin.scss b/app/assets/stylesheets/admin.scss index c9cca23dc..f63f26bae 100644 --- a/app/assets/stylesheets/admin.scss +++ b/app/assets/stylesheets/admin.scss @@ -147,10 +147,6 @@ label { gap: var(--su-4); padding-top: var(--su-6); padding-bottom: var(--su-2); - - .auth-warning { - padding: var(--su-2); // Override default crayons notice padding - } } .admin-config-checkmark { @@ -180,3 +176,7 @@ label { .mod-actions__data { max-width: 300px; } + +.capitalize { + text-transform: capitalize; +} diff --git a/app/helpers/authentication_helper.rb b/app/helpers/authentication_helper.rb index e5606564b..7d244c7fc 100644 --- a/app/helpers/authentication_helper.rb +++ b/app/helpers/authentication_helper.rb @@ -23,11 +23,6 @@ module AuthenticationHelper Authentication::Providers.available.map(&:to_s) end - def provider_keys_configured?(provider) - SiteConfig.public_send("#{provider}_key").present? && - SiteConfig.public_send("#{provider}_secret").present? - end - def forem_creator_flow_enabled? FeatureFlag.enabled?(:creator_onboarding) && waiting_on_first_user? end @@ -45,7 +40,7 @@ module AuthenticationHelper invite_only_mode_or_no_enabled_auth_options ? "crayons-tooltip" : "" end - def disabled_attr_on_auth_provider_enablebtn + def disabled_attr_on_auth_provider_enable_btn invite_only_mode_or_no_enabled_auth_options ? "disabled" : "" end diff --git a/app/javascript/admin/adminModal.js b/app/javascript/admin/adminModal.js index 185480d7b..6cc097666 100644 --- a/app/javascript/admin/adminModal.js +++ b/app/javascript/admin/adminModal.js @@ -2,54 +2,62 @@ * A function to generate the HTML for a Crayons modal within the /admin/ space. * * @function adminModal - * @param {string} title The title of the modal. - * @param {string} body The modal's content. May use HTML tags for styling. - * @param {string} confirmBtnText The text for the modal's "Confirm" button. - * @param {string} confirmBtnAction The function that fires when "Confirm" button is clicked. - * @param {string} cancelBtnText The text for the modal's "Cancel" button. - * @param {string} cancelBtnAction The function that fires when "Cancel" button is clicked. - * @param {string} customAttr A custom data attribute name. Will be apprended to the "data-" part. - * @param {string} customAttrValue The value of the custom attribute "customAttr". + * @param {Object} modalProps Properties of the Modal + * @param {string} modalProps.title The title of the modal. + * @param {string} modalProps.body The modal's content. May use HTML tags for styling. + * @param {string} modalProps.leftBtnText The text for the modal's left button. + * @param {string} modalProps.leftBtnAction The function that fires when left button is clicked. + * @param {string} modalProps.rightBtnText The text for the modal's right button. + * @param {string} modalProps.rightBtnAction The function that fires when right button is clicked. + * @param {string} modalProps.leftBtnClasses Classes applied to left button. + * @param {string} modalProps.rightBtnClasses Classes applied to right button. + * @param {string} modalProps.leftCustomDataAttr A custom data attribute for the left button. + * @param {string} modalProps.rightCustomDataAttr A custom data attribute for the right button. */ -const adminModal = ( +const adminModal = function ({ title, body, - confirmBtnText, - confirmBtnAction, - cancelBtnText, - cancelBtnAction, - customAttr = null, - customAttrValue = null, -) => ` -
-
-
-

${title}

- -
-
- ${body} -
- - + +
+ ${body} +
+ + +
+
-
-
-`; + `; +}; export default adminModal; diff --git a/app/javascript/admin/controllers/config_controller.js b/app/javascript/admin/controllers/config_controller.js index 3fe4d2271..664d16fa3 100644 --- a/app/javascript/admin/controllers/config_controller.js +++ b/app/javascript/admin/controllers/config_controller.js @@ -20,6 +20,8 @@ const emailAuthModalBody = ` export default class ConfigController extends Controller { static targets = [ 'authenticationProviders', + 'authSectionForm', + 'collectiveNoun', 'configModalAnchor', 'emailAuthSettingsBtn', 'enabledIndicator', @@ -66,10 +68,23 @@ export default class ConfigController extends Controller { } } - closeAdminConfigModal() { + closeAdminModal() { + // per forem/internalEngineering#336, need to short-circuit the + // "Update Site Configuration" button submit action; chose not to + // define Target on actual "Update" button (since it's a partial). + // The Target is defined on the Authentication form, and that section's + // "Update" button is queried. + const submitBtn = this.authSectionFormTarget.querySelector( + 'input[type="submit"]', + ); + this.configModalAnchorTarget.innerHTML = ''; document.body.style.height = 'inherit'; document.body.style.overflowY = 'inherit'; + + if (submitBtn.hasAttribute('disabled')) { + submitBtn.removeAttribute('disabled'); + } } positionModalOnPage() { @@ -110,21 +125,23 @@ export default class ConfigController extends Controller { activateEmailAuthModal(event) { event.preventDefault(); - this.configModalAnchorTarget.innerHTML = adminModal( - emailAuthModalTitle, - emailAuthModalBody, - 'Confirm disable', - 'disableEmailAuthFromModal', - 'Cancel', - 'closeAdminConfigModal', - ); + this.configModalAnchorTarget.innerHTML = adminModal({ + title: emailAuthModalTitle, + body: emailAuthModalBody, + leftBtnText: 'Confirm disable', + leftBtnAction: 'disableEmailAuthFromModal', + rightBtnText: 'Cancel', + rightBtnAction: 'closeAdminModal', + leftBtnClasses: 'crayons-btn--danger', + rightBtnClasses: 'crayons-btn--secondary', + }); this.positionModalOnPage(); } disableEmailAuthFromModal(event) { event.preventDefault(); this.disableEmailAuth(event); - this.closeAdminConfigModal(event); + this.closeAdminModal(); } disableEmailAuth(event) { @@ -142,13 +159,13 @@ export default class ConfigController extends Controller { enableOrEditAuthProvider(event) { event.preventDefault(); - const provider = event.target.dataset.authProviderEnable; + const providerName = event.target.dataset.providerName; const enabledIndicator = document.getElementById( - `${provider}-enabled-indicator`, + `${providerName}-enabled-indicator`, ); document - .getElementById(`${provider}-auth-settings`) + .getElementById(`${providerName}-auth-settings`) .classList.remove('hidden'); event.target.classList.add('hidden'); @@ -161,16 +178,16 @@ export default class ConfigController extends Controller { disableAuthProvider(event) { event.preventDefault(); - const provider = event.target.dataset.authProvider; + const providerName = event.target.dataset.providerName; const enabledIndicator = document.getElementById( - `${provider}-enabled-indicator`, + `${providerName}-enabled-indicator`, ); - const authEnableButton = document.querySelector( - `[data-auth-provider-enable="${provider}"]`, + const authEnableButton = document.getElementById( + `${providerName}-auth-btn`, ); authEnableButton.setAttribute('data-enable-auth', 'false'); enabledIndicator.classList.remove('visible'); - this.listAuthToBeEnabled(event); + this.listAuthToBeEnabled(); this.hideAuthProviderSettings(event); } @@ -184,36 +201,37 @@ export default class ConfigController extends Controller { activateAuthProviderModal(event) { event.preventDefault(); - const provider = event.target.dataset.authProvider; - const official_provider = event.target.dataset.authProviderOfficial; - this.configModalAnchorTarget.innerHTML = adminModal( - this.authProviderModalTitle(official_provider), - this.authProviderModalBody(official_provider), - 'Confirm disable', - 'disableAuthProviderFromModal', - 'Cancel', - 'closeAdminConfigModal', - 'auth-provider', - provider, - ); + const providerName = event.target.dataset.providerName; + const providerOfficialName = event.target.dataset.providerOfficialName; + this.configModalAnchorTarget.innerHTML = adminModal({ + title: this.authProviderModalTitle(providerOfficialName), + body: this.authProviderModalBody(providerOfficialName), + leftBtnText: 'Confirm disable', + leftBtnAction: 'disableAuthProviderFromModal', + rightBtnText: 'Cancel', + rightBtnAction: 'closeAdminModal', + leftBtnClasses: 'crayons-btn--danger', + rightBtnClasses: 'crayons-btn--secondary', + leftCustomDataAttr: `data-provider-name=${providerName}`, + }); this.positionModalOnPage(); } disableAuthProviderFromModal(event) { event.preventDefault(); - const provider = event.target.dataset.authProvider; - const authEnableButton = document.querySelector( - `[data-auth-provider-enable="${provider}"]`, + const providerName = event.target.dataset.providerName; + const authEnableButton = document.getElementById( + `${providerName}-auth-btn`, ); const enabledIndicator = document.getElementById( - `${provider}-enabled-indicator`, + `${providerName}-enabled-indicator`, ); authEnableButton.setAttribute('data-enable-auth', 'false'); this.listAuthToBeEnabled(event); this.checkForAndGuardSoleAuthProvider(); enabledIndicator.classList.remove('visible'); this.hideAuthProviderSettings(event); - this.closeAdminConfigModal(event); + this.closeAdminModal(); } checkForAndGuardSoleAuthProvider() { @@ -237,11 +255,13 @@ export default class ConfigController extends Controller { hideAuthProviderSettings(event) { event.preventDefault(); - const provider = event.target.dataset.authProvider; + const providerName = event.target.dataset.providerName; document - .getElementById(`${provider}-auth-settings`) + .getElementById(`${providerName}-auth-settings`) .classList.add('hidden'); - document.getElementById(`${provider}-auth-btn`).classList.remove('hidden'); + document + .getElementById(`${providerName}-auth-btn`) + .classList.remove('hidden'); } listAuthToBeEnabled() { @@ -249,7 +269,7 @@ export default class ConfigController extends Controller { document .querySelectorAll('[data-enable-auth="true"]') .forEach((provider) => { - enabledProviderArray.push(provider.dataset.authProviderEnable); + enabledProviderArray.push(provider.dataset.providerName); }); document.getElementById( 'auth_providers_to_enable', @@ -265,4 +285,85 @@ export default class ConfigController extends Controller { } } // AUTH PROVIDERS FUNCTIONS END + + enabledProvidersWithMissingKeys() { + const providersWithMissingKeys = []; + document + .querySelectorAll('[data-enable-auth="true"]') + .forEach((provider) => { + const { providerName } = provider.dataset; + if ( + !document.getElementById(`site_config_${providerName}_key`).value || + !document.getElementById(`site_config_${providerName}_secret`).value + ) { + providersWithMissingKeys.push(providerName); + } + }); + + return providersWithMissingKeys; + } + + generateProvidersList(providers) { + const list = providers.reduce((html, provider) => { + return `${html}
  • ${provider}
    • `; + }, ''); + + return list; + } + + missingAuthKeysModalBody(providers) { + return ` +

    You haven't filled out all of the required fields to enable the following authentication providers:

    + +

    You may continue editing these authentication providers, or you may cancel.

    + `; + } + + submitForm() { + this.authSectionFormTarget.submit(); + } + + activateMissingKeysModal(providers) { + this.configModalAnchorTarget.innerHTML = adminModal({ + title: 'Setup not complete', + body: this.missingAuthKeysModalBody(providers), + leftBtnText: 'Continue editing', + leftBtnAction: 'closeAdminModal', + rightBtnText: 'Cancel', + rightBtnAction: 'cancelAuthProviderEnable', + rightBtnClasses: 'crayons-btn--secondary', + }); + } + + configUpdatePrecheck(event) { + if (this.enabledProvidersWithMissingKeys().length > 0) { + event.preventDefault(); + this.activateMissingKeysModal(this.enabledProvidersWithMissingKeys()); + } else { + event.target.submit(); + } + } + + cancelAuthProviderEnable() { + const providers = this.enabledProvidersWithMissingKeys(); + + providers.forEach((provider) => { + const enabledIndicator = document.getElementById( + `${provider}-enabled-indicator`, + ); + const authEnableButton = document.getElementById(`${provider}-auth-btn`); + + authEnableButton.setAttribute('data-enable-auth', 'false'); + enabledIndicator.classList.remove('visible'); + this.listAuthToBeEnabled(); + document + .getElementById(`${provider}-auth-settings`) + .classList.add('hidden'); + document + .getElementById(`${provider}-auth-btn`) + .classList.remove('hidden'); + + this.closeAdminModal(); + }); + } } diff --git a/app/views/admin/configs/_apple_auth_provider_settings.html.erb b/app/views/admin/configs/_apple_auth_provider_settings.html.erb index 49da9da65..5b033a795 100644 --- a/app/views/admin/configs/_apple_auth_provider_settings.html.erb +++ b/app/views/admin/configs/_apple_auth_provider_settings.html.erb @@ -54,21 +54,21 @@ requires a custom partial class="crayons-btn crayons-btn--danger" data-action="click->config#activateAuthProviderModal" data-tooltip="<%= tooltip_text_email_or_auth_provider_btns %>" - data-auth-provider="<%= provider.provider_name %>" - data-auth-provider-official="<%= provider.official_name %>" - <%= disabled_attr_on_auth_provider_enablebtn %>> + data-provider-name="<%= provider.provider_name %>" + data-provider-official-name="<%= provider.official_name %>" + <%= disabled_attr_on_auth_provider_enable_btn %>> Disable <% else %> diff --git a/app/views/admin/configs/_auth_provider_settings.html.erb b/app/views/admin/configs/_auth_provider_settings.html.erb index 07c39b193..fad538877 100644 --- a/app/views/admin/configs/_auth_provider_settings.html.erb +++ b/app/views/admin/configs/_auth_provider_settings.html.erb @@ -1,8 +1,4 @@
    -
    "> - Note: This authentication provider will not be enabled if the key or secret are missing. Please make sure that you enter your key and secret correctly. -
    <%= admin_config_label :"#{provider.provider_name}_key" %>

    @@ -29,21 +25,21 @@ class="crayons-btn crayons-btn--danger" data-action="click->config#activateAuthProviderModal" data-tooltip="<%= tooltip_text_email_or_auth_provider_btns %>" - data-auth-provider="<%= provider.provider_name %>" - data-auth-provider-official="<%= provider.official_name %>" - <%= disabled_attr_on_auth_provider_enablebtn %>> + data-provider-name="<%= provider.provider_name %>" + data-provider-official-name="<%= provider.official_name %>" + <%= disabled_attr_on_auth_provider_enable_btn %>> Disable <% else %> diff --git a/app/views/admin/configs/show.html.erb b/app/views/admin/configs/show.html.erb index 492e89a2b..b9fa72b3b 100644 --- a/app/views/admin/configs/show.html.erb +++ b/app/views/admin/configs/show.html.erb @@ -147,7 +147,7 @@

    <% end %> - <%= form_for(SiteConfig.new, url: admin_config_path) do |f| %> + <%= form_for(SiteConfig.new, url: admin_config_path, html: { data: { action: "submit->config#configUpdatePrecheck", "config-target": "authSectionForm" } }) do |f| %>
    <%= render partial: "admin/shared/card_header", locals: { @@ -211,7 +211,7 @@ data-button-text="<%= SiteConfig.allow_email_password_registration ? "edit" : "enable" %>" data-config-target="emailAuthSettingsBtn" data-action="click->config#enableOrEditEmailAuthSettings" - <%= disabled_attr_on_auth_provider_enablebtn %>> + <%= disabled_attr_on_auth_provider_enable_btn %>> <%= SiteConfig.allow_email_password_registration ? "Edit" : "Enable" %> @@ -308,7 +308,7 @@ readonly: true %>
    <% authentication_available_providers.each do |provider| %> - <% next if provider.provider_name == :apple && !Flipper.enabled?(:apple_auth) %> + <% next if provider.provider_name == :apple && !Flipper.enabled?(:apple_auth) %>
    <%= inline_svg_tag("#{provider.provider_name}.svg", class: "crayons-icon", aria: true, title: "#{provider.official_name} logo") %> @@ -333,9 +333,10 @@ id="<%= provider.provider_name %>-auth-btn" data-button-text="<%= authentication_enabled_providers.include?(provider) ? "edit" : "enable" %>" data-action="click->config#enableOrEditAuthProvider" - data-auth-provider-enable="<%= provider.provider_name %>" + data-provider-name="<%= provider.provider_name %>" + data-provider-official-name="<%= provider.official_name %>" data-enable-auth="<%= authentication_enabled_providers.include?(provider) ? "true" : "false" %>" - <%= disabled_attr_on_auth_provider_enablebtn %>> + <%= disabled_attr_on_auth_provider_enable_btn %>> <%= authentication_enabled_providers.include?(provider) ? "Edit" : "Enable" %>
    @@ -1334,5 +1335,5 @@ -
    +
    diff --git a/spec/helpers/authentication_helper_spec.rb b/spec/helpers/authentication_helper_spec.rb index b0b3afec0..77ac4ed1e 100644 --- a/spec/helpers/authentication_helper_spec.rb +++ b/spec/helpers/authentication_helper_spec.rb @@ -38,29 +38,6 @@ RSpec.describe AuthenticationHelper, type: :helper do end end - describe "#provider_keys_configured?(provider)" do - let(:provider) { "facebook" } - - it "returns true if provider key and secret both present" do - allow(SiteConfig).to receive(:"#{provider}_key").and_return("someKey") - allow(SiteConfig).to receive(:"#{provider}_secret").and_return("someSecret") - - expect(provider_keys_configured?(provider)).to be(true) - end - - it "returns false if either provider key or secret is missing" do - allow(SiteConfig).to receive(:"#{provider}_key").and_return("someKey") - allow(SiteConfig).to receive(:"#{provider}_secret").and_return("") - - expect(provider_keys_configured?(provider)).to be(false) - - allow(SiteConfig).to receive(:"#{provider}_key").and_return("") - allow(SiteConfig).to receive(:"#{provider}_secret").and_return("someSecret") - - expect(provider_keys_configured?(provider)).to be(false) - end - end - describe "tooltip classes, attributes and content" do context "when invite-only-mode enabled and no enabled registration options" do before do @@ -74,7 +51,7 @@ RSpec.describe AuthenticationHelper, type: :helper do end it "returns 'disabled' attribute for relevant helper" do - expect(disabled_attr_on_auth_provider_enablebtn).to eq("disabled") + expect(disabled_attr_on_auth_provider_enable_btn).to eq("disabled") end it "returns appropriate text for 'tooltip_text_email_or_auth_provider_btns' helper" do