From 68a228120301f6400ea09a4f3f609b65eb643be3 Mon Sep 17 00:00:00 2001 From: rgarcia Date: Tue, 24 Jul 2018 20:58:51 +0200 Subject: [PATCH] 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}"))