Merge pull request consuldemocracy#2474 from consul/admin_newsletter_…

…email_refactor

Admin newsletter email refactor

app/controllers/admin/emails_download_controller.rb

@@ -13,6 +13,6 @@ def generate_csv
  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 @@ def destroy
 
  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

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
- .collect { |s| [t("admin.segment_recipient.#{s}"), s] }) %>
+ <%= select_tag :users_segment, options_for_select(user_segments_options) %>
 
  <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
- .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 %>

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? %>
- <%= 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",

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.",
- "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: '[email protected]',
  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 @@
 
  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(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

spec/factories.rb

@@ -945,9 +945,9 @@
 
  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