From 3252cb847c0f327fdd6ca1480ee76cb7d4511685 Mon Sep 17 00:00:00 2001 From: Vinoth Kannan Date: Thu, 16 Jul 2020 18:21:30 +0530 Subject: [PATCH] FIX: : trigger `user_updated` event only if email changed after user creation. Follow-up to 1460d7957c5d9b9300034e5e36675cf44cc3bc0f --- app/controllers/users_controller.rb | 2 ++ app/models/user_email.rb | 4 ---- lib/email_updater.rb | 1 + spec/components/email_updater_spec.rb | 18 ++++++++++++++++-- spec/models/user_email_spec.rb | 11 ++--------- spec/requests/users_controller_spec.rb | 20 ++++++++++++++++---- spec/requests/users_email_controller_spec.rb | 11 +++-------- 7 files changed, 40 insertions(+), 27 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 34e936a0f67..5127dc8d5b8 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -230,6 +230,7 @@ class UsersController < ApplicationController User.transaction do old_primary.update!(primary: false) new_primary.update!(primary: true) + DiscourseEvent.trigger(:user_updated, user) if current_user.staff? && current_user != user StaffActionLogger.new(current_user).log_update_email(user) @@ -259,6 +260,7 @@ class UsersController < ApplicationController ActiveRecord::Base.transaction do if user_email user_email.destroy + DiscourseEvent.trigger(:user_updated, user) elsif user.email_change_requests.where(new_email: params[:email]).destroy_all end diff --git a/app/models/user_email.rb b/app/models/user_email.rb index 6c6b0783d20..a3e76f22fd7 100644 --- a/app/models/user_email.rb +++ b/app/models/user_email.rb @@ -19,10 +19,6 @@ class UserEmail < ActiveRecord::Base scope :secondary, -> { where(primary: false) } - after_commit(on: [:create, :update, :destroy]) do - DiscourseEvent.trigger(:user_updated, user) - end - private def strip_downcase_email diff --git a/lib/email_updater.rb b/lib/email_updater.rb index d615f7ba8a2..ad886a066eb 100644 --- a/lib/email_updater.rb +++ b/lib/email_updater.rb @@ -121,6 +121,7 @@ class EmailUpdater @user.user_emails.create!(email: new_email) end + DiscourseEvent.trigger(:user_updated, @user) @user.set_automatic_groups end diff --git a/spec/components/email_updater_spec.rb b/spec/components/email_updater_spec.rb index 43709612044..3b110a78cae 100644 --- a/spec/components/email_updater_spec.rb +++ b/spec/components/email_updater_spec.rb @@ -140,10 +140,17 @@ describe EmailUpdater do context 'confirming a valid token' do it "updates the user's email" do Jobs.expects(:enqueue).once.with(:critical_user_email, has_entries(type: :notify_old_email, to_address: old_email)) - updater.confirm(@change_req.new_email_token.token) + + event = DiscourseEvent.track_events { + updater.confirm(@change_req.new_email_token.token) + }.last + expect(updater.errors).to be_blank expect(user.reload.email).to eq(new_email) + expect(event[:event_name]).to eq(:user_updated) + expect(event[:params].first).to eq(user) + @change_req.reload expect(@change_req.change_state).to eq(EmailChangeRequest.states[:complete]) end @@ -162,10 +169,17 @@ describe EmailUpdater do expect(UserHistory.where(action: UserHistory.actions[:add_email], acting_user_id: user.id).last).to be_present Jobs.expects(:enqueue).once.with(:critical_user_email, has_entries(type: :notify_old_email_add, to_address: old_email)) - updater.confirm(@change_req.new_email_token.token) + + event = DiscourseEvent.track_events { + updater.confirm(@change_req.new_email_token.token) + }.last + expect(updater.errors).to be_blank expect(UserEmail.where(user_id: user.id).pluck(:email)).to contain_exactly(user.email, new_email) + expect(event[:event_name]).to eq(:user_updated) + expect(event[:params].first).to eq(user) + @change_req.reload expect(@change_req.change_state).to eq(EmailChangeRequest.states[:complete]) end diff --git a/spec/models/user_email_spec.rb b/spec/models/user_email_spec.rb index ebbc05b91b4..ac821c7a5df 100644 --- a/spec/models/user_email_spec.rb +++ b/spec/models/user_email_spec.rb @@ -13,17 +13,10 @@ describe UserEmail do end it "allows multiple secondary emails" do - events = DiscourseEvent.track_events { - Fabricate(:secondary_email, user: user, primary: false) - Fabricate(:secondary_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 - expect(events.count).to eq 2 - - event = events.first - expect(event[:event_name]).to eq(:user_updated) - expect(event[:params].first).to eq(user) end it "does not allow an invalid email" do diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 5782bd1b832..4d1707d33d9 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -2652,11 +2652,17 @@ describe UsersController do expect(user_email.reload.primary).to eq(true) expect(other_email.reload.primary).to eq(false) - expect { put "/u/#{user.username}/preferences/primary-email.json", params: { email: other_email.email } } - .to change { UserHistory.where(action: UserHistory.actions[:update_email], acting_user_id: user.id).count }.by(1) + event = DiscourseEvent.track_events { + expect { put "/u/#{user.username}/preferences/primary-email.json", params: { email: other_email.email } } + .to change { UserHistory.where(action: UserHistory.actions[:update_email], acting_user_id: user.id).count }.by(1) + }.last + expect(response.status).to eq(200) expect(user_email.reload.primary).to eq(false) expect(other_email.reload.primary).to eq(true) + + expect(event[:event_name]).to eq(:user_updated) + expect(event[:params].first).to eq(user) end end @@ -2676,10 +2682,16 @@ describe UsersController do expect(response.status).to eq(428) expect(user.reload.user_emails.pluck(:email)).to contain_exactly(user_email.email, other_email.email) - expect { delete "/u/#{user.username}/preferences/email.json", params: { email: other_email.email } } - .to change { UserHistory.where(action: UserHistory.actions[:destroy_email], acting_user_id: user.id).count }.by(1) + event = DiscourseEvent.track_events { + expect { delete "/u/#{user.username}/preferences/email.json", params: { email: other_email.email } } + .to change { UserHistory.where(action: UserHistory.actions[:destroy_email], acting_user_id: user.id).count }.by(1) + }.last + expect(response.status).to eq(200) expect(user.reload.user_emails.pluck(:email)).to contain_exactly(user_email.email) + + expect(event[:event_name]).to eq(:user_updated) + expect(event[:params].first).to eq(user) end end diff --git a/spec/requests/users_email_controller_spec.rb b/spec/requests/users_email_controller_spec.rb index 372b10cde0c..06cc9ced98c 100644 --- a/spec/requests/users_email_controller_spec.rb +++ b/spec/requests/users_email_controller_spec.rb @@ -63,14 +63,9 @@ describe UsersEmailController do it 'confirms with a correct token' do user.user_stat.update_columns(bounce_score: 42, reset_bounce_score_after: 1.week.from_now) - event = DiscourseEvent.track_events { - put "/u/confirm-new-email", params: { - token: "#{user.email_tokens.last.token}" - } - }.last - - expect(event[:event_name]).to eq(:user_updated) - expect(event[:params].first).to eq(user) + put "/u/confirm-new-email", params: { + token: "#{user.email_tokens.last.token}" + } expect(response.status).to eq(302) expect(response.redirect_url).to include("done")