From bdbb32e82463fa4f9cf07621db6044a00fad7b92 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Tue, 20 Feb 2018 18:57:20 +0100 Subject: [PATCH 1/9] Move `newsletter` User scope outside UserSegments Why: UserSegments are not only used for Newsletters or Email downloads, but also for internal Global Notifications. We don't want to have that scope hardcoded inside UserSegments as users that have opted-out from the newsletter should still be recipients of global notifications. How: Removing the scope from the UserSegments `all_users` method that acts as base for all the other segments. Including that `newsletter` scope only on the places that is relevant: * When listing recipients for a newsletter * When downloading a listing emails that can be newsletter recipients Also updated relevant tests --- .../admin/emails_download_controller.rb | 2 +- app/models/newsletter.rb | 2 +- lib/user_segments.rb | 2 +- spec/lib/user_segments_spec.rb | 10 +++------- spec/models/newsletter_spec.rb | 18 ++++++++++++++++++ 5 files changed, 24 insertions(+), 10 deletions(-) diff --git a/app/controllers/admin/emails_download_controller.rb b/app/controllers/admin/emails_download_controller.rb index 326c32ff1..179240358 100644 --- a/app/controllers/admin/emails_download_controller.rb +++ b/app/controllers/admin/emails_download_controller.rb @@ -13,6 +13,6 @@ class Admin::EmailsDownloadController < Admin::BaseController private def users_segment_emails_csv(users_segment) - UserSegments.send(users_segment).pluck(:email).to_csv + UserSegments.send(users_segment).newsletter.pluck(:email).to_csv end end diff --git a/app/models/newsletter.rb b/app/models/newsletter.rb index 0b7bc4481..beaa59495 100644 --- a/app/models/newsletter.rb +++ b/app/models/newsletter.rb @@ -14,7 +14,7 @@ class Newsletter < ActiveRecord::Base validates_format_of :from, :with => /@/ def list_of_recipients - UserSegments.send(segment_recipient) + UserSegments.send(segment_recipient).newsletter end def draft? diff --git a/lib/user_segments.rb b/lib/user_segments.rb index 02fbe9b5f..8a50f6d31 100644 --- a/lib/user_segments.rb +++ b/lib/user_segments.rb @@ -7,7 +7,7 @@ class UserSegments winner_investment_authors) def self.all_users - User.newsletter.active + User.active end def self.proposal_authors diff --git a/spec/lib/user_segments_spec.rb b/spec/lib/user_segments_spec.rb index 77dff6a27..3914c11bf 100644 --- a/spec/lib/user_segments_spec.rb +++ b/spec/lib/user_segments_spec.rb @@ -6,15 +6,11 @@ describe UserSegments do let(:user3) { create(:user) } describe "#all_users" do - it "returns all active users with newsletter enabled" do - active_user1 = create(:user, newsletter: true) - active_user2 = create(:user, newsletter: true) - active_user3 = create(:user, newsletter: false) + it "returns all active users enabled" do + active_user = create(:user) erased_user = create(:user, erased_at: Time.current) - expect(described_class.all_users).to include active_user1 - expect(described_class.all_users).to include active_user2 - expect(described_class.all_users).not_to include active_user3 + expect(described_class.all_users).to include active_user expect(described_class.all_users).not_to include erased_user end end diff --git a/spec/models/newsletter_spec.rb b/spec/models/newsletter_spec.rb index 09d66055d..6ab83ffcd 100644 --- a/spec/models/newsletter_spec.rb +++ b/spec/models/newsletter_spec.rb @@ -17,6 +17,11 @@ describe Newsletter do expect(newsletter).not_to be_valid end + it 'is not valid with an inexistent user segment for segment_recipient' do + newsletter.segment_recipient = 'invalid_user_segment_name' + expect(newsletter).not_to be_valid + end + it 'is not valid without a from' do newsletter.from = nil expect(newsletter).not_to be_valid @@ -31,4 +36,17 @@ describe Newsletter do newsletter.from = "this_is_not_an_email" expect(newsletter).not_to be_valid end + + describe '#list_of_recipients' do + before do + create(:user, newsletter: true, username: 'newsletter_user') + create(:user, newsletter: false) + newsletter.update(segment_recipient: 'all_users') + end + + it 'returns list of recipients excluding users with disabled newsletter' do + expect(newsletter.list_of_recipients.count).to eq(1) + expect(newsletter.list_of_recipients.first.username).to eq('newsletter_user') + end + end end From 4becd0eb35d4490f2eccd124dc11ac0b3bc600cd Mon Sep 17 00:00:00 2001 From: Bertocq Date: Tue, 20 Feb 2018 22:38:49 +0100 Subject: [PATCH 2/9] Change Newsletter's segment_recipient to string Why: Newsletter attribute `segment_recipient` is an integer to be used as enum. There's no advantage to store a number instead of an string if the ammount of elements in the table is not going to be huge, or we can take advantage of using an enum. Also maintaining both Newsletters enum paired with UserSegments::SEGMENTS would be a maintenance burden. How: * Migration to change segment_recipient column from integer to string * Removing enumeration from Newsletter model class * Using UserSegments::SEGMENTS instead of Newsletter.segment_recipients or integer values --- app/controllers/admin/newsletters_controller.rb | 4 +--- app/helpers/user_segments_helper.rb | 7 +++++++ app/models/newsletter.rb | 6 ------ app/views/admin/emails_download/index.html.erb | 3 +-- app/views/admin/newsletters/_form.html.erb | 3 +-- db/dev_seeds/newsletters.rb | 2 +- ...11105_change_newsletter_segment_recipient_to_string.rb | 5 +++++ db/schema.rb | 4 ++-- spec/factories.rb | 6 +++--- spec/features/admin/emails/newsletters_spec.rb | 8 ++++---- 10 files changed, 25 insertions(+), 23 deletions(-) create mode 100644 app/helpers/user_segments_helper.rb create mode 100644 db/migrate/20180220211105_change_newsletter_segment_recipient_to_string.rb diff --git a/app/controllers/admin/newsletters_controller.rb b/app/controllers/admin/newsletters_controller.rb index 4dbf963f7..980125ee7 100644 --- a/app/controllers/admin/newsletters_controller.rb +++ b/app/controllers/admin/newsletters_controller.rb @@ -56,8 +56,6 @@ class Admin::NewslettersController < Admin::BaseController private def newsletter_params - newsletter_params = params.require(:newsletter) - .permit(:subject, :segment_recipient, :from, :body) - newsletter_params.merge(segment_recipient: newsletter_params[:segment_recipient].to_i) + params.require(:newsletter).permit(:subject, :segment_recipient, :from, :body) end end diff --git a/app/helpers/user_segments_helper.rb b/app/helpers/user_segments_helper.rb new file mode 100644 index 000000000..6431216ef --- /dev/null +++ b/app/helpers/user_segments_helper.rb @@ -0,0 +1,7 @@ +module UserSegmentsHelper + def user_segments_options + UserSegments::SEGMENTS.collect do |user_segment_name| + [t("admin.segment_recipient.#{user_segment_name}"), user_segment_name] + end + end +end diff --git a/app/models/newsletter.rb b/app/models/newsletter.rb index beaa59495..98b53d295 100644 --- a/app/models/newsletter.rb +++ b/app/models/newsletter.rb @@ -1,10 +1,4 @@ class Newsletter < ActiveRecord::Base - enum segment_recipient: { all_users: 1, - proposal_authors: 2, - investment_authors: 3, - feasible_and_undecided_investment_authors: 4, - selected_investment_authors: 5, - winner_investment_authors: 6 } validates :subject, presence: true validates :segment_recipient, presence: true diff --git a/app/views/admin/emails_download/index.html.erb b/app/views/admin/emails_download/index.html.erb index 20bdb1775..7c82a6844 100644 --- a/app/views/admin/emails_download/index.html.erb +++ b/app/views/admin/emails_download/index.html.erb @@ -10,8 +10,7 @@ <%= t('admin.emails_download.index.download_segment_help_text') %>

- <%= select_tag :users_segment, options_for_select(UserSegments::SEGMENTS - .collect { |s| [t("admin.segment_recipient.#{s}"), s] }) %> + <%= select_tag :users_segment, options_for_select(user_segments_options) %>
<%= submit_tag t('admin.emails_download.index.download_emails_button'), class: "button" %> diff --git a/app/views/admin/newsletters/_form.html.erb b/app/views/admin/newsletters/_form.html.erb index 2631cfc10..bf6b71d5b 100644 --- a/app/views/admin/newsletters/_form.html.erb +++ b/app/views/admin/newsletters/_form.html.erb @@ -1,8 +1,7 @@ <%= form_for [:admin, @newsletter] do |f| %> <%= render 'shared/errors', resource: @newsletter %> - <%= f.select :segment_recipient, options_for_select(Newsletter.segment_recipients - .collect { |k,v| [t("admin.segment_recipient.#{k}"), v] }, + <%= f.select :segment_recipient, options_for_select(user_segments_options, @newsletter[:segment_recipient]) %> <%= f.text_field :subject %> <%= f.text_field :from %> diff --git a/db/dev_seeds/newsletters.rb b/db/dev_seeds/newsletters.rb index be9074f10..2f70b52b6 100644 --- a/db/dev_seeds/newsletters.rb +++ b/db/dev_seeds/newsletters.rb @@ -8,7 +8,7 @@ section "Creating Newsletters" do 5.times do |n| Newsletter.create!( subject: "Newsletter subject #{n}", - segment_recipient: Newsletter.segment_recipients.values.sample, + segment_recipient: UserSegments::SEGMENTS.sample, from: 'no-reply@consul.dev', body: newsletter_body.sample, sent_at: [Time.now, nil].sample diff --git a/db/migrate/20180220211105_change_newsletter_segment_recipient_to_string.rb b/db/migrate/20180220211105_change_newsletter_segment_recipient_to_string.rb new file mode 100644 index 000000000..1f87a56f5 --- /dev/null +++ b/db/migrate/20180220211105_change_newsletter_segment_recipient_to_string.rb @@ -0,0 +1,5 @@ +class ChangeNewsletterSegmentRecipientToString < ActiveRecord::Migration + def change + change_column :newsletters, :segment_recipient, :string, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 25d241bd8..612ca8e1e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180205170054) do +ActiveRecord::Schema.define(version: 20180220211105) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -626,7 +626,7 @@ ActiveRecord::Schema.define(version: 20180205170054) do create_table "newsletters", force: :cascade do |t| t.string "subject" - t.integer "segment_recipient" + t.string "segment_recipient", null: false t.string "from" t.text "body" t.date "sent_at" diff --git a/spec/factories.rb b/spec/factories.rb index 106502fc6..5019f326a 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -945,9 +945,9 @@ LOREM_IPSUM factory :newsletter do sequence(:subject) { |n| "Subject #{n}" } - segment_recipient [1, 2, 3, 4, 5, 6].sample - sequence(:from) { |n| "noreply#{n}@consul.dev" } - sequence(:body) { |n| "Body #{n}" } + segment_recipient UserSegments::SEGMENTS.sample + sequence(:from) { |n| "noreply#{n}@consul.dev" } + sequence(:body) { |n| "Body #{n}" } end end diff --git a/spec/features/admin/emails/newsletters_spec.rb b/spec/features/admin/emails/newsletters_spec.rb index a73f4efc8..94f93556e 100644 --- a/spec/features/admin/emails/newsletters_spec.rb +++ b/spec/features/admin/emails/newsletters_spec.rb @@ -10,7 +10,7 @@ feature "Admin newsletter emails" do scenario "Show" do newsletter = create(:newsletter, subject: "This is a subject", - segment_recipient: 1, + segment_recipient: 'all_users', from: "no-reply@consul.dev", body: "This is a body") @@ -116,14 +116,14 @@ feature "Admin newsletter emails" do end scenario "Select list of users to send newsletter" do - Newsletter.segment_recipients.each_key do |user_group| + UserSegments::SEGMENTS.each do |user_segment| visit new_admin_newsletter_path fill_in_newsletter_form - select I18n.t("admin.segment_recipient.#{user_group}"), from: 'newsletter_segment_recipient' + select I18n.t("admin.segment_recipient.#{user_segment}"), from: 'newsletter_segment_recipient' click_button "Create Newsletter" - expect(page).to have_content(I18n.t("admin.segment_recipient.#{user_group}")) + expect(page).to have_content(I18n.t("admin.segment_recipient.#{user_segment}")) end end end From 7cfa7b18f9b711afb8ade90ff01316f24e83fae2 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Tue, 20 Feb 2018 22:46:00 +0100 Subject: [PATCH 3/9] Validate Newsletter segment_recipient value Why: A Newsletter can only be sent if the are available user recipient emails and that means the `segment_recipient` value actually corresponds to a function on the UserSegments class. We could rely on the UserSegments::SEGMENTS constant as the list of possible user segments functions that a Newsletter can use to gather emails, so any value not included in that hash would not be valid. But to be 100% sure the newsletter can get a recipients_list we should just check if the UserSegments class has a method with same name as the `segment_recipient` value. How: * Adding an validation method that checks if UserSegment has a method with same name as the `segment_recipient` value. * Adding an scenario to the Newsletter model spec to check this --- app/models/newsletter.rb | 13 ++++++++++++- config/locales/en/activerecord.yml | 4 ++++ config/locales/es/activerecord.yml | 4 ++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/app/models/newsletter.rb b/app/models/newsletter.rb index 98b53d295..4ac6d59af 100644 --- a/app/models/newsletter.rb +++ b/app/models/newsletter.rb @@ -4,14 +4,25 @@ class Newsletter < ActiveRecord::Base validates :segment_recipient, presence: true validates :from, presence: true validates :body, presence: true + validate :validate_segment_recipient validates_format_of :from, :with => /@/ def list_of_recipients - UserSegments.send(segment_recipient).newsletter + UserSegments.send(segment_recipient).newsletter if valid_segment_recipient? + end + + def valid_segment_recipient? + segment_recipient && UserSegments.respond_to?(segment_recipient) end def draft? sent_at.nil? end + + private + + def validate_segment_recipient + errors.add(:segment_recipient, :invalid) unless valid_segment_recipient? + end end diff --git a/config/locales/en/activerecord.yml b/config/locales/en/activerecord.yml index 94b8b3d73..765f92932 100644 --- a/config/locales/en/activerecord.yml +++ b/config/locales/en/activerecord.yml @@ -261,6 +261,10 @@ en: attachment: min_image_width: "Image Width must be at least %{required_min_width}px" min_image_height: "Image Height must be at least %{required_min_height}px" + newsletter: + attributes: + segment_recipient: + invalid: "The user recipients segment is invalid" map_location: attributes: map: diff --git a/config/locales/es/activerecord.yml b/config/locales/es/activerecord.yml index 564676f23..9e195bd25 100644 --- a/config/locales/es/activerecord.yml +++ b/config/locales/es/activerecord.yml @@ -257,6 +257,10 @@ es: attachment: min_image_width: "La imagen debe tener al menos %{required_min_width}px de largo" min_image_height: "La imagen debe tener al menos %{required_min_height}px de alto" + newsletter: + attributes: + segment_recipient: + invalid: "El segmento de usuarios es inválido" map_location: attributes: map: From 7dfa056fe0d412272ba9e70bc83dbf5368aef125 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Wed, 21 Feb 2018 00:58:35 +0100 Subject: [PATCH 4/9] Add invalid newsletter scenarios to feature spec Newsletters with an invalid user segment should display a warning on show and index. Also those newsletters should not be sent. --- .../features/admin/emails/newsletters_spec.rb | 86 +++++++++++++------ 1 file changed, 59 insertions(+), 27 deletions(-) diff --git a/spec/features/admin/emails/newsletters_spec.rb b/spec/features/admin/emails/newsletters_spec.rb index 94f93556e..b953c140a 100644 --- a/spec/features/admin/emails/newsletters_spec.rb +++ b/spec/features/admin/emails/newsletters_spec.rb @@ -8,33 +8,56 @@ feature "Admin newsletter emails" do create(:budget) end - scenario "Show" do - newsletter = create(:newsletter, subject: "This is a subject", - segment_recipient: 'all_users', - from: "no-reply@consul.dev", - body: "This is a body") + context "Show" do + scenario "Valid newsletter" do + newsletter = create(:newsletter, subject: "This is a subject", + segment_recipient: 'all_users', + from: "no-reply@consul.dev", + body: "This is a body") - visit admin_newsletter_path(newsletter) + visit admin_newsletter_path(newsletter) - 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 "no-reply@consul.dev" - expect(page).to have_content "This is a body" + 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 "no-reply@consul.dev" + expect(page).to have_content "This is a body" + end + + scenario "Invalid newsletter" do + invalid_newsletter = create(:newsletter) + invalid_newsletter.update_attribute(:segment_recipient, 'invalid_segment') + + visit admin_newsletter_path(invalid_newsletter) + + expect(page).to have_content("Recipients user segment is invalid") + end end - scenario "Index" do - 3.times { create(:newsletter) } + context "Index" do + scenario "Valid newsletters" do + 3.times { create(:newsletter) } - visit admin_newsletters_path + visit admin_newsletters_path - expect(page).to have_css(".newsletter", count: 3) + expect(page).to have_css(".newsletter", count: 3) - Newsletter.all.each do |newsletter| - within("#newsletter_#{newsletter.id}") do - expect(page).to have_content newsletter.subject - expect(page).to have_content I18n.t("admin.segment_recipient.#{newsletter.segment_recipient}") + Newsletter.all.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 + end end end + + scenario "Invalid newsletter" do + invalid_newsletter = create(:newsletter) + invalid_newsletter.update_attribute(:segment_recipient, 'invalid_segment') + + visit admin_newsletters_path + + expect(page).to have_content("Recipients user segment is invalid") + end end scenario "Create" do @@ -103,24 +126,33 @@ feature "Admin newsletter emails" do expect(page).to have_content error_message end - scenario "Send newsletter email", :js do - newsletter = create(:newsletter) - visit admin_newsletter_path(newsletter) + context "Send newsletter", :js do + scenario "Sends newsletter emails", :js do + newsletter = create(:newsletter) + visit admin_newsletter_path(newsletter) - click_link "Send" + click_link "Send" - total_users = newsletter.list_of_recipients.count - page.accept_confirm("Are you sure you want to send this newsletter to #{total_users} users?") + total_users = newsletter.list_of_recipients.count + page.accept_confirm("Are you sure you want to send this newsletter to #{total_users} users?") - expect(page).to have_content "Newsletter sent successfully" + expect(page).to have_content "Newsletter sent successfully" + end + + scenario "Invalid newsletter cannot be sent", :js do + invalid_newsletter = create(:newsletter) + invalid_newsletter.update_attribute(:segment_recipient, 'invalid_segment') + visit admin_newsletter_path(invalid_newsletter) + + expect(page).not_to have_link("Send") + end end scenario "Select list of users to send newsletter" do UserSegments::SEGMENTS.each do |user_segment| visit new_admin_newsletter_path - fill_in_newsletter_form - select I18n.t("admin.segment_recipient.#{user_segment}"), from: 'newsletter_segment_recipient' + fill_in_newsletter_form(segment_recipient: I18n.t("admin.segment_recipient.#{user_segment}")) click_button "Create Newsletter" expect(page).to have_content(I18n.t("admin.segment_recipient.#{user_segment}")) From 38f7a222f5a9db13bcd8bc78bc8df61fe4063d1d Mon Sep 17 00:00:00 2001 From: Bertocq Date: Wed, 21 Feb 2018 01:02:51 +0100 Subject: [PATCH 5/9] Add error message for invalid Newsletter user segment Instead of the name of the User Segment we'll display an error message --- app/helpers/user_segments_helper.rb | 8 ++++++++ app/views/admin/newsletters/index.html.erb | 2 +- app/views/admin/newsletters/show.html.erb | 2 +- config/locales/en/admin.yml | 1 + config/locales/es/admin.yml | 1 + 5 files changed, 12 insertions(+), 2 deletions(-) diff --git a/app/helpers/user_segments_helper.rb b/app/helpers/user_segments_helper.rb index 6431216ef..796ff03da 100644 --- a/app/helpers/user_segments_helper.rb +++ b/app/helpers/user_segments_helper.rb @@ -4,4 +4,12 @@ module UserSegmentsHelper [t("admin.segment_recipient.#{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 + end end diff --git a/app/views/admin/newsletters/index.html.erb b/app/views/admin/newsletters/index.html.erb index c94a2c046..00a94e336 100644 --- a/app/views/admin/newsletters/index.html.erb +++ b/app/views/admin/newsletters/index.html.erb @@ -19,7 +19,7 @@ <%= newsletter.subject %> - <%= t("admin.segment_recipient.#{newsletter.segment_recipient}") %> + <%= segment_name(newsletter.segment_recipient) %> <% if newsletter.draft? %> diff --git a/app/views/admin/newsletters/show.html.erb b/app/views/admin/newsletters/show.html.erb index a4da2a746..7f95bacde 100644 --- a/app/views/admin/newsletters/show.html.erb +++ b/app/views/admin/newsletters/show.html.erb @@ -27,7 +27,7 @@
<%= t("admin.newsletters.show.segment_recipient") %>
- <%= t("admin.segment_recipient.#{@newsletter.segment_recipient}") %> + <%= segment_name(@newsletter.segment_recipient) %> <%= t("admin.newsletters.show.affected_users", n: recipients_count) %>
diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index 2a0e0b502..4b135a1b4 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -523,6 +523,7 @@ en: feasible_and_undecided_investment_authors: Authors of feasible or undecided investments in the current budget selected_investment_authors: Authors of selected investments in the current budget winner_investment_authors: Authors of winner investments in the current budget + invalid_recipients_segment: "Recipients user segment is invalid" newsletters: create_success: Newsletter created successfully update_success: Newsletter updated successfully diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index df4588bc8..0059768ae 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -522,6 +522,7 @@ es: feasible_and_undecided_investment_authors: Usuarios autores de proyectos de gasto viables o sin decidir en los actuales presupuestos selected_investment_authors: Usuarios autores de proyectos de gasto seleccionadas en los actuales presupuestos winner_investment_authors: Usuarios autores de proyectos de gasto ganadoras en los actuales presupuestos + invalid_recipients_segment: "El segmento de destinatarios es inválido" newsletters: create_success: Newsletter creada correctamente update_success: Newsletter actualizada correctamente From f34d6c1c1d1bbd00138dbeea2780f124a7267f63 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Wed, 21 Feb 2018 01:06:09 +0100 Subject: [PATCH 6/9] Prevent invalid newsletters from being sent Why: Newsletters without a valid user segment can't be sent since there is no recipient user email list. How: * Hiding the send button at the form * Preventing an invalid newsletter from being delivered at the controller --- app/controllers/admin/newsletters_controller.rb | 11 ++++++++--- app/views/admin/newsletters/show.html.erb | 5 +++-- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/app/controllers/admin/newsletters_controller.rb b/app/controllers/admin/newsletters_controller.rb index 980125ee7..3cc2853c1 100644 --- a/app/controllers/admin/newsletters_controller.rb +++ b/app/controllers/admin/newsletters_controller.rb @@ -46,11 +46,16 @@ class Admin::NewslettersController < Admin::BaseController def deliver @newsletter = Newsletter.find(params[:id]) - Mailer.newsletter(@newsletter).deliver_later - @newsletter.update(sent_at: Time.current) + if @newsletter.valid? + Mailer.newsletter(@newsletter).deliver_later + @newsletter.update(sent_at: Time.current) + flash[:notice] = t("admin.newsletters.send_success") + else + flash[:error] = t("admin.segment_recipient.invalid_recipients_segment") + end - redirect_to [:admin, @newsletter], notice: t("admin.newsletters.send_success") + redirect_to [:admin, @newsletter] end private diff --git a/app/views/admin/newsletters/show.html.erb b/app/views/admin/newsletters/show.html.erb index 7f95bacde..1c78d7247 100644 --- a/app/views/admin/newsletters/show.html.erb +++ b/app/views/admin/newsletters/show.html.erb @@ -42,8 +42,9 @@
-<% if @newsletter.draft? %> - <%= link_to t("admin.newsletters.show.send"), deliver_admin_newsletter_path(@newsletter), +<% if @newsletter.draft? && @newsletter.valid_segment_recipient? %> + <%= link_to t("admin.newsletters.show.send"), + deliver_admin_newsletter_path(@newsletter), "data-alert": t("admin.newsletters.show.send_alert", n: recipients_count), method: :post, id: "js-send-newsletter-alert", From 24aa157998d4bdf20dbd9f62f181f55e7fcb26f6 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Wed, 21 Feb 2018 01:06:28 +0100 Subject: [PATCH 7/9] Fix invalid Newsletter recipients count to 0 When a newsletter doesn't have a valid user segment for the recipients list, the number of users should be 0. --- app/views/admin/newsletters/show.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/admin/newsletters/show.html.erb b/app/views/admin/newsletters/show.html.erb index 1c78d7247..35f6d86b8 100644 --- a/app/views/admin/newsletters/show.html.erb +++ b/app/views/admin/newsletters/show.html.erb @@ -2,7 +2,7 @@

<%= t("admin.newsletters.show.title") %>

-<%- recipients_count = @newsletter.list_of_recipients.count %> +<% recipients_count = @newsletter.valid_segment_recipient? ? @newsletter.list_of_recipients.count : 0 %>
From fb2c92228ab5bc2309dfb462bd6f2ed33e57c31d Mon Sep 17 00:00:00 2001 From: Bertocq Date: Wed, 21 Feb 2018 01:07:25 +0100 Subject: [PATCH 8/9] Refactor UserSegment class Simple refactor to avoid creating unnecessary variables and make it easier to read. --- lib/user_segments.rb | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/lib/user_segments.rb b/lib/user_segments.rb index 8a50f6d31..dd26132d0 100644 --- a/lib/user_segments.rb +++ b/lib/user_segments.rb @@ -11,30 +11,24 @@ class UserSegments end def self.proposal_authors - author_ids = Proposal.not_archived.not_retired.pluck(:author_id).uniq - author_ids(author_ids) + author_ids(Proposal.not_archived.not_retired.pluck(:author_id).uniq) end def self.investment_authors - author_ids = current_budget_investments.pluck(:author_id).uniq - author_ids(author_ids) + author_ids(current_budget_investments.pluck(:author_id).uniq) end def self.feasible_and_undecided_investment_authors - author_ids = current_budget_investments.where(feasibility: %w(feasible undecided)) - .pluck(:author_id).uniq - - author_ids(author_ids) + feasibility = %w(feasible undecided) + author_ids(current_budget_investments.where(feasibility: feasibility).pluck(:author_id).uniq) end def self.selected_investment_authors - author_ids = current_budget_investments.selected.pluck(:author_id).uniq - author_ids(author_ids) + author_ids(current_budget_investments.selected.pluck(:author_id).uniq) end def self.winner_investment_authors - author_ids = current_budget_investments.winners.pluck(:author_id).uniq - author_ids(author_ids) + author_ids(current_budget_investments.winners.pluck(:author_id).uniq) end private From 12fd0a7cd357fa1c1990b9f8c8f9a88f52674018 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Wed, 21 Feb 2018 01:33:17 +0100 Subject: [PATCH 9/9] Make newsletter seed file line lenght compliant --- db/dev_seeds/newsletters.rb | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/db/dev_seeds/newsletters.rb b/db/dev_seeds/newsletters.rb index 2f70b52b6..08eda5301 100644 --- a/db/dev_seeds/newsletters.rb +++ b/db/dev_seeds/newsletters.rb @@ -1,8 +1,14 @@ section "Creating Newsletters" do newsletter_body = [ - "We choose to go to the moon in this decade and do the other things, not because they are easy, but because they are hard, because that goal will serve to organize and measure the best of our energies and skills, because that challenge is one that we are willing to accept, one we are unwilling to postpone, and one which we intend to win.", - "Spaceflights cannot be stopped. This is not the work of any one man or even a group of men. It is a historical process which mankind is carrying out in accordance with the natural laws of human development.", - "Many say exploration is part of our destiny, but it’s actually our duty to future generations and their quest to ensure the survival of the human species." + "We choose to go to the moon in this decade and do the other things, not because they are easy"\ + ", but because they are hard, because that goal will serve to organize and measure the best of"\ + " our energies and skills, because that challenge is one that we are willing to accept, one we"\ + " are unwilling to postpone, and one which we intend to win.", + "Spaceflights cannot be stopped. This is not the work of any one man or even a group of men."\ + " It is a historical process which mankind is carrying out in accordance with the natural laws"\ + " of human development.", + "Many say exploration is part of our destiny, but it’s actually our duty to future generations"\ + " and their quest to ensure the survival of the human species." ] 5.times do |n|