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
This commit is contained in:
@@ -56,8 +56,6 @@ class Admin::NewslettersController < Admin::BaseController
|
|||||||
private
|
private
|
||||||
|
|
||||||
def newsletter_params
|
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
|
||||||
end
|
end
|
||||||
|
|||||||
7
app/helpers/user_segments_helper.rb
Normal file
7
app/helpers/user_segments_helper.rb
Normal file
@@ -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
|
||||||
@@ -1,10 +1,4 @@
|
|||||||
class Newsletter < ActiveRecord::Base
|
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 :subject, presence: true
|
||||||
validates :segment_recipient, presence: true
|
validates :segment_recipient, presence: true
|
||||||
|
|||||||
@@ -10,8 +10,7 @@
|
|||||||
<%= t('admin.emails_download.index.download_segment_help_text') %>
|
<%= t('admin.emails_download.index.download_segment_help_text') %>
|
||||||
</p>
|
</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">
|
<div class="margin-top">
|
||||||
<%= submit_tag t('admin.emails_download.index.download_emails_button'), class: "button" %>
|
<%= submit_tag t('admin.emails_download.index.download_emails_button'), class: "button" %>
|
||||||
|
|||||||
@@ -1,8 +1,7 @@
|
|||||||
<%= form_for [:admin, @newsletter] do |f| %>
|
<%= form_for [:admin, @newsletter] do |f| %>
|
||||||
<%= render 'shared/errors', resource: @newsletter %>
|
<%= 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]) %>
|
@newsletter[:segment_recipient]) %>
|
||||||
<%= f.text_field :subject %>
|
<%= f.text_field :subject %>
|
||||||
<%= f.text_field :from %>
|
<%= f.text_field :from %>
|
||||||
|
|||||||
@@ -8,7 +8,7 @@ section "Creating Newsletters" do
|
|||||||
5.times do |n|
|
5.times do |n|
|
||||||
Newsletter.create!(
|
Newsletter.create!(
|
||||||
subject: "Newsletter subject #{n}",
|
subject: "Newsletter subject #{n}",
|
||||||
segment_recipient: Newsletter.segment_recipients.values.sample,
|
segment_recipient: UserSegments::SEGMENTS.sample,
|
||||||
from: 'no-reply@consul.dev',
|
from: 'no-reply@consul.dev',
|
||||||
body: newsletter_body.sample,
|
body: newsletter_body.sample,
|
||||||
sent_at: [Time.now, nil].sample
|
sent_at: [Time.now, nil].sample
|
||||||
|
|||||||
@@ -0,0 +1,5 @@
|
|||||||
|
class ChangeNewsletterSegmentRecipientToString < ActiveRecord::Migration
|
||||||
|
def change
|
||||||
|
change_column :newsletters, :segment_recipient, :string, null: false
|
||||||
|
end
|
||||||
|
end
|
||||||
@@ -11,7 +11,7 @@
|
|||||||
#
|
#
|
||||||
# It's strongly recommended that you check this file into your version control system.
|
# 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
|
# These are extensions that must be enabled in order to support this database
|
||||||
enable_extension "plpgsql"
|
enable_extension "plpgsql"
|
||||||
@@ -626,7 +626,7 @@ ActiveRecord::Schema.define(version: 20180205170054) do
|
|||||||
|
|
||||||
create_table "newsletters", force: :cascade do |t|
|
create_table "newsletters", force: :cascade do |t|
|
||||||
t.string "subject"
|
t.string "subject"
|
||||||
t.integer "segment_recipient"
|
t.string "segment_recipient", null: false
|
||||||
t.string "from"
|
t.string "from"
|
||||||
t.text "body"
|
t.text "body"
|
||||||
t.date "sent_at"
|
t.date "sent_at"
|
||||||
|
|||||||
@@ -945,7 +945,7 @@ LOREM_IPSUM
|
|||||||
|
|
||||||
factory :newsletter do
|
factory :newsletter do
|
||||||
sequence(:subject) { |n| "Subject #{n}" }
|
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
|
||||||
|
|||||||
@@ -10,7 +10,7 @@ feature "Admin newsletter emails" do
|
|||||||
|
|
||||||
scenario "Show" do
|
scenario "Show" do
|
||||||
newsletter = create(:newsletter, subject: "This is a subject",
|
newsletter = create(:newsletter, subject: "This is a subject",
|
||||||
segment_recipient: 1,
|
segment_recipient: 'all_users',
|
||||||
from: "no-reply@consul.dev",
|
from: "no-reply@consul.dev",
|
||||||
body: "This is a body")
|
body: "This is a body")
|
||||||
|
|
||||||
@@ -116,14 +116,14 @@ feature "Admin newsletter emails" do
|
|||||||
end
|
end
|
||||||
|
|
||||||
scenario "Select list of users to send newsletter" do
|
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
|
visit new_admin_newsletter_path
|
||||||
|
|
||||||
fill_in_newsletter_form
|
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"
|
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
|
end
|
||||||
end
|
end
|
||||||
|
|||||||
Reference in New Issue
Block a user