Remove empty emails from users segments recipients

Why:

User with an empty email value (nil) should not appear in the recipient
list for a given UserSegment at Newsletters or Email Downloads.

How:

Using Enumerable#compact and Enumerable#select to filter out empty emails

Increasing Email Download feature spec and Newsletter model spec to cover
all possible scenarios including the nil email one.

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

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?

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

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