From 68a228120301f6400ea09a4f3f609b65eb643be3 Mon Sep 17 00:00:00 2001 From: rgarcia Date: Tue, 24 Jul 2018 20:58:51 +0200 Subject: [PATCH 1/7] Refactor segment constant into a class method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We're going to make it dynamic using the geozones. Besides, class methods can be overwritten using custom models, while constants can't be overwritten without getting a warning [1]. Makes the definition of segments with geozones a little cleaner. I think it’s worth it, compared to the slight memory gain of using a constant [2]. [1] warning: already initialized constant UserSegments::SEGMENTS [2] https://stackoverflow.com/questions/15903835/class-method-vs-constant-in-ruby-rails#answer-15903970 --- app/helpers/user_segments_helper.rb | 2 +- db/dev_seeds/newsletters.rb | 2 +- lib/user_segments.rb | 20 ++++++++++--------- spec/factories/emails.rb | 2 +- spec/factories/notifications.rb | 2 +- spec/system/admin/admin_notifications_spec.rb | 2 +- spec/system/admin/emails/newsletters_spec.rb | 2 +- 7 files changed, 17 insertions(+), 15 deletions(-) diff --git a/app/helpers/user_segments_helper.rb b/app/helpers/user_segments_helper.rb index 72925c6b1..16ee9e923 100644 --- a/app/helpers/user_segments_helper.rb +++ b/app/helpers/user_segments_helper.rb @@ -1,6 +1,6 @@ module UserSegmentsHelper def user_segments_options - UserSegments::SEGMENTS.map do |user_segment_name| + UserSegments.segments.map do |user_segment_name| [t("admin.segment_recipient.#{user_segment_name}"), user_segment_name] end end diff --git a/db/dev_seeds/newsletters.rb b/db/dev_seeds/newsletters.rb index 3105a108e..e196f8a0a 100644 --- a/db/dev_seeds/newsletters.rb +++ b/db/dev_seeds/newsletters.rb @@ -14,7 +14,7 @@ section "Creating Newsletters" do 5.times do |n| Newsletter.create!( subject: "Newsletter subject #{n}", - segment_recipient: UserSegments::SEGMENTS.sample, + segment_recipient: UserSegments.segments.sample, from: "no-reply@consul.dev", body: newsletter_body.sample, sent_at: [Time.current, nil].sample diff --git a/lib/user_segments.rb b/lib/user_segments.rb index efa0d51a4..03a0497c9 100644 --- a/lib/user_segments.rb +++ b/lib/user_segments.rb @@ -1,13 +1,15 @@ class UserSegments - SEGMENTS = %w[all_users - administrators - all_proposal_authors - proposal_authors - investment_authors - feasible_and_undecided_investment_authors - selected_investment_authors - winner_investment_authors - not_supported_on_current_budget].freeze + def self.segments + %w[all_users + administrators + all_proposal_authors + proposal_authors + investment_authors + feasible_and_undecided_investment_authors + selected_investment_authors + winner_investment_authors + not_supported_on_current_budget].freeze + end def self.all_users User.active.where.not(confirmed_at: nil) diff --git a/spec/factories/emails.rb b/spec/factories/emails.rb index 3ed748737..9ddb6c9bb 100644 --- a/spec/factories/emails.rb +++ b/spec/factories/emails.rb @@ -1,7 +1,7 @@ FactoryBot.define do factory :newsletter do sequence(:subject) { |n| "Subject #{n}" } - segment_recipient { UserSegments::SEGMENTS.sample } + segment_recipient { UserSegments.segments.sample } sequence(:from) { |n| "noreply#{n}@consul.dev" } sequence(:body) { |n| "Body #{n}" } end diff --git a/spec/factories/notifications.rb b/spec/factories/notifications.rb index 8b300f85c..b0d8eaecc 100644 --- a/spec/factories/notifications.rb +++ b/spec/factories/notifications.rb @@ -24,7 +24,7 @@ FactoryBot.define do sequence(:title) { |n| "Admin Notification title #{n}" } sequence(:body) { |n| "Admin Notification body #{n}" } link { nil } - segment_recipient { UserSegments::SEGMENTS.sample } + segment_recipient { UserSegments.segments.sample } recipients_count { nil } sent_at { nil } diff --git a/spec/system/admin/admin_notifications_spec.rb b/spec/system/admin/admin_notifications_spec.rb index 901342fad..596ecd3c5 100644 --- a/spec/system/admin/admin_notifications_spec.rb +++ b/spec/system/admin/admin_notifications_spec.rb @@ -215,7 +215,7 @@ describe "Admin Notifications", :admin do end scenario "Select list of users to send notification" do - UserSegments::SEGMENTS.each do |user_segment| + UserSegments.segments.each do |user_segment| segment_recipient = I18n.t("admin.segment_recipient.#{user_segment}") visit new_admin_admin_notification_path diff --git a/spec/system/admin/emails/newsletters_spec.rb b/spec/system/admin/emails/newsletters_spec.rb index 91f391c33..a64f2d987 100644 --- a/spec/system/admin/emails/newsletters_spec.rb +++ b/spec/system/admin/emails/newsletters_spec.rb @@ -162,7 +162,7 @@ describe "Admin newsletter emails", :admin do end scenario "Select list of users to send newsletter" do - UserSegments::SEGMENTS.each do |user_segment| + UserSegments.segments.each do |user_segment| visit new_admin_newsletter_path fill_in_newsletter_form(segment_recipient: I18n.t("admin.segment_recipient.#{user_segment}")) From a0416d4d857546bf08eba38db36321e8f69e49f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 10 Nov 2021 20:08:59 +0100 Subject: [PATCH 2/7] Fix method description in user segments tests We generally use "#" to describe instance methods and "." to describe class methods. --- spec/lib/user_segments_spec.rb | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/spec/lib/user_segments_spec.rb b/spec/lib/user_segments_spec.rb index f59f55968..8d7f6eceb 100644 --- a/spec/lib/user_segments_spec.rb +++ b/spec/lib/user_segments_spec.rb @@ -5,7 +5,7 @@ describe UserSegments do let(:user2) { create(:user) } let(:user3) { create(:user) } - describe "#all_users" do + describe ".all_users" do it "returns all active users enabled" do active_user = create(:user) erased_user = create(:user, erased_at: Time.current) @@ -15,7 +15,7 @@ describe UserSegments do end end - describe "#administrators" do + describe ".administrators" do it "returns all active administrators users" do active_user = create(:user) active_admin = create(:administrator).user @@ -27,7 +27,7 @@ describe UserSegments do end end - describe "#all_proposal_authors" do + describe ".all_proposal_authors" do it "returns users that have created a proposal even if is archived or retired" do create(:proposal, author: user1) create(:proposal, :archived, author: user2) @@ -49,7 +49,7 @@ describe UserSegments do end end - describe "#proposal_authors" do + describe ".proposal_authors" do it "returns users that have created a proposal" do create(:proposal, author: user1) @@ -67,7 +67,7 @@ describe UserSegments do end end - describe "#investment_authors" do + describe ".investment_authors" do it "returns users that have created a budget investment" do investment = create(:budget_investment, author: user1) budget = create(:budget) @@ -90,7 +90,7 @@ describe UserSegments do end end - describe "#feasible_and_undecided_investment_authors" do + describe ".feasible_and_undecided_investment_authors" do it "returns authors of a feasible or an undecided budget investment" do user4 = create(:user) user5 = create(:user) @@ -128,7 +128,7 @@ describe UserSegments do end end - describe "#selected_investment_authors" do + describe ".selected_investment_authors" do it "returns authors of selected budget investments" do selected_investment = create(:budget_investment, :selected, author: user1) unselected_investment = create(:budget_investment, :unselected, author: user2) @@ -153,7 +153,7 @@ describe UserSegments do end end - describe "#winner_investment_authors" do + describe ".winner_investment_authors" do it "returns authors of winner budget investments" do winner_investment = create(:budget_investment, :winner, author: user1) selected_investment = create(:budget_investment, :selected, author: user2) @@ -178,7 +178,7 @@ describe UserSegments do end end - describe "#current_budget_investments" do + describe ".current_budget_investments" do it "only returns investments from the current budget" do investment1 = create(:budget_investment, author: create(:user)) investment2 = create(:budget_investment, author: create(:user)) @@ -192,7 +192,7 @@ describe UserSegments do end end - describe "#not_supported_on_current_budget" do + describe ".not_supported_on_current_budget" do it "only returns users that haven't supported investments on current budget" do investment1 = create(:budget_investment) investment2 = create(:budget_investment) @@ -209,7 +209,7 @@ describe UserSegments do end end - describe "#user_segment_emails" do + describe ".user_segment_emails" do it "returns list of emails sorted by user creation date" do create(:user, email: "first@email.com", created_at: 1.day.ago) create(:user, email: "last@email.com") From 78e543f6d33fd81b526f8363cb305a20066bf1ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 10 Nov 2021 20:35:46 +0100 Subject: [PATCH 3/7] Extract method to get a user segment name We're going to add geozones as user segments, so it's handy to have the method in the UserSegments class. We're also changing the `user_segment_emails` parameter name for consistency and simplicity. --- .../admin/emails_download_controller.rb | 2 +- app/helpers/user_segments_helper.rb | 8 ++------ config/i18n-tasks.yml | 1 + lib/user_segments.rb | 8 ++++++-- spec/lib/user_segments_spec.rb | 16 ++++++++++++++++ spec/system/admin/admin_notifications_spec.rb | 6 +++--- spec/system/admin/emails/newsletters_spec.rb | 13 +++++++------ 7 files changed, 36 insertions(+), 18 deletions(-) diff --git a/app/controllers/admin/emails_download_controller.rb b/app/controllers/admin/emails_download_controller.rb index 3bafa7668..b05ad83b7 100644 --- a/app/controllers/admin/emails_download_controller.rb +++ b/app/controllers/admin/emails_download_controller.rb @@ -4,7 +4,7 @@ class Admin::EmailsDownloadController < Admin::BaseController def generate_csv users_segment = params[:users_segment] - filename = t("admin.segment_recipient.#{users_segment}") + filename = UserSegments.segment_name(users_segment) csv_file = users_segment_emails_csv(users_segment) send_data csv_file, filename: "#{filename}.csv" diff --git a/app/helpers/user_segments_helper.rb b/app/helpers/user_segments_helper.rb index 16ee9e923..3f4d5e2e1 100644 --- a/app/helpers/user_segments_helper.rb +++ b/app/helpers/user_segments_helper.rb @@ -1,15 +1,11 @@ module UserSegmentsHelper def user_segments_options UserSegments.segments.map do |user_segment_name| - [t("admin.segment_recipient.#{user_segment_name}"), user_segment_name] + [segment_name(user_segment_name), user_segment_name] end end def segment_name(user_segment) - if user_segment && UserSegments.respond_to?(user_segment) - I18n.t("admin.segment_recipient.#{user_segment}") - else - I18n.t("admin.segment_recipient.invalid_recipients_segment") - end + UserSegments.segment_name(user_segment) || I18n.t("admin.segment_recipient.invalid_recipients_segment") end end diff --git a/config/i18n-tasks.yml b/config/i18n-tasks.yml index 2cf5a6e35..b5932360a 100644 --- a/config/i18n-tasks.yml +++ b/config/i18n-tasks.yml @@ -81,6 +81,7 @@ search: - app/ - db/pages/ - db/dev_seeds/ + - lib/ ## Root directories for relative keys resolution. # relative_roots: diff --git a/lib/user_segments.rb b/lib/user_segments.rb index 03a0497c9..b125f42c2 100644 --- a/lib/user_segments.rb +++ b/lib/user_segments.rb @@ -11,6 +11,10 @@ class UserSegments not_supported_on_current_budget].freeze end + def self.segment_name(segment) + I18n.t("admin.segment_recipient.#{segment}") if segments.include?(segment.to_s) + end + def self.all_users User.active.where.not(confirmed_at: nil) end @@ -53,8 +57,8 @@ class UserSegments ) end - def self.user_segment_emails(users_segment) - UserSegments.send(users_segment).newsletter.order(:created_at).pluck(:email).compact + def self.user_segment_emails(segment) + UserSegments.send(segment).newsletter.order(:created_at).pluck(:email).compact end private diff --git a/spec/lib/user_segments_spec.rb b/spec/lib/user_segments_spec.rb index 8d7f6eceb..01194f42e 100644 --- a/spec/lib/user_segments_spec.rb +++ b/spec/lib/user_segments_spec.rb @@ -5,6 +5,22 @@ describe UserSegments do let(:user2) { create(:user) } let(:user3) { create(:user) } + describe ".segment_name" do + it "returns a readable name of the segment" do + expect(UserSegments.segment_name("all_users")).to eq "All users" + expect(UserSegments.segment_name("administrators")).to eq "Administrators" + expect(UserSegments.segment_name("proposal_authors")).to eq "Proposal authors" + end + + it "accepts symbols as parameters" do + expect(UserSegments.segment_name(:all_users)).to eq "All users" + end + + it "returns nil for invalid segments" do + expect(UserSegments.segment_name("invalid")).to be nil + end + end + describe ".all_users" do it "returns all active users enabled" do active_user = create(:user) diff --git a/spec/system/admin/admin_notifications_spec.rb b/spec/system/admin/admin_notifications_spec.rb index 596ecd3c5..9a3adaf49 100644 --- a/spec/system/admin/admin_notifications_spec.rb +++ b/spec/system/admin/admin_notifications_spec.rb @@ -215,15 +215,15 @@ describe "Admin Notifications", :admin do end scenario "Select list of users to send notification" do - UserSegments.segments.each do |user_segment| - segment_recipient = I18n.t("admin.segment_recipient.#{user_segment}") + UserSegments.segments.each do |segment| + segment_recipient = UserSegments.segment_name(segment) visit new_admin_admin_notification_path fill_in_admin_notification_form(segment_recipient: segment_recipient) click_button "Create notification" - expect(page).to have_content(I18n.t("admin.segment_recipient.#{user_segment}")) + expect(page).to have_content segment_recipient end end end diff --git a/spec/system/admin/emails/newsletters_spec.rb b/spec/system/admin/emails/newsletters_spec.rb index a64f2d987..3af4f19d1 100644 --- a/spec/system/admin/emails/newsletters_spec.rb +++ b/spec/system/admin/emails/newsletters_spec.rb @@ -16,7 +16,7 @@ describe "Admin newsletter emails", :admin do expect(page).to have_link "Go back", href: admin_newsletters_path expect(page).to have_content "This is a subject" - expect(page).to have_content I18n.t("admin.segment_recipient.#{newsletter.segment_recipient}") + expect(page).to have_content "All users" expect(page).to have_content "no-reply@consul.dev" expect(page).to have_content "This is a body" end @@ -40,10 +40,9 @@ describe "Admin newsletter emails", :admin do expect(page).to have_css(".newsletter", count: 3) newsletters.each do |newsletter| - segment_recipient = I18n.t("admin.segment_recipient.#{newsletter.segment_recipient}") within("#newsletter_#{newsletter.id}") do expect(page).to have_content newsletter.subject - expect(page).to have_content segment_recipient + expect(page).to have_content UserSegments.segment_name(newsletter.segment_recipient) end end end @@ -162,13 +161,15 @@ describe "Admin newsletter emails", :admin do end scenario "Select list of users to send newsletter" do - UserSegments.segments.each do |user_segment| + UserSegments.segments.each do |segment| + segment_recipient = UserSegments.segment_name(segment) + visit new_admin_newsletter_path - fill_in_newsletter_form(segment_recipient: I18n.t("admin.segment_recipient.#{user_segment}")) + fill_in_newsletter_form(segment_recipient: segment_recipient) click_button "Create Newsletter" - expect(page).to have_content(I18n.t("admin.segment_recipient.#{user_segment}")) + expect(page).to have_content segment_recipient end end end From 7a028411ab4efd41edb94922ff0920deb464a455 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 10 Nov 2021 20:45:00 +0100 Subject: [PATCH 4/7] Extract methods to get recipients and valid segments This way we don't have to use the `send` method in other places, like the AdminNotification class, and we can change the internal implementation at any point. --- app/models/admin_notification.rb | 4 ++-- app/models/newsletter.rb | 2 +- lib/user_segments.rb | 10 +++++++++- spec/lib/user_segments_spec.rb | 22 ++++++++++++++++++++++ 4 files changed, 34 insertions(+), 4 deletions(-) diff --git a/app/models/admin_notification.rb b/app/models/admin_notification.rb index 52c741c93..7b5e3b74e 100644 --- a/app/models/admin_notification.rb +++ b/app/models/admin_notification.rb @@ -13,11 +13,11 @@ class AdminNotification < ApplicationRecord before_validation :complete_link_url def list_of_recipients - UserSegments.send(segment_recipient) if valid_segment_recipient? + UserSegments.recipients(segment_recipient) if valid_segment_recipient? end def valid_segment_recipient? - segment_recipient && UserSegments.respond_to?(segment_recipient) + UserSegments.valid_segment?(segment_recipient) end def draft? diff --git a/app/models/newsletter.rb b/app/models/newsletter.rb index 7e8955931..d4095be1e 100644 --- a/app/models/newsletter.rb +++ b/app/models/newsletter.rb @@ -15,7 +15,7 @@ class Newsletter < ApplicationRecord end def valid_segment_recipient? - segment_recipient && UserSegments.respond_to?(segment_recipient) + UserSegments.valid_segment?(segment_recipient) end def draft? diff --git a/lib/user_segments.rb b/lib/user_segments.rb index b125f42c2..47cd36d13 100644 --- a/lib/user_segments.rb +++ b/lib/user_segments.rb @@ -57,8 +57,16 @@ class UserSegments ) end + def self.valid_segment?(segment) + segment && respond_to?(segment) + end + + def self.recipients(segment) + send(segment) + end + def self.user_segment_emails(segment) - UserSegments.send(segment).newsletter.order(:created_at).pluck(:email).compact + recipients(segment).newsletter.order(:created_at).pluck(:email).compact end private diff --git a/spec/lib/user_segments_spec.rb b/spec/lib/user_segments_spec.rb index 01194f42e..906827d7e 100644 --- a/spec/lib/user_segments_spec.rb +++ b/spec/lib/user_segments_spec.rb @@ -21,6 +21,28 @@ describe UserSegments do end end + describe ".valid_segment?" do + it "returns true when the segment exists" do + expect(UserSegments.valid_segment?("all_proposal_authors")).to be true + expect(UserSegments.valid_segment?("investment_authors")).to be true + expect(UserSegments.valid_segment?("feasible_and_undecided_investment_authors")).to be true + end + + it "accepts symbols as parameters" do + expect(UserSegments.valid_segment?(:selected_investment_authors)).to be true + expect(UserSegments.valid_segment?(:winner_investment_authors)).to be true + expect(UserSegments.valid_segment?(:not_supported_on_current_budget)).to be true + end + + it "is falsey when the segment doesn't exist" do + expect(UserSegments.valid_segment?("imaginary_segment")).to be_falsey + end + + it "is falsey when nil is passed" do + expect(UserSegments.valid_segment?(nil)).to be_falsey + end + end + describe ".all_users" do it "returns all active users enabled" do active_user = create(:user) From 25a8950330e84bf8173d56fa15f4092c1b24ad1c Mon Sep 17 00:00:00 2001 From: rgarcia Date: Tue, 24 Jul 2018 20:29:09 +0200 Subject: [PATCH 5/7] Add geozones as user segments --- lib/user_segments.rb | 16 +++- spec/lib/user_segments_spec.rb | 80 ++++++++++++++++++++ spec/system/admin/emails/newsletters_spec.rb | 23 ++++-- 3 files changed, 110 insertions(+), 9 deletions(-) diff --git a/lib/user_segments.rb b/lib/user_segments.rb index 47cd36d13..5ee5bfe49 100644 --- a/lib/user_segments.rb +++ b/lib/user_segments.rb @@ -8,11 +8,11 @@ class UserSegments feasible_and_undecided_investment_authors selected_investment_authors winner_investment_authors - not_supported_on_current_budget].freeze + not_supported_on_current_budget] + geozones.keys end def self.segment_name(segment) - I18n.t("admin.segment_recipient.#{segment}") if segments.include?(segment.to_s) + geozones[segment.to_s]&.name || I18n.t("admin.segment_recipient.#{segment}") if valid_segment?(segment) end def self.all_users @@ -58,11 +58,15 @@ class UserSegments end def self.valid_segment?(segment) - segment && respond_to?(segment) + segments.include?(segment.to_s) end def self.recipients(segment) - send(segment) + if geozones[segment.to_s] + all_users.where(geozone: geozones[segment.to_s]) + else + send(segment) + end end def self.user_segment_emails(segment) @@ -78,4 +82,8 @@ class UserSegments def self.author_ids(author_ids) all_users.where(id: author_ids) end + + def self.geozones + Geozone.order(:name).map { |geozone| [geozone.name.parameterize.underscore, geozone] }.to_h + end end diff --git a/spec/lib/user_segments_spec.rb b/spec/lib/user_segments_spec.rb index 906827d7e..4aa454cbe 100644 --- a/spec/lib/user_segments_spec.rb +++ b/spec/lib/user_segments_spec.rb @@ -19,6 +19,26 @@ describe UserSegments do it "returns nil for invalid segments" do expect(UserSegments.segment_name("invalid")).to be nil end + + context "with geozones in the database" do + before do + create(:geozone, name: "Lands and Borderlands") + create(:geozone, name: "Lowlands and Highlands") + end + + it "returns geozone names when the geozone exists" do + expect(UserSegments.segment_name("lands_and_borderlands")).to eq "Lands and Borderlands" + expect(UserSegments.segment_name("lowlands_and_highlands")).to eq "Lowlands and Highlands" + end + + it "returns regular segments when the geozone doesn't exist" do + expect(UserSegments.segment_name("all_users")).to eq "All users" + end + + it "returns nil for invalid segments" do + expect(UserSegments.segment_name("invalid")).to be nil + end + end end describe ".valid_segment?" do @@ -41,6 +61,30 @@ describe UserSegments do it "is falsey when nil is passed" do expect(UserSegments.valid_segment?(nil)).to be_falsey end + + context "with geozones in the database" do + before do + create(:geozone, name: "Lands and Borderlands") + create(:geozone, name: "Lowlands and Highlands") + end + + it "returns true when the geozone exists" do + expect(UserSegments.valid_segment?("lands_and_borderlands")).to be true + expect(UserSegments.valid_segment?("lowlands_and_highlands")).to be true + end + + it "returns true when the segment exists" do + expect(UserSegments.valid_segment?("all_users")).to be true + end + + it "is falsey when the segment doesn't exist" do + expect(UserSegments.valid_segment?("imaginary_segment")).to be_falsey + end + + it "is falsey when nil is passed" do + expect(UserSegments.valid_segment?(nil)).to be_falsey + end + end end describe ".all_users" do @@ -256,4 +300,40 @@ describe UserSegments do expect(emails).to eq ["first@email.com", "last@email.com"] end end + + context "Geozones" do + let!(:new_york) { create(:geozone, name: "New York") } + let!(:california) { create(:geozone, name: "California") } + let!(:user1) { create(:user, geozone: new_york) } + let!(:user2) { create(:user, geozone: new_york) } + let!(:user3) { create(:user, geozone: california) } + + before do + create(:geozone, name: "Mars") + create(:user, geozone: nil) + end + + it "includes geozones in available segments" do + expect(UserSegments.segments).to include("new_york") + expect(UserSegments.segments).to include("california") + expect(UserSegments.segments).to include("mars") + expect(UserSegments.segments).not_to include("jupiter") + end + + it "returns users of a geozone" do + expect(UserSegments.recipients("new_york")).to match_array [user1, user2] + expect(UserSegments.recipients("california")).to eq [user3] + end + + it "accepts symbols as parameters" do + expect(UserSegments.recipients(:new_york)).to match_array [user1, user2] + expect(UserSegments.recipients(:california)).to eq [user3] + end + + it "only returns active users of a geozone" do + user2.update!(erased_at: Time.current) + + expect(UserSegments.recipients("new_york")).to eq [user1] + end + end end diff --git a/spec/system/admin/emails/newsletters_spec.rb b/spec/system/admin/emails/newsletters_spec.rb index 3af4f19d1..1dcbaf4c5 100644 --- a/spec/system/admin/emails/newsletters_spec.rb +++ b/spec/system/admin/emails/newsletters_spec.rb @@ -160,16 +160,29 @@ describe "Admin newsletter emails", :admin do end end - scenario "Select list of users to send newsletter" do - UserSegments.segments.each do |segment| - segment_recipient = UserSegments.segment_name(segment) + describe "Select list of users to send newsletter" do + scenario "Custom user segments" do + UserSegments.segments.each do |segment| + segment_recipient = UserSegments.segment_name(segment) + + visit new_admin_newsletter_path + + fill_in_newsletter_form(segment_recipient: segment_recipient) + click_button "Create Newsletter" + + expect(page).to have_content segment_recipient + end + end + + scenario "Geozone segments" do + create(:geozone, name: "Queens and Brooklyn") visit new_admin_newsletter_path - fill_in_newsletter_form(segment_recipient: segment_recipient) + fill_in_newsletter_form(segment_recipient: "Queens and Brooklyn") click_button "Create Newsletter" - expect(page).to have_content segment_recipient + expect(page).to have_content "Queens and Brooklyn" end end end From 18910d09043989645b065a5bbdf7c24f56e5f7ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 10 Nov 2021 21:05:22 +0100 Subject: [PATCH 6/7] Reduce number of requests in user segments tests We were testing the creation of newsletters and admin notifications for each existing segment, which IMHO is a bit overkill, considering how slow system tests are. So far we don't have any reasons to believe creating newsletters and admin notifications will only work for some user segments, so we're testing a random one instead. Running these tests on my machine is now about 15 seconds faster. --- spec/system/admin/admin_notifications_spec.rb | 13 ++++++------- spec/system/admin/emails/newsletters_spec.rb | 13 ++++++------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/spec/system/admin/admin_notifications_spec.rb b/spec/system/admin/admin_notifications_spec.rb index 9a3adaf49..e6c2be180 100644 --- a/spec/system/admin/admin_notifications_spec.rb +++ b/spec/system/admin/admin_notifications_spec.rb @@ -215,15 +215,14 @@ describe "Admin Notifications", :admin do end scenario "Select list of users to send notification" do - UserSegments.segments.each do |segment| - segment_recipient = UserSegments.segment_name(segment) + segment = UserSegments.segments.sample + segment_recipient = UserSegments.segment_name(segment) - visit new_admin_admin_notification_path + visit new_admin_admin_notification_path - fill_in_admin_notification_form(segment_recipient: segment_recipient) - click_button "Create notification" + fill_in_admin_notification_form(segment_recipient: segment_recipient) + click_button "Create notification" - expect(page).to have_content segment_recipient - end + expect(page).to have_content segment_recipient end end diff --git a/spec/system/admin/emails/newsletters_spec.rb b/spec/system/admin/emails/newsletters_spec.rb index 1dcbaf4c5..ad09ed149 100644 --- a/spec/system/admin/emails/newsletters_spec.rb +++ b/spec/system/admin/emails/newsletters_spec.rb @@ -162,16 +162,15 @@ describe "Admin newsletter emails", :admin do describe "Select list of users to send newsletter" do scenario "Custom user segments" do - UserSegments.segments.each do |segment| - segment_recipient = UserSegments.segment_name(segment) + segment = UserSegments.segments.sample + segment_recipient = UserSegments.segment_name(segment) - visit new_admin_newsletter_path + visit new_admin_newsletter_path - fill_in_newsletter_form(segment_recipient: segment_recipient) - click_button "Create Newsletter" + fill_in_newsletter_form(segment_recipient: segment_recipient) + click_button "Create Newsletter" - expect(page).to have_content segment_recipient - end + expect(page).to have_content segment_recipient end scenario "Geozone segments" do From cd58b96fad79e15d23a0bbb63977ae13b8ffffef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 20 Dec 2021 16:22:14 +0100 Subject: [PATCH 7/7] Support geozone segments with non-Latin characters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `parameterize` method uses the `I18n.transliterate` method, whose documentation says: ``` I18n.transliterate("Ærøskøbing") => "AEroskobing" I18n.transliterate("日本語") => "???" ``` That means we can't use it for dictionaries where characters don't have a transliteration to the latin alphabet. So we're changing the code in order to only transliterate characters with a transliteration to the latin alphabet. Note the first example ("Česká republika") already worked with the previous code; the test has been added to make sure accented characters are handled properly. --- lib/user_segments.rb | 8 +++++++- spec/lib/user_segments_spec.rb | 10 ++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/lib/user_segments.rb b/lib/user_segments.rb index 5ee5bfe49..7e4e8a154 100644 --- a/lib/user_segments.rb +++ b/lib/user_segments.rb @@ -84,6 +84,12 @@ class UserSegments end def self.geozones - Geozone.order(:name).map { |geozone| [geozone.name.parameterize.underscore, geozone] }.to_h + Geozone.order(:name).map do |geozone| + [geozone.name.gsub(/./) { |char| character_approximation(char) }.underscore.tr(" ", "_"), geozone] + end.to_h + end + + def self.character_approximation(char) + I18n::Backend::Transliterator::HashTransliterator::DEFAULT_APPROXIMATIONS[char] || char end end diff --git a/spec/lib/user_segments_spec.rb b/spec/lib/user_segments_spec.rb index 4aa454cbe..79b6d176f 100644 --- a/spec/lib/user_segments_spec.rb +++ b/spec/lib/user_segments_spec.rb @@ -31,6 +31,16 @@ describe UserSegments do expect(UserSegments.segment_name("lowlands_and_highlands")).to eq "Lowlands and Highlands" end + it "supports international alphabets" do + create(:geozone, name: "Česká republika") + create(:geozone, name: "България") + create(:geozone, name: "日本") + + expect(UserSegments.segment_name("ceska_republika")).to eq "Česká republika" + expect(UserSegments.segment_name("България")).to eq "България" + expect(UserSegments.segment_name("日本")).to eq "日本" + end + it "returns regular segments when the geozone doesn't exist" do expect(UserSegments.segment_name("all_users")).to eq "All users" end