Merge pull request #2474 from consul/admin_newsletter_email_refactor

Admin newsletter email refactor

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

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)
+      params.require(:newsletter).permit(:subject, :segment_recipient, :from, :body)
-                                .permit(:subject, :segment_recipient, :from, :body)
-      newsletter_params.merge(segment_recipient: newsletter_params[:segment_recipient].to_i)
     end
 end

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

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

app/views/admin/emails_download/index.html.erb

@@ -10,8 +10,7 @@
       <%= t('admin.emails_download.index.download_segment_help_text') %>
     </p>
 
-    <%= select_tag :users_segment, options_for_select(UserSegments::SEGMENTS
+    <%= select_tag :users_segment, options_for_select(user_segments_options) %>
-                                   .collect { |s| [t("admin.segment_recipient.#{s}"), s] }) %>
 
     <div class="margin-top">
       <%= submit_tag t('admin.emails_download.index.download_emails_button'), class: "button" %>

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
+  <%= f.select :segment_recipient, options_for_select(user_segments_options,
-                                   .collect { |k,v| [t("admin.segment_recipient.#{k}"), v] },
                                                       @newsletter[:segment_recipient]) %>
   <%= f.text_field :subject %>
   <%= f.text_field :from %>

app/views/admin/newsletters/index.html.erb

@@ -19,7 +19,7 @@
           <%= newsletter.subject %>
         </td>
         <td>
-          <%= t("admin.segment_recipient.#{newsletter.segment_recipient}") %>
+          <%= segment_name(newsletter.segment_recipient) %>
         </td>
         <td>
           <% if newsletter.draft? %>

app/views/admin/newsletters/show.html.erb

@@ -2,7 +2,7 @@
 
 <h2><%= t("admin.newsletters.show.title") %></h2>
 
-<%- recipients_count = @newsletter.list_of_recipients.count %>
+<% recipients_count = @newsletter.valid_segment_recipient? ? @newsletter.list_of_recipients.count : 0 %>
 
 <div class="small-12 column">
   <div class="callout highlight">
@@ -27,7 +27,7 @@
     <div class="row">
       <div class="small-12 column">
         <strong><%= t("admin.newsletters.show.segment_recipient") %></strong><br>
-        <%= t("admin.segment_recipient.#{@newsletter.segment_recipient}") %>
+        <%= segment_name(@newsletter.segment_recipient) %>
         <%= t("admin.newsletters.show.affected_users", n: recipients_count) %>
       </div>
     </div>
@@ -42,8 +42,9 @@
   </div>
 </div>
 
-<% if @newsletter.draft? %>
+<% if @newsletter.draft? && @newsletter.valid_segment_recipient? %>
-  <%= link_to t("admin.newsletters.show.send"), deliver_admin_newsletter_path(@newsletter),
+  <%= 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",

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:

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

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:

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

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.",
+    "We choose to go to the moon in this decade and do the other things, not because they are easy"\
-    "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.",
+    ", but because they are hard, because that goal will serve to organize and measure the best of"\
-    "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."
+    " 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

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

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"

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(Proposal.not_archived.not_retired.pluck(:author_id).uniq)
-    author_ids(author_ids)
   end
 
   def self.investment_authors
-    author_ids = current_budget_investments.pluck(:author_id).uniq
+    author_ids(current_budget_investments.pluck(:author_id).uniq)
-    author_ids(author_ids)
   end
 
   def self.feasible_and_undecided_investment_authors
-    author_ids = current_budget_investments.where(feasibility: %w(feasible undecided))
+    feasibility = %w(feasible undecided)
-                                           .pluck(:author_id).uniq
+    author_ids(current_budget_investments.where(feasibility: feasibility).pluck(:author_id).uniq)
-
-    author_ids(author_ids)
   end
 
   def self.selected_investment_authors
-    author_ids = current_budget_investments.selected.pluck(:author_id).uniq
+    author_ids(current_budget_investments.selected.pluck(:author_id).uniq)
-    author_ids(author_ids)
   end
 
   def self.winner_investment_authors
-    author_ids = current_budget_investments.winners.pluck(:author_id).uniq
+    author_ids(current_budget_investments.winners.pluck(:author_id).uniq)
-    author_ids(author_ids)
   end
 
   private

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
+    segment_recipient  UserSegments::SEGMENTS.sample
-    sequence(:from) { |n| "noreply#{n}@consul.dev" }
+    sequence(:from)    { |n| "noreply#{n}@consul.dev" }
-    sequence(:body) { |n| "Body #{n}" }
+    sequence(:body)    { |n| "Body #{n}" }
   end
 
 end

spec/features/admin/emails/newsletters_spec.rb

@@ -8,33 +8,56 @@ feature "Admin newsletter emails" do
     create(:budget)
   end
 
-  scenario "Show" do
+  context "Show" do
-    newsletter = create(:newsletter, subject: "This is a subject",
+    scenario "Valid newsletter" do
-                                     segment_recipient: 1,
+      newsletter = create(:newsletter, subject: "This is a subject",
-                                     from: "no-reply@consul.dev",
+                                       segment_recipient: 'all_users',
-                                     body: "This is a body")
+                                       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 "This is a subject"
-    expect(page).to have_content I18n.t("admin.segment_recipient.#{newsletter.segment_recipient}")
+      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 "no-reply@consul.dev"
-    expect(page).to have_content "This is a body"
+      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
+  context "Index" do
-    3.times { create(:newsletter) }
+    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|
+      Newsletter.all.each do |newsletter|
-      within("#newsletter_#{newsletter.id}") do
+        segment_recipient = I18n.t("admin.segment_recipient.#{newsletter.segment_recipient}")
-        expect(page).to have_content newsletter.subject
+        within("#newsletter_#{newsletter.id}") do
-        expect(page).to have_content I18n.t("admin.segment_recipient.#{newsletter.segment_recipient}")
+          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
+  context "Send newsletter", :js do
-    newsletter = create(:newsletter)
+    scenario "Sends newsletter emails", :js do
-    visit admin_newsletter_path(newsletter)
+      newsletter = create(:newsletter)
+      visit admin_newsletter_path(newsletter)
 
-    click_link "Send"
+      click_link "Send"
 
-    total_users = newsletter.list_of_recipients.count
+      total_users = newsletter.list_of_recipients.count
-    page.accept_confirm("Are you sure you want to send this newsletter to #{total_users} users?")
+      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
+      fill_in_newsletter_form(segment_recipient: I18n.t("admin.segment_recipient.#{user_segment}"))
-      select I18n.t("admin.segment_recipient.#{user_group}"), 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

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
+    it "returns all active users enabled" do
-      active_user1 = create(:user, newsletter: true)
+      active_user = create(:user)
-      active_user2 = create(:user, newsletter: true)
-      active_user3 = create(:user, newsletter: false)
       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_user
-      expect(described_class.all_users).to include active_user2
-      expect(described_class.all_users).not_to include active_user3
       expect(described_class.all_users).not_to include erased_user
     end
   end

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