From e1e18aa0aa6f45e0831811b88f63e481c3b4d75a Mon Sep 17 00:00:00 2001 From: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com> Date: Mon, 12 Jul 2021 14:14:43 -0600 Subject: [PATCH] [15-Minute Fix] Ensure "Locked Screen" Checkbox Conditionally Renders (#14161) * Uses Settings::UserExperience.public to conditionally render btn * Updates landing page-related specs to account for private forem * Uses Settings::UserExperience in place of ForemInstance in admin_creates_new_page_spec.rb * Reverts Settings::UserExperience.public? changes throughout codebase - Removes any changes to ForemInstance.private? - Adds a new helper method, self.invitation_only? - Updates self.private? logic * Updates landing page specs and reverts unnecessary changes - Reverts any changes that removed the check for ForemInstance.private? - Updates landingPage.spec.js to use findByRole rather than findByText * Reverts a change to admin_manages_pages_spec.rb * Updates landingPage.spec.js per PR review comment * Updates #private_forem_or_no_enabled_auth_options and spec * Uses .invite_only? in _providers_registration_form.html.erb * Adjusts authenticationSection.spec.js to test that FB is enabled * Updates necessary specs to check .invitation_only? * Consistently uses invite_only_mode_or_no_enabled_auth_options - Updates all necessary places within the codebase to use reverted method name, invite_only_mode_or_no_enabled_auth_providers - Updates #self.enabled to check ForemInstance.invitation_only? rather than ForemInstance.private? - Reverts change to Facebook assertion within authenticationSection e2e test * Removes superfluous .to from authenticationSection.spec.js --- app/helpers/authentication_helper.rb | 10 +++++----- app/models/forem_instance.rb | 6 +++++- app/services/authentication/providers.rb | 2 +- .../settings/forms/_authentication.html.erb | 4 ++-- .../registrations/_registration_form.html.erb | 2 +- .../_providers_registration_form.html.erb | 2 +- .../config/authenticationSection.spec.js | 17 ++++++++++------ .../adminFlows/pages/landingPage.spec.js | 20 +++++++++++-------- spec/system/admin/admin_manages_pages_spec.rb | 8 ++++---- .../user_logs_in_with_email_spec.rb | 4 ++-- .../user_logs_in_with_facebook_spec.rb | 4 ++-- .../user_logs_in_with_github_spec.rb | 4 ++-- .../user_logs_in_with_twitter_spec.rb | 4 ++-- 13 files changed, 50 insertions(+), 37 deletions(-) diff --git a/app/helpers/authentication_helper.rb b/app/helpers/authentication_helper.rb index b6041f003..5b80d94f6 100644 --- a/app/helpers/authentication_helper.rb +++ b/app/helpers/authentication_helper.rb @@ -45,22 +45,22 @@ module AuthenticationHelper Settings::General.waiting_on_first_user end - def private_forem_or_no_enabled_auth_options - ForemInstance.private? || + def invite_only_mode_or_no_enabled_auth_options + ForemInstance.invitation_only? || (authentication_enabled_providers.none? && !Settings::Authentication.allow_email_password_registration) end def tooltip_class_on_auth_provider_enablebtn - private_forem_or_no_enabled_auth_options ? "crayons-tooltip" : "" + invite_only_mode_or_no_enabled_auth_options ? "crayons-tooltip" : "" end def disabled_attr_on_auth_provider_enable_btn - private_forem_or_no_enabled_auth_options ? "disabled" : "" + invite_only_mode_or_no_enabled_auth_options ? "disabled" : "" end def tooltip_text_email_or_auth_provider_btns - if private_forem_or_no_enabled_auth_options + if invite_only_mode_or_no_enabled_auth_options "You cannot do this until you disable Invite Only Mode" else "" diff --git a/app/models/forem_instance.rb b/app/models/forem_instance.rb index 7ed1e891c..5d29e127b 100644 --- a/app/models/forem_instance.rb +++ b/app/models/forem_instance.rb @@ -28,7 +28,11 @@ class ForemInstance (Settings::SMTP.user_name.present? && Settings::SMTP.password.present?) || ENV["SENDGRID_API_KEY"].present? end - def self.private? + def self.invitation_only? Settings::Authentication.invite_only_mode? end + + def self.private? + !Settings::UserExperience.public? + end end diff --git a/app/services/authentication/providers.rb b/app/services/authentication/providers.rb index 330fa16f4..082a6409a 100644 --- a/app/services/authentication/providers.rb +++ b/app/services/authentication/providers.rb @@ -44,7 +44,7 @@ module Authentication # TODO: [@forem/oss] ideally this should be "available - disabled" # we can get there once we have feature flags def self.enabled - return [] if ForemInstance.private? + return [] if ForemInstance.invitation_only? Settings::Authentication.providers.map(&:to_sym).sort end diff --git a/app/views/admin/settings/forms/_authentication.html.erb b/app/views/admin/settings/forms/_authentication.html.erb index 84eb05d1b..073ac74bf 100644 --- a/app/views/admin/settings/forms/_authentication.html.erb +++ b/app/views/admin/settings/forms/_authentication.html.erb @@ -13,10 +13,10 @@ General settings
" + class="crayons-field crayons-field--checkbox <%= invite_only_mode_or_no_enabled_auth_options ? "crayons-tooltip" : "" %>" data-tooltip="Unchecking this will enable Email Registration"> <%= f.check_box :invite_only_mode, - checked: private_forem_or_no_enabled_auth_options, + checked: invite_only_mode_or_no_enabled_auth_options, data: { action: "config#adjustAuthenticationOptions", "config-target": "inviteOnlyMode" }, class: "crayons-checkbox" %>
diff --git a/app/views/devise/registrations/_registration_form.html.erb b/app/views/devise/registrations/_registration_form.html.erb index fd1f9f4e3..97af77506 100644 --- a/app/views/devise/registrations/_registration_form.html.erb +++ b/app/views/devise/registrations/_registration_form.html.erb @@ -2,7 +2,7 @@

- <% if params[:state] == "new-user" && ForemInstance.private? %> + <% if params[:state] == "new-user" && ForemInstance.invitation_only? %> <%= community_name %> is invite only. <% else %> Welcome to <%= community_name %> diff --git a/app/views/shared/authentication/_providers_registration_form.html.erb b/app/views/shared/authentication/_providers_registration_form.html.erb index 2a3404eca..b575aa7bf 100644 --- a/app/views/shared/authentication/_providers_registration_form.html.erb +++ b/app/views/shared/authentication/_providers_registration_form.html.erb @@ -8,7 +8,7 @@ <% end %> <% end %> <% end %> - <% if params[:state] == "new-user" && Settings::Authentication.allow_email_password_registration && !ForemInstance.private? %> + <% if params[:state] == "new-user" && Settings::Authentication.allow_email_password_registration && !ForemInstance.invitation_only? %> <%= link_to "#{inline_svg_tag('email.svg', aria_hidden: true, class: 'crayons-icon', title: 'email')}Sign up with Email".html_safe, request.params.merge(state: "email_signup").except("i"), class: "crayons-btn crayons-btn--l crayons-btn--brand-email crayons-btn--icon-left whitespace-nowrap", diff --git a/cypress/integration/adminFlows/config/authenticationSection.spec.js b/cypress/integration/adminFlows/config/authenticationSection.spec.js index e97275bd5..75d3f77e8 100644 --- a/cypress/integration/adminFlows/config/authenticationSection.spec.js +++ b/cypress/integration/adminFlows/config/authenticationSection.spec.js @@ -23,9 +23,11 @@ describe('Authentication Section', () => { cy.visit('/admin/customization/config'); cy.findByTestId('authSectionForm').as('authSectionForm'); - cy.get('@authSectionForm').findByText('Authentication').click(); cy.get('@authSectionForm') - .findByLabelText('Invite-only mode') + .findByRole('heading', { name: 'Authentication' }) + .click(); + cy.get('@authSectionForm') + .findByRole('checkbox', { name: 'Invite-only mode' }) .should('not.be.checked') .check(); @@ -34,7 +36,9 @@ describe('Authentication Section', () => { .type( `My username is @${username} and this action is 100% safe and appropriate.`, ); - cy.get('@authSectionForm').findByText('Update Settings').click(); + cy.get('@authSectionForm') + .findByRole('button', { name: 'Update Settings' }) + .click(); cy.url().should('contains', '/admin/customization/config'); @@ -43,10 +47,11 @@ describe('Authentication Section', () => { cy.findByText('Successfully updated settings.').should('be.visible'); - cy.get('@authSectionForm').findByText('Authentication').click(); - cy.get('@authSectionForm') - .findByLabelText('Invite-only mode') + .findByRole('heading', { name: 'Authentication' }) + .click(); + cy.get('@authSectionForm') + .findByRole('checkbox', { name: 'Invite-only mode' }) .should('be.checked'); // Ensure that none of the authentication providers are enabled. diff --git a/cypress/integration/adminFlows/pages/landingPage.spec.js b/cypress/integration/adminFlows/pages/landingPage.spec.js index fe6280718..cde9d05f0 100644 --- a/cypress/integration/adminFlows/pages/landingPage.spec.js +++ b/cypress/integration/adminFlows/pages/landingPage.spec.js @@ -4,23 +4,27 @@ describe('Set a landing page from the admin portal', () => { cy.fixture('users/adminUser.json').as('user'); cy.get('@user').then((user) => { cy.loginAndVisit(user, '/admin/customization/config').then(() => { + cy.get('#new_settings_user_experience').as('userExperienceSectionForm'); // Ensure Forem instance is private // NOTE: @citizen428 - We may need to find a better situation for this // long-term. - cy.findByTestId('authSectionForm').as('authSectionForm'); - cy.get('@authSectionForm').findByText('Authentication').click(); - cy.get('@authSectionForm') - .findByLabelText('Invite-only mode') - .should('not.be.checked') - .check(); + cy.get('@userExperienceSectionForm') + .findByRole('heading', { name: 'User Experience and Brand' }) + .click(); + cy.get('@userExperienceSectionForm') + .findByRole('checkbox', { name: 'Public' }) + .should('be.checked') + .uncheck(); - cy.get('@authSectionForm') + cy.get('@userExperienceSectionForm') .findByPlaceholderText('Confirmation text') .type( `My username is @${user.username} and this action is 100% safe and appropriate.`, ); - cy.get('@authSectionForm').findByText('Update Settings').click(); + cy.get('@userExperienceSectionForm') + .findByRole('button', { name: 'Update Settings' }) + .click(); cy.visit('/admin/customization/pages'); }); diff --git a/spec/system/admin/admin_manages_pages_spec.rb b/spec/system/admin/admin_manages_pages_spec.rb index f80547129..3e2245842 100644 --- a/spec/system/admin/admin_manages_pages_spec.rb +++ b/spec/system/admin/admin_manages_pages_spec.rb @@ -124,7 +124,7 @@ RSpec.describe "Admin manages pages", type: :system do it "allows a landing page to be updated", :aggregate_failures do visit edit_admin_page_path(current_landing_page.id) - expect(page).to have_content("Use as 'Locked Screen") + expect(page).to have_content("Use as 'Locked Screen'") uncheck "Use as 'Locked Screen'" click_on("Update Page") expect(page).to have_current_path(admin_pages_path) @@ -132,7 +132,7 @@ RSpec.describe "Admin manages pages", type: :system do it "allows an Admin to click through to the current landing page via the modal", :aggregate_failures do visit edit_admin_page_path(new_landing_page.id) - expect(page).to have_content("Use as 'Locked Screen") + expect(page).to have_content("Use as 'Locked Screen'") check "Use as 'Locked Screen'" expect(page).to have_link("Current Locked Screen: #{new_landing_page.title}") click_on("Current Locked Screen") @@ -142,7 +142,7 @@ RSpec.describe "Admin manages pages", type: :system do it "allows an Admin to overwrite the current landing page via the checkbox and modal", :aggregate_failures do visit edit_admin_page_path(new_landing_page.id) - expect(page).to have_content("Use as 'Locked Screen") + expect(page).to have_content("Use as 'Locked Screen'") check "Use as 'Locked Screen'" expect(page).to have_link("Current Locked Screen: #{new_landing_page.title}") click_on("Overwrite current locked screen") @@ -155,7 +155,7 @@ RSpec.describe "Admin manages pages", type: :system do it "does not give admins the option to set a lock screen" do allow(ForemInstance).to receive(:private).and_return(false) visit edit_admin_page_path(new_landing_page.id) - expect(page).not_to have_content("Use as 'Locked Screen") + expect(page).not_to have_content("Use as 'Locked Screen'") end end end diff --git a/spec/system/authentication/user_logs_in_with_email_spec.rb b/spec/system/authentication/user_logs_in_with_email_spec.rb index 29d952e88..b73449645 100644 --- a/spec/system/authentication/user_logs_in_with_email_spec.rb +++ b/spec/system/authentication/user_logs_in_with_email_spec.rb @@ -114,9 +114,9 @@ RSpec.describe "Authenticating with Email" do end end - context "when community is in invite only mode" do + context "when community is in invite-only mode" do before do - allow(ForemInstance).to receive(:private?).and_return(true) + allow(ForemInstance).to receive(:invitation_only?).and_return(true) end it "doesn't present the authentication option" do diff --git a/spec/system/authentication/user_logs_in_with_facebook_spec.rb b/spec/system/authentication/user_logs_in_with_facebook_spec.rb index 15615f842..83246eeb7 100644 --- a/spec/system/authentication/user_logs_in_with_facebook_spec.rb +++ b/spec/system/authentication/user_logs_in_with_facebook_spec.rb @@ -232,9 +232,9 @@ RSpec.describe "Authenticating with Facebook" do end end - context "when community is in invite only mode" do + context "when community is in invite-only mode" do before do - allow(ForemInstance).to receive(:private?).and_return(true) + allow(ForemInstance).to receive(:invitation_only?).and_return(true) end it "doesn't present the authentication option" do diff --git a/spec/system/authentication/user_logs_in_with_github_spec.rb b/spec/system/authentication/user_logs_in_with_github_spec.rb index 407390cbc..2d6cb2a6d 100644 --- a/spec/system/authentication/user_logs_in_with_github_spec.rb +++ b/spec/system/authentication/user_logs_in_with_github_spec.rb @@ -198,9 +198,9 @@ RSpec.describe "Authenticating with GitHub" do end end - context "when community is in invite only mode" do + context "when community is in invite-only mode" do before do - allow(ForemInstance).to receive(:private?).and_return(true) + allow(ForemInstance).to receive(:invitation_only?).and_return(true) end it "doesn't present the authentication option" do diff --git a/spec/system/authentication/user_logs_in_with_twitter_spec.rb b/spec/system/authentication/user_logs_in_with_twitter_spec.rb index 63d842152..c72b509cc 100644 --- a/spec/system/authentication/user_logs_in_with_twitter_spec.rb +++ b/spec/system/authentication/user_logs_in_with_twitter_spec.rb @@ -190,9 +190,9 @@ RSpec.describe "Authenticating with Twitter" do end end - context "when community is in invite only mode" do + context "when community is in invite-only mode" do before do - allow(ForemInstance).to receive(:private?).and_return(true) + allow(ForemInstance).to receive(:invitation_only?).and_return(true) end it "doesn't present the authentication option" do