From 21ebb1cd54dd19767a2d821d4c92f24f0c72a12c Mon Sep 17 00:00:00 2001 From: Leo McArdle Date: Tue, 3 Jul 2018 12:51:22 +0100 Subject: [PATCH] FEATURE: Secondary emails support. --- .../admin/templates/user-index.hbs | 26 ++++- .../javascripts/discourse/models/user.js.es6 | 1 + .../stylesheets/common/admin/users.scss | 4 + app/controllers/users_controller.rb | 5 +- app/models/user.rb | 8 ++ app/models/user_email.rb | 2 + app/serializers/admin_user_list_serializer.rb | 2 + config/locales/client.en.yml | 3 + spec/components/email/receiver_spec.rb | 54 ++++++++++ spec/fabricators/user_email_fabricator.rb | 2 +- spec/fabricators/user_fabricator.rb | 2 +- spec/models/user_email_spec.rb | 12 +-- spec/models/user_spec.rb | 13 +++ spec/requests/users_controller_spec.rb | 11 ++- .../admin_user_list_serializer_spec.rb | 78 +++++++++++++++ .../acceptance/admin-user-emails-test.js.es6 | 98 +++++++++++++++++++ 16 files changed, 307 insertions(+), 14 deletions(-) create mode 100644 spec/serializers/admin_user_list_serializer_spec.rb create mode 100644 test/javascripts/acceptance/admin-user-emails-test.js.es6 diff --git a/app/assets/javascripts/admin/templates/user-index.hbs b/app/assets/javascripts/admin/templates/user-index.hbs index 88d3b6bfcca..14bbf89fb85 100644 --- a/app/assets/javascripts/admin/templates/user-index.hbs +++ b/app/assets/javascripts/admin/templates/user-index.hbs @@ -60,7 +60,7 @@ {{#if canCheckEmails}}
-
{{i18n 'user.email.title'}}
+
{{i18n 'user.email.primary'}}
{{#unless model.active}}
{{i18n 'admin.users.not_verified'}}
@@ -73,6 +73,30 @@
+
+
{{i18n 'user.email.secondary'}}
+ +
+ {{#if model.email}} + {{#if model.secondary_emails}} +
    + {{#each model.secondary_emails as |email| }} +
  • {{email}}
    • + {{/each}} +
+ {{else}} + {{i18n 'user.email.no_secondary'}} + {{/if}} + {{else}} + {{d-button action="checkEmail" + actionParam=model + icon="envelope-o" + label="admin.users.check_email.text" + title="admin.users.check_email.title"}} + {{/if}} +
+
+
{{i18n 'admin.user.bounce_score'}}
{{model.bounceScore}}
diff --git a/app/assets/javascripts/discourse/models/user.js.es6 b/app/assets/javascripts/discourse/models/user.js.es6 index 96f078baeb3..2d42c14c8db 100644 --- a/app/assets/javascripts/discourse/models/user.js.es6 +++ b/app/assets/javascripts/discourse/models/user.js.es6 @@ -604,6 +604,7 @@ const User = RestModel.extend({ if (result) { this.setProperties({ email: result.email, + secondary_emails: result.secondary_emails, associated_accounts: result.associated_accounts }); } diff --git a/app/assets/stylesheets/common/admin/users.scss b/app/assets/stylesheets/common/admin/users.scss index a92ddd8f35b..246dfc5c523 100644 --- a/app/assets/stylesheets/common/admin/users.scss +++ b/app/assets/stylesheets/common/admin/users.scss @@ -29,6 +29,10 @@ &:after { clear: both; } + &.secondary-emails ul { + margin: 0; + list-style: none; + } .field { font-weight: bold; width: 17.65765%; diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 187d859e793..cdb20e2129f 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -146,8 +146,11 @@ class UsersController < ApplicationController StaffActionLogger.new(current_user).log_check_email(user, context: params[:context]) + email, *secondary_emails = user.emails + render json: { - email: user.email, + email: email, + secondary_emails: secondary_emails, associated_accounts: user.associated_accounts } rescue Discourse::InvalidAccess diff --git a/app/models/user.rb b/app/models/user.rb index a0022b7db56..bf4b0bd3e01 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1069,6 +1069,14 @@ class User < ActiveRecord::Base end end + def emails + self.user_emails.order("user_emails.primary DESC NULLS LAST").pluck(:email) + end + + def secondary_emails + self.user_emails.secondary.pluck(:email) + end + def recent_time_read self.created_at && self.created_at < 60.days.ago ? self.user_visits.where('visited_at >= ?', 60.days.ago).sum(:time_read) : diff --git a/app/models/user_email.rb b/app/models/user_email.rb index 4a5a9e80cab..aaf608a6247 100644 --- a/app/models/user_email.rb +++ b/app/models/user_email.rb @@ -15,6 +15,8 @@ class UserEmail < ActiveRecord::Base validate :user_id_not_changed, if: :primary validate :unique_email + scope :secondary, -> { where(primary: false) } + private def strip_downcase_email diff --git a/app/serializers/admin_user_list_serializer.rb b/app/serializers/admin_user_list_serializer.rb index 32021dad747..47e5fea86d9 100644 --- a/app/serializers/admin_user_list_serializer.rb +++ b/app/serializers/admin_user_list_serializer.rb @@ -1,6 +1,7 @@ class AdminUserListSerializer < BasicUserSerializer attributes :email, + :secondary_emails, :active, :admin, :moderator, @@ -41,6 +42,7 @@ class AdminUserListSerializer < BasicUserSerializer (scope.is_staff? && object.staged?) end + alias_method :include_secondary_emails?, :include_email? alias_method :include_associated_accounts?, :include_email? def silenced diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 29db1a3a5e1..f99b5fec6d6 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -808,6 +808,9 @@ en: email: title: "Email" + primary: "Primary Email" + secondary: "Secondary Emails" + no_secondary: "No secondary emails" instructions: "never shown to the public" ok: "We will email you to confirm" invalid: "Please enter a valid email address" diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb index 927f6961d5e..1c08e59cb43 100644 --- a/spec/components/email/receiver_spec.rb +++ b/spec/components/email/receiver_spec.rb @@ -169,6 +169,21 @@ describe Email::Receiver do expect { process(:from_reply_by_email_address) }.to raise_error(Email::Receiver::FromReplyByAddressError) end + it "accepts reply from secondary email address" do + Fabricate(:secondary_email, email: "someone_else@bar.com", user: user) + + expect { process(:reply_user_not_matching) } + .to change { topic.posts.count } + + post = Post.last + + expect(post.raw).to eq( + "Lorem ipsum dolor sit amet, consectetur adipiscing elit." + ) + + expect(post.user).to eq(user) + end + it "raises a TopicNotFoundError when the topic was deleted" do topic.update_columns(deleted_at: 1.day.ago) expect { process(:reply_user_matching) }.to raise_error(Email::Receiver::TopicNotFoundError) @@ -490,6 +505,27 @@ describe Email::Receiver do expect(topic.topic_users.count).to eq(3) end + it "invites users with a secondary email in the chain" do + user1 = Fabricate(:user, + trust_level: SiteSetting.email_in_min_trust, + user_emails: [ + Fabricate.build(:secondary_email, email: "discourse@bar.com"), + Fabricate.build(:secondary_email, email: "someone@else.com"), + ] + ) + + user2 = Fabricate(:user, + trust_level: SiteSetting.email_in_min_trust, + user_emails: [ + Fabricate.build(:secondary_email, email: "team@bar.com"), + Fabricate.build(:secondary_email, email: "wat@bar.com"), + ] + ) + + expect { process(:cc) }.to change(Topic, :count) + expect(Topic.last.allowed_users).to contain_exactly(user1, user2) + end + it "cap the number of staged users created per email" do SiteSetting.maximum_staged_users_per_email = 1 expect { process(:cc) }.to change(Topic, :count) @@ -696,6 +732,24 @@ describe Email::Receiver do SiteSetting.ignore_by_title = "foo" expect { process(:ignored) }.to_not change(Topic, :count) end + + it "associates email from a secondary address with user" do + user = Fabricate(:user, + trust_level: SiteSetting.email_in_min_trust, + user_emails: [ + Fabricate.build(:secondary_email, email: "existing@bar.com") + ] + ) + + expect { process(:existing_user) }.to change(Topic, :count).by(1) + + topic = Topic.last + + expect(topic.posts.last.raw) + .to eq("Hey, this is a topic from an existing user ;)") + + expect(topic.user).to eq(user) + end end context "new topic in a category that allows strangers" do diff --git a/spec/fabricators/user_email_fabricator.rb b/spec/fabricators/user_email_fabricator.rb index 8e7814bf219..099b97fb3c7 100644 --- a/spec/fabricators/user_email_fabricator.rb +++ b/spec/fabricators/user_email_fabricator.rb @@ -3,7 +3,7 @@ Fabricator(:user_email) do primary true end -Fabricator(:alternate_email, from: :user_email) do +Fabricator(:secondary_email, from: :user_email) do email { sequence(:email) { |i| "bwayne#{i}@wayne.com" } } primary false end diff --git a/spec/fabricators/user_fabricator.rb b/spec/fabricators/user_fabricator.rb index dc535a4e250..858e4e30c79 100644 --- a/spec/fabricators/user_fabricator.rb +++ b/spec/fabricators/user_fabricator.rb @@ -12,7 +12,7 @@ Fabricator(:user_single_email, class_name: :user) do end Fabricator(:user, from: :user_single_email) do - after_create { |user| Fabricate(:alternate_email, user: user) } + after_create { |user| Fabricate(:secondary_email, user: user) } end Fabricator(:coding_horror, from: :user) do diff --git a/spec/models/user_email_spec.rb b/spec/models/user_email_spec.rb index c2d822491ea..22690c9bc30 100644 --- a/spec/models/user_email_spec.rb +++ b/spec/models/user_email_spec.rb @@ -6,14 +6,14 @@ describe UserEmail do it "allows only one primary email" do user = Fabricate(:user_single_email) expect { - Fabricate(:alternate_email, user: user, primary: true) + Fabricate(:secondary_email, user: user, primary: true) }.to raise_error(ActiveRecord::RecordInvalid) end it "allows multiple secondary emails" do user = Fabricate(:user_single_email) - Fabricate(:alternate_email, user: user, primary: false) - Fabricate(:alternate_email, user: user, primary: false) + Fabricate(:secondary_email, user: user, primary: false) + Fabricate(:secondary_email, user: user, primary: false) expect(user.user_emails.count).to eq 3 end end @@ -22,14 +22,14 @@ describe UserEmail do it "allows only one primary email" do user = Fabricate(:user_single_email) expect { - Fabricate.build(:alternate_email, user: user, primary: true).save(validate: false) + Fabricate.build(:secondary_email, user: user, primary: true).save(validate: false) }.to raise_error(ActiveRecord::RecordNotUnique) end it "allows multiple secondary emails" do user = Fabricate(:user_single_email) - Fabricate.build(:alternate_email, user: user, primary: false).save(validate: false) - Fabricate.build(:alternate_email, user: user, primary: false).save(validate: false) + Fabricate.build(:secondary_email, user: user, primary: false).save(validate: false) + Fabricate.build(:secondary_email, user: user, primary: false).save(validate: false) expect(user.user_emails.count).to eq 3 end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 6a34ecae5c0..e91c3e86c8c 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1758,4 +1758,17 @@ describe User do filter_by(:filter_by_username_or_email) end end + + describe "#secondary_emails" do + let(:user) { Fabricate(:user_single_email) } + + it "only contains secondary emails" do + expect(user.user_emails.secondary).to eq([]) + + secondary_email = Fabricate(:secondary_email, user: user) + + expect(user.user_emails.secondary).to contain_exactly(secondary_email) + end + end + end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 9f86db38a7d..3fd4f18a9f5 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -1900,14 +1900,16 @@ describe UsersController do expect(response).to be_forbidden end - it "returns both email and associated_accounts when you're allowed to see them" do + it "returns emails and associated_accounts when you're allowed to see them" do + user = Fabricate(:user) sign_in_admin - get "/u/#{Fabricate(:user).username}/emails.json" + get "/u/#{user.username}/emails.json" expect(response.status).to eq(200) json = JSON.parse(response.body) - expect(json["email"]).to be_present + expect(json["email"]).to eq(user.email) + expect(json["secondary_emails"]).to eq(user.secondary_emails) expect(json["associated_accounts"]).to be_present end @@ -1919,7 +1921,8 @@ describe UsersController do expect(response.status).to eq(200) json = JSON.parse(response.body) - expect(json["email"]).to be_present + expect(json["email"]).to eq(inactive_user.email) + expect(json["secondary_emails"]).to eq(inactive_user.secondary_emails) expect(json["associated_accounts"]).to be_present end end diff --git a/spec/serializers/admin_user_list_serializer_spec.rb b/spec/serializers/admin_user_list_serializer_spec.rb new file mode 100644 index 00000000000..a8bf155f39b --- /dev/null +++ b/spec/serializers/admin_user_list_serializer_spec.rb @@ -0,0 +1,78 @@ +require 'rails_helper' +require_dependency 'user' + +describe AdminUserListSerializer do + + context "emails" do + let(:admin) { Fabricate(:user_single_email, admin: true, email: "admin@email.com") } + let(:user) { Fabricate(:user_single_email, email: "user@email.com") } + let(:guardian) { Guardian.new(admin) } + + let(:json) do + AdminUserListSerializer.new(user, + scope: guardian, + root: false + ).as_json + end + + def fabricate_secondary_emails_for(u) + ["first", "second"].each do |name| + Fabricate(:secondary_email, user: u, email: "#{name}@email.com") + end + end + + shared_examples "shown" do |email| + it "contains emails" do + expect(json[:email]).to eq(email) + + expect(json[:secondary_emails]).to contain_exactly( + "first@email.com", + "second@email.com" + ) + end + end + + shared_examples "not shown" do + it "doesn't contain emails" do + expect(json[:email]).to eq(nil) + expect(json[:secondary_emails]).to eq(nil) + end + end + + context "with myself" do + let(:user) { admin } + + before do + fabricate_secondary_emails_for(admin) + end + + include_examples "shown", "admin@email.com" + end + + context "with a normal user" do + before do + fabricate_secondary_emails_for(user) + end + + include_examples "not shown" + end + + context "with a normal user after clicking 'show emails'" do + before do + guardian.can_see_emails = true + fabricate_secondary_emails_for(user) + end + + include_examples "shown", "user@email.com" + end + + context "with a staged user" do + before do + user.staged = true + fabricate_secondary_emails_for(user) + end + + include_examples "shown", "user@email.com" + end + end +end diff --git a/test/javascripts/acceptance/admin-user-emails-test.js.es6 b/test/javascripts/acceptance/admin-user-emails-test.js.es6 new file mode 100644 index 00000000000..e4323749cb8 --- /dev/null +++ b/test/javascripts/acceptance/admin-user-emails-test.js.es6 @@ -0,0 +1,98 @@ +import { acceptance } from "helpers/qunit-helpers"; + +acceptance("Admin - User Emails", { loggedIn: true }); + +const responseWithSecondary = secondaryEmails => { + return [ + 200, + { "Content-Type": "application/json" }, + { + id: 1, + username: "eviltrout", + email: "eviltrout@example.com", + secondary_emails: secondaryEmails + } + ]; +}; + +const assertNoSecondary = assert => { + assert.equal( + find(".display-row.email .value a").text(), + "eviltrout@example.com", + "it should display the primary email" + ); + + assert.equal( + find(".display-row.secondary-emails .value").text().trim(), + I18n.t("user.email.no_secondary"), + "it should not display secondary emails" + ); +}; + +const assertMultipleSecondary = assert => { + assert.equal( + find(".display-row.secondary-emails .value li:first-of-type a").text(), + "eviltrout1@example.com", + "it should display the first secondary email" + ); + + assert.equal( + find(".display-row.secondary-emails .value li:last-of-type a").text(), + "eviltrout2@example.com", + "it should display the second secondary email" + ); +}; + +QUnit.test("viewing self without secondary emails", async assert => { + server.get("/admin/users/1.json", () => { // eslint-disable-line no-undef + return responseWithSecondary([]); + }); + + await visit("/admin/users/1/eviltrout"); + + assertNoSecondary(assert); +}); + +QUnit.test("viewing self with multiple secondary emails", async assert => { + server.get("/admin/users/1.json", () => { // eslint-disable-line no-undef + return responseWithSecondary([ + "eviltrout1@example.com", + "eviltrout2@example.com", + ]); + }); + + await visit("/admin/users/1/eviltrout"); + + assert.equal( + find(".display-row.email .value a").text(), + "eviltrout@example.com", + "it should display the user's primary email" + ); + + assertMultipleSecondary(assert); +}); + +QUnit.test("viewing another user with no secondary email", async assert => { + await visit("/admin/users/1234/regular"); + await click(`.display-row.secondary-emails button`); + + assertNoSecondary(assert); +}); + +QUnit.test("viewing another account with secondary emails", async assert => { + server.get("/u/regular/emails.json", () => { // eslint-disable-line no-undef + return [ + 200, + { "Content-Type": "application/json" }, + { + email: "eviltrout@example.com", + secondary_emails: ["eviltrout1@example.com", "eviltrout2@example.com"] + } + ]; + }); + + await visit("/admin/users/1234/regular"); + await click(`.display-row.secondary-emails button`); + + assertMultipleSecondary(assert); +});