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/controllers/admin/newsletters_controller.rb b/app/controllers/admin/newsletters_controller.rb index 4dbf963f7..3cc2853c1 100644 --- a/app/controllers/admin/newsletters_controller.rb +++ b/app/controllers/admin/newsletters_controller.rb @@ -46,18 +46,21 @@ 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 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..796ff03da --- /dev/null +++ b/app/helpers/user_segments_helper.rb @@ -0,0 +1,15 @@ +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 + + 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/models/newsletter.rb b/app/models/newsletter.rb index 0b7bc4481..4ac6d59af 100644 --- a/app/models/newsletter.rb +++ b/app/models/newsletter.rb @@ -1,23 +1,28 @@ 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 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) + 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/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/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..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 %>
@@ -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) %>
@@ -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", 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/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/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: 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 diff --git a/db/dev_seeds/newsletters.rb b/db/dev_seeds/newsletters.rb index be9074f10..08eda5301 100644 --- a/db/dev_seeds/newsletters.rb +++ b/db/dev_seeds/newsletters.rb @@ -1,14 +1,20 @@ 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| 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/lib/user_segments.rb b/lib/user_segments.rb index 02fbe9b5f..dd26132d0 100644 --- a/lib/user_segments.rb +++ b/lib/user_segments.rb @@ -7,34 +7,28 @@ class UserSegments winner_investment_authors) def self.all_users - User.newsletter.active + User.active 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 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..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: 1, - 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,27 +126,36 @@ 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 - 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' + 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_group}")) + expect(page).to have_content(I18n.t("admin.segment_recipient.#{user_segment}")) end end end 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