maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Improve the UX of the SMTP Section (#16223)

Browse source 
* feat: allow reply_to and email_from to be set for an SMTP config

* feat: use these SMTP values in the mailers

* spec: test the application_mailer

* fix: validate with email, and not url

* feat: mimic macs changes from https://github.com/forem/forem/pull/16216 to use in this PR

* setup packs for admin

* refactor: order the keys and add a const for the auth methods

* feat: rename the header to a more user friendlly name

* chore: move the section with Emails

* feat: add a toggle that will show and hide the SMTP form under certain conditions

* feat: add the javaScript to handle the toggles

* feat: add a better description until we convert to a dropdown

* feat: ensure that we have declared sendgrid_enabled

* chore: add  anote to the config controller

* chore: remove references of the email addresses to keep brnach scoped

* feat: tweak js

* test: cypress workflow to update smtp settings

* feat : update the smtp tests

* remove comments

* update test

* chore: rename NOTE

* feat: polisha dn test ForemInstance.only_sendgrid_enabled?

* chore: remove specs

* Update app/lib/constants/settings/smtp.rb

Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>

* Update spec/system/admin/config/admin_updates_smtp_settings_spec.rb

Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>

* Update spec/system/admin/config/admin_updates_smtp_settings_spec.rb

Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>

* Update spec/system/admin/config/admin_updates_smtp_settings_spec.rb

Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>

* Update app/javascript/packs/admin/config/smtp.js

Co-authored-by: Nick Taylor <nick@iamdeveloper.com>

* refactor js as per comments

* refactor as per comments

* Update cypress/integration/seededFlows/adminFlows/config/emailServerSettingsSection.spec.js

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>

* refactor: update the Cypress tests

Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
Co-authored-by: Nick Taylor <nick@iamdeveloper.com>
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
This commit is contained in:
Ridhwana 2022-02-15 18:40:14 +02:00 • committed by GitHub
parent c25435e596
commit 1025900d16
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
13 changed files with 254 additions and 24 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
app/controllers/stories/feeds_controller.rb
View file

@ -65,8 +65,8 @@ module Stories
end
def signed_out_base_feed
strategy = AbExperiment.get(experiment: AbExperiment::CURRENT_FEED_STRATEGY_EXPERIMENT, controller: self, user: current_user,
default_value: AbExperiment::ORIGINAL_VARIANT)
strategy = AbExperiment.get(experiment: AbExperiment::CURRENT_FEED_STRATEGY_EXPERIMENT, controller: self,
user: current_user, default_value: AbExperiment::ORIGINAL_VARIANT)
feed = if strategy.weighted_query_strategy?
Articles::Feeds::WeightedQueryStrategy.new(user: current_user, page: @page, tags: params[:tag])
elsif Settings::UserExperience.feed_strategy == "basic"

2
app/javascript/admin/controllers/config_controller.js
View file

@ -16,6 +16,8 @@ const emailAuthModalBody = `
<p>If you disable Email address as a registration option, people cannot create an account with their email address.</p>
<p>However, people who have already created an account using their email address can continue to login.</p>`;
// NOTE: In an effort to move away from Stimulus and create consistency across the codebase
// we are using vanilla JavaScript (app/javascript/packs/admin/config) to handle any new interactions.
export default class ConfigController extends Controller {
static targets = [
'authenticationProviders',

23
app/javascript/packs/admin/config/smtp.js Normal file
View file

@ -0,0 +1,23 @@
document
.getElementById('settings_smtp_own_email_server')
?.addEventListener('change', (event) => {
const [customSMTPSection] = document.getElementsByClassName(
'js-custom-smtp-section',
);
if (event.target.checked) {
customSMTPSection.classList.remove('hidden');
} else {
// when the user indicates that they do not want to use their own server
// we clear the form values except for those fields that have a default value.
const inputs = customSMTPSection.getElementsByTagName('input');
const haveDefaultValues = ['authentication'];
for (const input of inputs) {
if (!haveDefaultValues.some((el) => input.name.includes(el))) {
input.value = '';
}
}
customSMTPSection.classList.add('hidden');
}
});

7
app/lib/constants/settings/smtp.rb
View file

@ -4,7 +4,7 @@ module Constants
DETAILS = {
address: {
description: "Address of the remote mail server",
placeholder: "ie. smtp.gmail.com"
placeholder: "i.e. smtp.gmail.com"
},
port: {
description: "The port that your mail server runs on",
@ -12,8 +12,9 @@ module Constants
},
authentication: {
description: " If your mail server requires authentication, " \
"you need to specify the authentication type here",
placeholder: "ie. plain, login, or cram_md5"
"you need to specify the authentication type here. " \
" i.e. plain, login, or cram_md5",
placeholder: "i.e. plain, login, or cram_md5"
},
user_name: {
description: "If your mail server requires authentication, copy the username from your server",

8
app/models/forem_instance.rb
View file

@ -32,6 +32,14 @@ class ForemInstance
Settings::SMTP.provided_minimum_settings? || ENV["SENDGRID_API_KEY"].present?
end
def self.sendgrid_enabled?
ENV["SENDGRID_API_KEY"].present?
end
def self.only_sendgrid_enabled?
ForemInstance.sendgrid_enabled? && !Settings::SMTP.provided_minimum_settings?
end
def self.invitation_only?
Settings::Authentication.invite_only_mode?
end

5
app/models/settings/smtp.rb
View file

@ -2,11 +2,12 @@ module Settings
class SMTP < Base
self.table_name = :settings_smtp
OPTIONS = %i[address port authentication user_name password domain].freeze
OPTIONS = %i[user_name password address authentication domain port].freeze
AUTHENTICATION_METHODS = %w[plain login cram_md5].freeze
setting :address, type: :string, default: ApplicationConfig["SMTP_ADDRESS"].presence
setting :authentication, type: :string, default: ApplicationConfig["SMTP_AUTHENTICATION"].presence,
validates: { inclusion: %w[plain login cram_md5] }
validates: { inclusion: AUTHENTICATION_METHODS }
setting :domain, type: :string, default: ApplicationConfig["SMTP_DOMAIN"].presence
setting :password, type: :string, default: ApplicationConfig["SMTP_PASSWORD"].presence
setting :port, type: :integer, default: ApplicationConfig["SMTP_PORT"].presence || 25

4
app/services/articles/feeds/weighted_query_strategy.rb
View file

@ -119,7 +119,7 @@ module Articles
# Weight to give to the number of comments on the article.
comments_count_factor: {
clause: "articles.comments_count",
cases: (0..9).map { |n| [n, 0.8 + 0.02 * n] },
cases: (0..9).map { |n| [n, 0.8 + (0.02 * n)] },
fallback: 1,
requires_user: false,
group_by: "articles.comments_count"
@ -190,7 +190,7 @@ module Articles
# user follows and the article has.
matching_tags_factor: {
clause: "LEAST(10.0, SUM(followed_tags.points))::integer",
cases: (0..9).map { |n| [n, 0.70 + 0.0303 * n] },
cases: (0..9).map { |n| [n, 0.70 + (0.0303 * n)] },
fallback: 1,
requires_user: true,
joins: ["LEFT OUTER JOIN taggings

45
app/views/admin/settings/forms/_smtp.html.erb
View file

@ -1,22 +1,47 @@
<%= javascript_packs_with_chunks_tag "admin/config/smtp", defer: true %>
<%= form_for(Settings::SMTP.new,
url: admin_settings_smtp_settings_path,
html: { data: { action: "submit->config#updateConfigurationSettings" } }) do |f| %>
html: { data: {
action: "submit->config#updateConfigurationSettings",
testid: "emailServerSettings"
} }) do |f| %>
<div class="card mt-3" id="smtp-section">
<%= render partial: "admin/shared/card_header",
locals: { header: "SMTP Settings", state: "collapse", target: "smtpSettingsBodyContainer", expanded: false } %>
locals: { header: "Email Server Settings (SMTP)", state: "collapse", target: "smtpSettingsBodyContainer", expanded: false } %>
<div id="smtpSettingsBodyContainer" class="card-body collapse hide" aria-labelledby="smtpSettingsBodyContainer">
<fieldset class="grid gap-4">
<% Settings::SMTP::OPTIONS.each do |config_key| %>
<div class="crayons-field">
<%= admin_config_label config_key %>
<%= admin_config_description Constants::Settings::SMTP::DETAILS[config_key][:description] %>
<%= f.text_field config_key,
class: "crayons-textfield",
value: Settings::SMTP.public_send(config_key),
placeholder: Constants::Settings::SMTP::DETAILS[config_key][:placeholder] %>
<% if ForemInstance.sendgrid_enabled? %>
<div class="crayons-field crayons-field--checkbox mt-3">
<%= f.check_box :own_email_server,
checked: Settings::SMTP.provided_minimum_settings?,
class: "crayons-checkbox" %>
<%= f.label :own_email_server, class: "crayons-field__label" do %>
Use my own email server
<% end %>
</div>
<% end %>
<% if ForemInstance.only_sendgrid_enabled? %>
<p class="mb-3">
As a Forem Cloud client, we provide an email server managed by the Forem team. All settings are managed by us and the from and reply email addresses are set as <%= ForemInstance.email %>. However, you can override this to use your own email server.
</p>
<% end %>
<div class="js-custom-smtp-section <%= "hidden" if ForemInstance.only_sendgrid_enabled? %>">
<% Settings::SMTP::OPTIONS.each do |config_key| %>
<div class="crayons-field mt-3">
<%= admin_config_label config_key, model: Settings::SMTP %>
<%= admin_config_description Constants::Settings::SMTP::DETAILS[config_key][:description] %>
<%= f.text_field config_key,
class: "crayons-textfield",
value: Settings::SMTP.public_send(config_key),
placeholder: Constants::Settings::SMTP::DETAILS[config_key][:placeholder],
"data-config-target": "smtpSetting#{config_key.to_s.camelize(:upper)}" %>
</div>
<% end %>
</div>
</fieldset>
<%= render "update_setting_button", f: f %>
</div>

2
app/views/admin/settings/show.html.erb
View file

@ -34,6 +34,7 @@
<%= render partial: "forms/community" %>
<%= render partial: "forms/credits" %>
<%= render partial: "forms/emails" %>
<%= render partial: "forms/smtp" %>
<%= render partial: "forms/google_analytics" %>
<%= render partial: "forms/images", locals: { logo_allowed_types: @logo_allowed_types, logo_max_file_size: @logo_max_file_size } %>
<%= render partial: "forms/mascot" %>
@ -42,7 +43,6 @@
<%= render partial: "forms/newsletter" %>
<%= render partial: "forms/onboarding" %>
<%= render partial: "forms/rate_limit" %>
<%= render partial: "forms/smtp" %>
<%= render partial: "forms/sponsors" %>
<%= render partial: "forms/tags" %>
<%= render partial: "forms/user_experience" %>

36
cypress/integration/seededFlows/adminFlows/config/emailServerSettingsSection.spec.js Normal file
View file

@ -0,0 +1,36 @@
describe('Email Server Settings Section', () => {
beforeEach(() => {
cy.testSetup();
cy.fixture('users/adminUser.json').as('user');
cy.get('@user').then((user) => {
cy.loginAndVisit(user, '/admin/customization/config');
});
});
describe('email server settings', () => {
it('updates the smtp fields', () => {
cy.findByTestId('emailServerSettings').as('emailServerSettings');
cy.get('@emailServerSettings')
.findByText('Email Server Settings (SMTP)')
.click();
cy.get('@emailServerSettings').within(() => {
cy.findByText('Email Server Settings (SMTP)').click();
cy.findByLabelText('User name').clear().type('jane_doe');
cy.findByLabelText('Password').clear().type('abc123456');
cy.findByLabelText('Address').clear().type('smtp.gmail.com');
cy.findByLabelText('Authentication').clear().type('plain');
cy.findByText('Update Settings').click();
});
cy.url().should('contains', '/admin/customization/config');
cy.findByText('Successfully updated settings.').should('be.visible');
cy.findByLabelText('User name').should('have.value', 'jane_doe');
cy.findByLabelText('Password').should('have.value', 'abc123456');
cy.findByLabelText('Address').should('have.value', 'smtp.gmail.com');
cy.findByLabelText('Authentication').should('have.value', 'plain');
});
});
});

6
cypress/integration/seededFlows/adminFlows/pages/landingPage.spec.js
View file

@ -112,8 +112,7 @@ describe('Set a landing page from the admin portal', () => {
cy.findByRole('main').within(() => {
cy.findByRole('checkbox', {
name: "Use as 'Locked Screen' Determines if this page will be used as a landing page for anonymous viewers.",
})
.check();
}).check();
cy.findAllByRole('button', { name: 'Update Page' }).first().click();
});
@ -139,8 +138,7 @@ describe('Set a landing page from the admin portal', () => {
cy.findByRole('main').within(() => {
cy.findByRole('checkbox', {
name: "Use as 'Locked Screen' Determines if this page will be used as a landing page for anonymous viewers.",
})
.check();
}).check();
cy.findAllByRole('button', { name: 'Cancel' }).first().click();

25
spec/models/forem_instance_spec.rb
View file

@ -108,4 +108,29 @@ RSpec.describe ForemInstance, type: :model do
expect(described_class.contact_email).to be(email)
end
end
describe ".only_sendgrid_enabled?" do
it "returns false when the minimum SMTP settings are provided" do
allow(Settings::SMTP).to receive(:user_name).and_return("something")
allow(Settings::SMTP).to receive(:password).and_return("something")
allow(Settings::SMTP).to receive(:address).and_return("something")
expect(described_class.only_sendgrid_enabled?).to be(false)
end
it "returns false when Sendgrid is not enabled" do
allow(described_class).to receive(:sendgrid_enabled?).and_return(false)
expect(described_class.only_sendgrid_enabled?).to be(false)
end
it "returns true if Sendgrid is enabled and the minimum SMTP settings are not provided" do
allow(described_class).to receive(:sendgrid_enabled?).and_return(true)
allow(Settings::SMTP).to receive(:user_name).and_return(nil)
allow(Settings::SMTP).to receive(:password).and_return(nil)
allow(Settings::SMTP).to receive(:address).and_return(nil)
expect(described_class.only_sendgrid_enabled?).to be(true)
end
end
end

111
spec/system/admin/config/admin_updates_smtp_settings_spec.rb Normal file
View file

@ -0,0 +1,111 @@
require "rails_helper"
RSpec.describe "Admin updates SMTP Settings", type: :system do
let(:admin) { create(:user, :super_admin) }
before do
sign_in admin
end
# We're unable to set and unset an ENV variable in Cypress to test different scenarios
# hence, we test the view layouts with Capybara, and we test successful updates with Cypress.
context "when Sendgrid is not enabled and SMTP is not enabled" do
before do
allow(ForemInstance).to receive(:sendgrid_enabled?).and_return(false)
allow(Settings::SMTP).to receive(:address).and_return(nil)
allow(Settings::SMTP).to receive(:user_name).and_return(nil)
allow(Settings::SMTP).to receive(:password).and_return(nil)
visit admin_config_path
end
it "does not show the 'Use my own email server' checkbox" do
within("form[data-testid='emailServerSettings']") do
expect(page).not_to have_content("Use my own email server")
end
end
it "shows the SMTP Form", :aggregate_failures do
within("form[data-testid='emailServerSettings']") do
expect(page).to have_selector(".js-custom-smtp-section")
expect(page).not_to have_selector(".js-custom-smtp-section.hidden")
end
end
end
context "when Sendgrid is enabled and SMTP is not enabled" do
before do
allow(ForemInstance).to receive(:sendgrid_enabled?).and_return(true)
allow(ForemInstance).to receive(:email).and_return("yo@forem.com")
allow(Settings::SMTP).to receive(:address).and_return(nil)
allow(Settings::SMTP).to receive(:user_name).and_return(nil)
allow(Settings::SMTP).to receive(:password).and_return(nil)
visit admin_config_path
end
it "shows the checkbox to allow one to toggle ones own server" do
within("form[data-testid='emailServerSettings']") do
expect(page).to have_content("Use my own email server")
end
end
it "shows a description" do
within("form[data-testid='emailServerSettings']") do
# rubocop:disable Layout/LineLength
expect(page).to have_content("As a Forem Cloud client, we provide an email server managed by the Forem team. All settings are managed by us and the from and reply email addresses are set as yo@forem.com. However, you can override this to use your own email server.")
# rubocop:enable Layout/LineLength
end
end
it "does not show an SMTP Form" do
within("form[data-testid='emailServerSettings']") do
expect(page).to have_selector(".js-custom-smtp-section.hidden")
end
end
end
context "when Sendgrid is not enabled and SMTP is enabled" do
before do
allow(ForemInstance).to receive(:sendgrid_enabled?).and_return(false)
allow(Settings::SMTP).to receive(:address).and_return("smtp.gmail.com")
allow(Settings::SMTP).to receive(:user_name).and_return("jane_doe")
allow(Settings::SMTP).to receive(:password).and_return("abc123456")
visit admin_config_path
end
it "does not show the 'Use my own email server' checkbox" do
within("form[data-testid='emailServerSettings']") do
expect(page).not_to have_content("Use my own email server")
end
end
it "shows the SMTP Form", :aggregate_failures do
within("form[data-testid='emailServerSettings']") do
expect(page).to have_selector(".js-custom-smtp-section")
expect(page).not_to have_selector(".js-custom-smtp-section.hidden")
end
end
end
context "when Sendgrid is enabled and SMTP is enabled" do
before do
allow(ForemInstance).to receive(:sendgrid_enabled?).and_return(true)
allow(Settings::SMTP).to receive(:address).and_return("smtp.gmail.com")
allow(Settings::SMTP).to receive(:user_name).and_return("jane_doe")
allow(Settings::SMTP).to receive(:password).and_return("abc123456")
visit admin_config_path
end
it "shows the 'Use my own email server' checkbox" do
within("form[data-testid='emailServerSettings']") do
expect(page).to have_content("Use my own email server")
end
end
it "shows an SMTP Form", :aggregate_failures do
within("form[data-testid='emailServerSettings']") do
expect(page).to have_selector(".js-custom-smtp-section")
expect(page).not_to have_selector(".js-custom-smtp-section.hidden")
end
end
end
end