From bdbb32e82463fa4f9cf07621db6044a00fad7b92 Mon Sep 17 00:00:00 2001
From: Bertocq <bertocq@gmail.com>
Date: Tue, 20 Feb 2018 18:57:20 +0100
Subject: [PATCH] Move `newsletter` User scope outside UserSegments

Why:

UserSegments are not only used for Newsletters or Email downloads, but
also for internal Global Notifications. We don't want to have that scope
hardcoded inside UserSegments as users that have opted-out from the
newsletter should still be recipients of global notifications.

How:

Removing the scope from the UserSegments `all_users` method that acts as
base for all the other segments. Including that `newsletter` scope only
on the places that is relevant:
* When listing recipients for a newsletter
* When downloading a listing emails that can be newsletter recipients

Also updated relevant tests
---
 .../admin/emails_download_controller.rb        |  2 +-
 app/models/newsletter.rb                       |  2 +-
 lib/user_segments.rb                           |  2 +-
 spec/lib/user_segments_spec.rb                 | 10 +++-------
 spec/models/newsletter_spec.rb                 | 18 ++++++++++++++++++
 5 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/app/controllers/admin/emails_download_controller.rb b/app/controllers/admin/emails_download_controller.rb
index 326c32ff1..179240358 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).pluck(:email).to_csv
+    UserSegments.send(users_segment).newsletter.pluck(:email).to_csv
   end
 end
diff --git a/app/models/newsletter.rb b/app/models/newsletter.rb
index 0b7bc4481..beaa59495 100644
--- a/app/models/newsletter.rb
+++ b/app/models/newsletter.rb
@@ -14,7 +14,7 @@ class Newsletter < ActiveRecord::Base
   validates_format_of :from, :with => /@/
 
   def list_of_recipients
-    UserSegments.send(segment_recipient)
+    UserSegments.send(segment_recipient).newsletter
   end
 
   def draft?
diff --git a/lib/user_segments.rb b/lib/user_segments.rb
index 02fbe9b5f..8a50f6d31 100644
--- a/lib/user_segments.rb
+++ b/lib/user_segments.rb
@@ -7,7 +7,7 @@ class UserSegments
                 winner_investment_authors)
 
   def self.all_users
-    User.newsletter.active
+    User.active
   end
 
   def self.proposal_authors
diff --git a/spec/lib/user_segments_spec.rb b/spec/lib/user_segments_spec.rb
index 77dff6a27..3914c11bf 100644
--- a/spec/lib/user_segments_spec.rb
+++ b/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
-      active_user1 = create(:user, newsletter: true)
-      active_user2 = create(:user, newsletter: true)
-      active_user3 = create(:user, newsletter: false)
+    it "returns all active users enabled" do
+      active_user = create(:user)
       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_user2
-      expect(described_class.all_users).not_to include active_user3
+      expect(described_class.all_users).to include active_user
       expect(described_class.all_users).not_to include erased_user
     end
   end
diff --git a/spec/models/newsletter_spec.rb b/spec/models/newsletter_spec.rb
index 09d66055d..6ab83ffcd 100644
--- a/spec/models/newsletter_spec.rb
+++ b/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