diff --git a/app/controllers/admin/emails_download_controller.rb b/app/controllers/admin/emails_download_controller.rb index 179240358..a1725677d 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).newsletter.pluck(:email).to_csv + UserSegments.send(users_segment).newsletter.pluck(:email).compact.join(',') end end diff --git a/app/models/newsletter.rb b/app/models/newsletter.rb index 4ac6d59af..75c4c0af9 100644 --- a/app/models/newsletter.rb +++ b/app/models/newsletter.rb @@ -9,7 +9,7 @@ class Newsletter < ActiveRecord::Base validates_format_of :from, :with => /@/ def list_of_recipients - UserSegments.send(segment_recipient).newsletter if valid_segment_recipient? + UserSegments.send(segment_recipient).newsletter.select {|user| !user.email.nil? } if valid_segment_recipient? end def valid_segment_recipient? diff --git a/spec/features/admin/emails/emails_download_spec.rb b/spec/features/admin/emails/emails_download_spec.rb index ec3908307..8a92337b0 100644 --- a/spec/features/admin/emails/emails_download_spec.rb +++ b/spec/features/admin/emails/emails_download_spec.rb @@ -2,29 +2,48 @@ require 'rails_helper' feature "Admin download user emails" do + let(:admin_user) { create(:user, newsletter: false, email: 'admin@consul.dev') } + background do - admin = create(:administrator) - login_as(admin.user) + create(:administrator, user: admin_user) + login_as(admin_user) end - context "Index" do - scenario "returns the selected users segment csv file" do - user1 = create(:user) - user2 = create(:user) + context "Download only emails from segment users with newsletter flag & present email " do + before do + create(:user, email: 'user@consul.dev') + + create(:administrator, user: create(:user, newsletter: true, email: 'admin_news1@consul.dev')) + create(:administrator, user: create(:user, newsletter: true, email: 'admin_news2@consul.dev')) + + create(:administrator, user: create(:user, newsletter: false, email: 'no_news@consul.dev')) + + admin_without_email = create(:user, newsletter: true, email: 'no_email@consul.dev') + create(:administrator, user: admin_without_email) + admin_without_email.update_attribute(:email, nil) + end + + scenario "returns the selected users segment csv file" do visit admin_emails_download_index_path within('#admin_download_emails') do - select 'All users', from: 'users_segment' + select 'Administrators', from: 'users_segment' click_button 'Download emails list' end header = page.response_headers['Content-Disposition'] expect(header).to match /^attachment/ - expect(header).to match /filename="All users.csv"$/ + expect(header).to match /filename="Administrators.csv"$/ - expect(page).to have_content(user1.email) - expect(page).to have_content(user2.email) + file_contents = page.body.split(',') + expect(file_contents.count).to eq(2) + expect(file_contents).to include('admin_news1@consul.dev') + expect(file_contents).to include('admin_news2@consul.dev') + expect(file_contents).not_to include('admin@consul.dev') + expect(file_contents).not_to include('user@consul.dev') + expect(file_contents).not_to include('no_news@consul.dev') + expect(file_contents).not_to include('no_email@consul.dev') end end end diff --git a/spec/models/newsletter_spec.rb b/spec/models/newsletter_spec.rb index 74e0d855c..e7e56ee06 100644 --- a/spec/models/newsletter_spec.rb +++ b/spec/models/newsletter_spec.rb @@ -48,19 +48,20 @@ describe Newsletter do end describe '#list_of_recipients' do - let(:erased_user) { create(:user, username: 'erased_user') } before do - create(:user, newsletter: true, username: 'newsletter_user') - create(:user, newsletter: false) + create(:user, newsletter: true, email: 'newsletter_user@consul.dev') + no_news_user = create(:user, newsletter: false, email: 'no_news_user@consul.dev') + erased_user = create(:user, email: 'erased_user@consul.dev') erased_user.erase 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') - expect(newsletter.list_of_recipients).not_to include(erased_user) + expect(newsletter.list_of_recipients.map(&:email)).to include('newsletter_user@consul.dev') + expect(newsletter.list_of_recipients.map(&:email)).not_to include('no_news_user@consul.dev') + expect(newsletter.list_of_recipients.map(&:email)).not_to include('erased_user@consul.dev') end end end