From 82de815a2b01218d3596cc3f05376decd16d8ee9 Mon Sep 17 00:00:00 2001 From: Marc Anguera Insa Date: Sun, 1 Mar 2020 22:32:58 +0100 Subject: [PATCH 1/3] send OrganizationNotifier.recent_posts in user's locale --- app/jobs/organization_notifier_job.rb | 14 ++++++- app/mailers/organization_notifier.rb | 13 ++---- spec/mailers/organization_notifier_spec.rb | 48 ++++------------------ 3 files changed, 25 insertions(+), 50 deletions(-) diff --git a/app/jobs/organization_notifier_job.rb b/app/jobs/organization_notifier_job.rb index 1bd46729c..b4e0e5db4 100644 --- a/app/jobs/organization_notifier_job.rb +++ b/app/jobs/organization_notifier_job.rb @@ -2,7 +2,7 @@ # # Strategy: go throught all organizations and take latest active posts from last week # posted by active members. Send an email to all active and online members -# with the email notifications enabled with those posts. +# with the email notifications enabled with those posts. Group emails by user's locale. # Schedule defined in config/schedule.yml file. @@ -12,9 +12,19 @@ class OrganizationNotifierJob < ActiveJob::Base def perform Organization.all.find_each do |org| posts = org.posts.active.of_active_members.from_last_week + if posts.present? - OrganizationNotifier.recent_posts(posts).deliver_now + users_by_locale(org).each do |locale, users| + OrganizationNotifier.recent_posts(posts, locale, users).deliver_now + end end end end + + private + + def users_by_locale(organization) + organization.users.online_active.actives. + notifications.group_by { |u| u.locale || I18n.default_locale } + end end diff --git a/app/mailers/organization_notifier.rb b/app/mailers/organization_notifier.rb index 8b355a003..87f3119a3 100644 --- a/app/mailers/organization_notifier.rb +++ b/app/mailers/organization_notifier.rb @@ -6,20 +6,15 @@ class OrganizationNotifier < ActionMailer::Base # # en.organization_notifier.recent_posts.subject # - def recent_posts(posts) + def recent_posts(posts, locale, users) # last 10 posts of offers and inquiries @offers = posts.where(type: "Offer").take(10) @inquiries = posts.where(type: "Inquiry").take(10) @organization_name = posts.take.organization.name - mail(bcc: emails_newsletter(posts)) - end - - private - - def emails_newsletter(posts) - posts.take.organization.users.online_active.actives. - notifications.pluck(:email) + I18n.with_locale(locale) do + mail(bcc: users.map(&:email)) + end end end diff --git a/spec/mailers/organization_notifier_spec.rb b/spec/mailers/organization_notifier_spec.rb index 52bfec384..4b0be3816 100644 --- a/spec/mailers/organization_notifier_spec.rb +++ b/spec/mailers/organization_notifier_spec.rb @@ -1,52 +1,22 @@ require "spec_helper" RSpec.describe OrganizationNotifier do - let (:test_organization) { Fabricate(:organization) } - let! (:offer) { Fabricate(:offer, organization: test_organization) } - let! (:inquiry) { Fabricate(:inquiry, organization: test_organization) } - let (:user) do - Fabricate(:user, sign_in_count: 2, email: "user@example.com") - end - let (:another_user) { Fabricate(:user, sign_in_count: 1) } - let (:yet_another_user) { Fabricate(:user, sign_in_count: 0) } - let! (:member) do - Fabricate(:member, - organization: test_organization, - user: user, - active: true) - end - let! (:another_member) do - Fabricate(:member, - organization: test_organization, - user: another_user, - active: false) - end - let! (:yet_another_member) do - Fabricate(:member, - organization: test_organization, - user: yet_another_user, - active: true) - end - - before(:each) do - ActionMailer::Base.delivery_method = :test - ActionMailer::Base.perform_deliveries = true - ActionMailer::Base.deliveries = [] - OrganizationNotifier.recent_posts(test_organization.posts).deliver_now - end - - after(:each) do - ActionMailer::Base.deliveries.clear - end + let(:test_organization) { Fabricate(:organization) } + let!(:offer) { Fabricate(:offer, organization: test_organization) } + let!(:inquiry) { Fabricate(:inquiry, organization: test_organization) } + let(:user) { Fabricate(:user, email: "user@example.com", locale: :en) } + let(:member) { Fabricate(:member, organization: test_organization, user: user) } describe "send an email" do it "should send an email" do - expect(ActionMailer::Base.deliveries.count).to eq(1) + expect { + OrganizationNotifier.recent_posts(test_organization.posts, :en, [user]).deliver_now + }.to change { ActionMailer::Base.deliveries.count }.by(1) end end describe "recent posts" do - let(:mail) { OrganizationNotifier.recent_posts(test_organization.posts) } + let(:mail) { OrganizationNotifier.recent_posts(test_organization.posts, :en, [user]) } it "receive email only active and online users" do expect(mail.bcc).to eql(["user@example.com"]) From 8f62ec69bd9f56b597936338ee052bf56e000c19 Mon Sep 17 00:00:00 2001 From: Marc Anguera Insa Date: Mon, 16 Mar 2020 19:36:33 +0100 Subject: [PATCH 2/3] OrganizationNotifierJob: change strategy from group_by to 1 query per org locale --- app/jobs/organization_notifier_job.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/jobs/organization_notifier_job.rb b/app/jobs/organization_notifier_job.rb index b4e0e5db4..e8949bba6 100644 --- a/app/jobs/organization_notifier_job.rb +++ b/app/jobs/organization_notifier_job.rb @@ -24,7 +24,11 @@ def perform private def users_by_locale(organization) - organization.users.online_active.actives. - notifications.group_by { |u| u.locale || I18n.default_locale } + with_notifications = organization.users.online_active.actives.notifications + org_locales = with_notifications.pluck(:locale).uniq + + org_locales.each_with_object({}) do |locale, hash| + hash[locale] = with_notifications.where(locale: locale) + end end end From 39295b8ef3ac3fcca8f12f812ea268c9edb2ce50 Mon Sep 17 00:00:00 2001 From: Marc Anguera Insa Date: Mon, 16 Mar 2020 21:13:32 +0100 Subject: [PATCH 3/3] add specs for OrganizationNotifierJob --- app/mailers/organization_notifier.rb | 5 ----- spec/jobs/organization_notifier_job_spec.rb | 19 +++++++++++++++++++ spec/mailers/organization_notifier_spec.rb | 2 +- 3 files changed, 20 insertions(+), 6 deletions(-) create mode 100644 spec/jobs/organization_notifier_job_spec.rb diff --git a/app/mailers/organization_notifier.rb b/app/mailers/organization_notifier.rb index 87f3119a3..44117b519 100644 --- a/app/mailers/organization_notifier.rb +++ b/app/mailers/organization_notifier.rb @@ -1,11 +1,6 @@ class OrganizationNotifier < ActionMailer::Base default from: "\"TimeOverflow\" " - # Subject can be set in your I18n file at config/locales/en.yml - # with the following lookup: - # - # en.organization_notifier.recent_posts.subject - # def recent_posts(posts, locale, users) # last 10 posts of offers and inquiries @offers = posts.where(type: "Offer").take(10) diff --git a/spec/jobs/organization_notifier_job_spec.rb b/spec/jobs/organization_notifier_job_spec.rb new file mode 100644 index 000000000..ce5dec963 --- /dev/null +++ b/spec/jobs/organization_notifier_job_spec.rb @@ -0,0 +1,19 @@ +require 'spec_helper' + +RSpec.describe OrganizationNotifierJob, type: :job do + let!(:org) { Fabricate(:organization) } + let!(:user) { Fabricate(:user, locale: :en, sign_in_count: 1) } + let!(:member) { Fabricate(:member, organization: org, user: user) } + let!(:user2) { Fabricate(:user, locale: :ca, sign_in_count: 2) } + let!(:member2) { Fabricate(:member, organization: org, user: user2) } + let!(:offer) { Fabricate(:offer, organization: org, user: user) } + let!(:inquiry) { Fabricate(:inquiry, organization: org, user: user2) } + + describe '#perform' do + it "should send emails in user's locale" do + expect { + OrganizationNotifierJob.perform_now + }.to change { ActionMailer::Base.deliveries.count }.by(2) + end + end +end diff --git a/spec/mailers/organization_notifier_spec.rb b/spec/mailers/organization_notifier_spec.rb index 4b0be3816..4582c1341 100644 --- a/spec/mailers/organization_notifier_spec.rb +++ b/spec/mailers/organization_notifier_spec.rb @@ -24,7 +24,7 @@ it "to should be null" do expect(mail.to).to be_nil end - it "assigns @organization_name" do + it "body contains organization name" do expect(mail.body.encoded).to match(test_organization.name) end end