From 1bf722784396082c1f07e876c98d78217f9356c5 Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Tue, 8 Jun 2021 22:45:01 +0530 Subject: [PATCH] chore: Fix emails being sent with the wrong translations (#2236) Co-authored-by: Pranav Raj S --- .../api/v1/accounts/agents_controller.rb | 2 ++ .../v1/accounts/conversations_controller.rb | 4 ++- .../api/v1/widget/conversations_controller.rb | 2 +- app/mailers/application_mailer.rb | 1 + app/models/concerns/reauthorizable.rb | 4 ++- app/models/user.rb | 2 +- .../email_templates/db_resolver_service.rb | 4 +-- .../mailer/confirmation_instructions.html.erb | 5 ++-- .../conversation_reply_email_worker.rb | 4 +-- db/seeds.rb | 16 +++++++++++ .../accounts/conversations_controller_spec.rb | 14 ++++++++-- .../widget/conversations_controller_spec.rb | 6 ++-- .../db_resolver_service_spec.rb | 28 +++++++++---------- .../mailers/confirmation_instructions_spec.rb | 6 ++-- .../conversation_reply_email_worker_spec.rb | 15 ++++++---- 15 files changed, 75 insertions(+), 38 deletions(-) diff --git a/app/controllers/api/v1/accounts/agents_controller.rb b/app/controllers/api/v1/accounts/agents_controller.rb index 46d35d09d..334844de9 100644 --- a/app/controllers/api/v1/accounts/agents_controller.rb +++ b/app/controllers/api/v1/accounts/agents_controller.rb @@ -38,6 +38,8 @@ class Api::V1::Accounts::AgentsController < Api::V1::Accounts::BaseController @user = User.find_by(email: new_agent_params[:email]) end + # TODO: move this to a builder and combine the save account user method into a builder + # ensure the account user association is also created in a single transaction def create_user return if @user diff --git a/app/controllers/api/v1/accounts/conversations_controller.rb b/app/controllers/api/v1/accounts/conversations_controller.rb index 897731d19..91381095d 100644 --- a/app/controllers/api/v1/accounts/conversations_controller.rb +++ b/app/controllers/api/v1/accounts/conversations_controller.rb @@ -41,7 +41,9 @@ class Api::V1::Accounts::ConversationsController < Api::V1::Accounts::BaseContro end def transcript - ConversationReplyMailer.conversation_transcript(@conversation, params[:email])&.deliver_later if params[:email].present? + render json: { error: 'email param missing' }, status: :unprocessable_entity and return if params[:email].blank? + + ConversationReplyMailer.with(account: @conversation.account).conversation_transcript(@conversation, params[:email])&.deliver_later head :ok end diff --git a/app/controllers/api/v1/widget/conversations_controller.rb b/app/controllers/api/v1/widget/conversations_controller.rb index cc68cb888..8d28345d4 100644 --- a/app/controllers/api/v1/widget/conversations_controller.rb +++ b/app/controllers/api/v1/widget/conversations_controller.rb @@ -23,7 +23,7 @@ class Api::V1::Widget::ConversationsController < Api::V1::Widget::BaseController def transcript if permitted_params[:email].present? && conversation.present? - ConversationReplyMailer.conversation_transcript( + ConversationReplyMailer.with(account: conversation.account).conversation_transcript( conversation, permitted_params[:email] )&.deliver_later diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index 9b5c876d6..9d5fd0214 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -65,6 +65,7 @@ class ApplicationMailer < ActionMailer::Base end def ensure_current_account(account) + Current.reset Current.account = account if account.present? end diff --git a/app/models/concerns/reauthorizable.rb b/app/models/concerns/reauthorizable.rb index 6e2433482..a0c43310b 100644 --- a/app/models/concerns/reauthorizable.rb +++ b/app/models/concerns/reauthorizable.rb @@ -36,7 +36,9 @@ module Reauthorizable # could used to manually prompt reauthorization if auth scope changes def prompt_reauthorization! ::Redis::Alfred.set(reauthorization_required_key, true) - AdministratorNotifications::ChannelNotificationsMailer.slack_disconnect(account)&.deliver_later if (is_a? Integrations::Hook) && slack? + return unless (is_a? Integrations::Hook) && slack? + + AdministratorNotifications::ChannelNotificationsMailer.with(account: account).slack_disconnect(account)&.deliver_later end # call this after you successfully Reauthorized the object in UI diff --git a/app/models/user.rb b/app/models/user.rb index e1b1b4e71..fefe8669d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -91,7 +91,7 @@ class User < ApplicationRecord scope :order_by_full_name, -> { order('lower(name) ASC') } def send_devise_notification(notification, *args) - devise_mailer.send(notification, self, *args).deliver_later + devise_mailer.with(account: Current.account).send(notification, self, *args).deliver_later end def set_password_and_uid diff --git a/app/services/email_templates/db_resolver_service.rb b/app/services/email_templates/db_resolver_service.rb index 139684526..42222fe75 100644 --- a/app/services/email_templates/db_resolver_service.rb +++ b/app/services/email_templates/db_resolver_service.rb @@ -43,12 +43,12 @@ class ::EmailTemplates::DbResolverService < ActionView::Resolver handler = ActionView::Template.registered_template_handler(:liquid) template_details = { + locals: [], format: Mime['html'].to_sym, - updated_at: @db_template.updated_at, virtual_path: virtual_path(path, partial) } - [ActionView::Template.new(@db_template.body, "DB Template - #{@db_template.id}", handler, template_details)] + [ActionView::Template.new(@db_template.body, "DB Template - #{@db_template.id}", handler, **template_details)] end private diff --git a/app/views/devise/mailer/confirmation_instructions.html.erb b/app/views/devise/mailer/confirmation_instructions.html.erb index c03e5dc3c..79f838dbb 100644 --- a/app/views/devise/mailer/confirmation_instructions.html.erb +++ b/app/views/devise/mailer/confirmation_instructions.html.erb @@ -1,7 +1,8 @@

Welcome, <%= @resource.name %>!

-<% if @resource&.inviter.present? %> -

<%= @resource.inviter.name %>, with <%= @resource.inviter.account.name %>, has invited you to try out Chatwoot!

+<% account_user = @resource&.account_users&.first %> +<% if account_user&.inviter.present? %> +

<%= account_user.inviter.name %>, with <%= account_user.account.name %>, has invited you to try out Chatwoot!

<% end %>

You can confirm your account email through the link below:

diff --git a/app/workers/conversation_reply_email_worker.rb b/app/workers/conversation_reply_email_worker.rb index c352711b0..1f97b43ee 100644 --- a/app/workers/conversation_reply_email_worker.rb +++ b/app/workers/conversation_reply_email_worker.rb @@ -8,9 +8,9 @@ class ConversationReplyEmailWorker # send the email if @conversation.messages.incoming&.last&.content_type == 'incoming_email' || email_inbox? - ConversationReplyMailer.reply_without_summary(@conversation, queued_time).deliver_later + ConversationReplyMailer.with(account: @conversation.account).reply_without_summary(@conversation, queued_time).deliver_later else - ConversationReplyMailer.reply_with_summary(@conversation, queued_time).deliver_later + ConversationReplyMailer.with(account: @conversation.account).reply_with_summary(@conversation, queued_time).deliver_later end # delete the redis set from the first new message on the conversation diff --git a/db/seeds.rb b/db/seeds.rb index 8d47bded9..6fb8b7970 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -16,6 +16,10 @@ unless Rails.env.production? name: 'Acme Inc' ) + secondary_account = Account.create!( + name: 'Acme Org' + ) + user = User.new(name: 'John', email: 'john@acme.inc', password: 'Password1!') user.skip_confirmation! user.save! @@ -26,6 +30,18 @@ unless Rails.env.production? role: :administrator ) + AccountUser.create!( + account_id: secondary_account.id, + user_id: user.id, + role: :administrator + ) + + # Enables creating additional accounts from dashboard + installation_config = InstallationConfig.find_by(name: 'CREATE_NEW_ACCOUNT_FROM_DASHBOARD') + installation_config.value = true + installation_config.save! + GlobalConfig.clear_cache + web_widget = Channel::WebWidget.create!(account: account, website_url: 'https://acme.inc') inbox = Inbox.create!(channel: web_widget, account: account, name: 'Acme Support') diff --git a/spec/controllers/api/v1/accounts/conversations_controller_spec.rb b/spec/controllers/api/v1/accounts/conversations_controller_spec.rb index e890ef295..2ddd3f31a 100644 --- a/spec/controllers/api/v1/accounts/conversations_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/conversations_controller_spec.rb @@ -374,14 +374,24 @@ RSpec.describe 'Conversations API', type: :request do let(:params) { { email: 'test@test.com' } } it 'mutes conversation' do - allow(ConversationReplyMailer).to receive(:conversation_transcript) + mailer = double + allow(ConversationReplyMailer).to receive(:with).and_return(mailer) + allow(mailer).to receive(:conversation_transcript) post "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}/transcript", headers: agent.create_new_auth_token, params: params, as: :json expect(response).to have_http_status(:success) - expect(ConversationReplyMailer).to have_received(:conversation_transcript).with(conversation, 'test@test.com') + expect(mailer).to have_received(:conversation_transcript).with(conversation, 'test@test.com') + end + + it 'renders error when parameter missing' do + post "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}/transcript", + headers: agent.create_new_auth_token, + params: {}, + as: :json + expect(response).to have_http_status(:unprocessable_entity) end end end diff --git a/spec/controllers/api/v1/widget/conversations_controller_spec.rb b/spec/controllers/api/v1/widget/conversations_controller_spec.rb index 9ba2d066a..2603d1e15 100644 --- a/spec/controllers/api/v1/widget/conversations_controller_spec.rb +++ b/spec/controllers/api/v1/widget/conversations_controller_spec.rb @@ -104,7 +104,9 @@ RSpec.describe '/api/v1/widget/conversations/toggle_typing', type: :request do describe 'POST /api/v1/widget/conversations/transcript' do context 'with a conversation' do it 'sends transcript email' do - allow(ConversationReplyMailer).to receive(:conversation_transcript) + mailer = double + allow(ConversationReplyMailer).to receive(:with).and_return(mailer) + allow(mailer).to receive(:conversation_transcript) post '/api/v1/widget/conversations/transcript', headers: { 'X-Auth-Token' => token }, @@ -112,7 +114,7 @@ RSpec.describe '/api/v1/widget/conversations/toggle_typing', type: :request do as: :json expect(response).to have_http_status(:success) - expect(ConversationReplyMailer).to have_received(:conversation_transcript).with(conversation, 'test@test.com') + expect(mailer).to have_received(:conversation_transcript).with(conversation, 'test@test.com') end end end diff --git a/spec/lib/email_templates/db_resolver_service_spec.rb b/spec/lib/email_templates/db_resolver_service_spec.rb index 7bbb5c508..8648d8514 100644 --- a/spec/lib/email_templates/db_resolver_service_spec.rb +++ b/spec/lib/email_templates/db_resolver_service_spec.rb @@ -15,43 +15,43 @@ describe ::EmailTemplates::DbResolverService do email_template = create(:email_template, name: 'test', body: 'test') handler = ActionView::Template.registered_template_handler(:liquid) template_details = { + locals: [], format: Mime['html'].to_sym, - updated_at: email_template.updated_at, virtual_path: 'test' } expect( - resolver.find_templates('test', '', false, []).first.to_json + resolver.find_templates('test', '', false, []).first.inspect ).to eq( ActionView::Template.new( email_template.body, - "DB Template - #{email_template.id}", handler, template_details - ).to_json + "DB Template - #{email_template.id}", handler, **template_details + ).inspect ) end end context 'when account template exists in db' do let(:account) { create(:account) } - let(:installation_template) { create(:email_template, name: 'test', body: 'test') } - let(:account_template) { create(:email_template, name: 'test', body: 'test2', account: account) } + let!(:installation_template) { create(:email_template, name: 'test', body: 'test') } + let!(:account_template) { create(:email_template, name: 'test', body: 'test2', account: account) } it 'return account template for current account' do Current.account = account handler = ActionView::Template.registered_template_handler(:liquid) template_details = { + locals: [], format: Mime['html'].to_sym, - updated_at: account_template.updated_at, virtual_path: 'test' } expect( - resolver.find_templates('test', '', false, []).first.to_json + resolver.find_templates('test', '', false, []).first.inspect ).to eq( ActionView::Template.new( account_template.body, - "DB Template - #{account_template.id}", handler, template_details - ).to_json + "DB Template - #{account_template.id}", handler, **template_details + ).inspect ) Current.account = nil end @@ -60,18 +60,18 @@ describe ::EmailTemplates::DbResolverService do Current.account = create(:account) handler = ActionView::Template.registered_template_handler(:liquid) template_details = { + locals: [], format: Mime['html'].to_sym, - updated_at: installation_template.updated_at, virtual_path: 'test' } expect( - resolver.find_templates('test', '', false, []).first.to_json + resolver.find_templates('test', '', false, []).first.inspect ).to eq( ActionView::Template.new( installation_template.body, - "DB Template - #{installation_template.id}", handler, template_details - ).to_json + "DB Template - #{installation_template.id}", handler, **template_details + ).inspect ) Current.account = nil end diff --git a/spec/mailers/confirmation_instructions_spec.rb b/spec/mailers/confirmation_instructions_spec.rb index 9914c1585..8310ab714 100644 --- a/spec/mailers/confirmation_instructions_spec.rb +++ b/spec/mailers/confirmation_instructions_spec.rb @@ -5,7 +5,7 @@ require 'rails_helper' RSpec.describe 'Confirmation Instructions', type: :mailer do describe :notify do let(:account) { create(:account) } - let(:confirmable_user) { build(:user, inviter: inviter_val, account: account) } + let(:confirmable_user) { create(:user, inviter: inviter_val, account: account) } let(:inviter_val) { nil } let(:mail) { Devise::Mailer.confirmation_instructions(confirmable_user, nil, {}) } @@ -27,11 +27,9 @@ RSpec.describe 'Confirmation Instructions', type: :mailer do let(:inviter_val) { create(:user, :administrator, skip_confirmation: true, account: account) } it 'refers to the inviter and their account' do - Current.account = account expect(mail.body).to match( - "#{CGI.escapeHTML(inviter_val.name)}, with #{CGI.escapeHTML(inviter_val.account.name)}, has invited you to try out Chatwoot!" + "#{CGI.escapeHTML(inviter_val.name)}, with #{CGI.escapeHTML(account.name)}, has invited you to try out Chatwoot!" ) - Current.account = nil end end end diff --git a/spec/workers/conversation_reply_email_worker_spec.rb b/spec/workers/conversation_reply_email_worker_spec.rb index 11be19b3f..469431c61 100644 --- a/spec/workers/conversation_reply_email_worker_spec.rb +++ b/spec/workers/conversation_reply_email_worker_spec.rb @@ -6,15 +6,18 @@ RSpec.describe ConversationReplyEmailWorker, type: :worker do let(:scheduled_job) { described_class.perform_at(perform_at, 1, Time.zone.now) } let(:conversation) { build(:conversation, display_id: nil) } let(:message) { build(:message, conversation: conversation, content_type: 'incoming_email', inbox: conversation.inbox) } + let(:mailer) { double } + let(:mailer_action) { double } describe 'testing ConversationSummaryEmailWorker' do before do conversation.save! allow(Conversation).to receive(:find).and_return(conversation) - mailer = double - allow(ConversationReplyMailer).to receive(:reply_with_summary).and_return(mailer) - allow(ConversationReplyMailer).to receive(:reply_without_summary).and_return(mailer) - allow(mailer).to receive(:deliver_later).and_return(true) + allow(ConversationReplyMailer).to receive(:with).and_return(mailer) + allow(ConversationReplyMailer).to receive(:with).and_return(mailer) + allow(mailer).to receive(:reply_with_summary).and_return(mailer_action) + allow(mailer).to receive(:reply_without_summary).and_return(mailer_action) + allow(mailer_action).to receive(:deliver_later).and_return(true) end it 'worker jobs are enqueued in the mailers queue' do @@ -32,13 +35,13 @@ RSpec.describe ConversationReplyEmailWorker, type: :worker do context 'with actions performed by the worker' do it 'calls ConversationSummaryMailer#reply_with_summary when last incoming message was not email' do described_class.new.perform(1, Time.zone.now) - expect(ConversationReplyMailer).to have_received(:reply_with_summary) + expect(mailer).to have_received(:reply_with_summary) end it 'calls ConversationSummaryMailer#reply_without_summary when last incoming message was from email' do message.save described_class.new.perform(1, Time.zone.now) - expect(ConversationReplyMailer).to have_received(:reply_without_summary) + expect(mailer).to have_received(:reply_without_summary) end end end