REFACTOR: Move option to return emails into the serializer

approved

#1

REFACTOR: Move option to return emails into the serializer

This makes more sense than having the guardian take an accessor. The logic belongs in the Serializer, where the JSON is calculated.

Also removed some of the DRYness in the spec. It’s fewer lines and made it easier to test the option on the serializer.

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index 14627a1..20dafe4 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -32,12 +32,13 @@ class Admin::UsersController < Admin::AdminController
   def index
     users = ::AdminUserIndexQuery.new(params).find_users
 
+    opts = {}
     if params[:show_emails] == "true"
-      guardian.can_see_emails = true
       StaffActionLogger.new(current_user).log_show_emails(users)
+      opts[:emails_desired] = true
     end
 
-    render_serialized(users, AdminUserListSerializer)
+    render_serialized(users, AdminUserListSerializer, opts)
   end
 
   def show
diff --git a/app/serializers/admin_user_list_serializer.rb b/app/serializers/admin_user_list_serializer.rb
index f318cac..81d953a 100644
--- a/app/serializers/admin_user_list_serializer.rb
+++ b/app/serializers/admin_user_list_serializer.rb
@@ -39,7 +39,7 @@ class AdminUserListSerializer < BasicUserSerializer
   def include_email?
     # staff members can always see their email
     (scope.is_staff? && (object.id == scope.user.id || object.staged?)) ||
-    (scope.is_admin? && scope.can_see_emails?)
+      (scope.is_admin? && @options[:emails_desired])
   end
 
   alias_method :include_secondary_emails?, :include_email?
diff --git a/lib/guardian.rb b/lib/guardian.rb
index 65a1d3f..cb80bb7 100644
--- a/lib/guardian.rb
+++ b/lib/guardian.rb
@@ -54,7 +54,6 @@ class Guardian
     end
   end
 
-  attr_accessor :can_see_emails
   attr_reader :request
 
   def initialize(user = nil, request = nil)
@@ -381,10 +380,6 @@ class Guardian
     )
   end
 
-  def can_see_emails?
-    @can_see_emails
-  end
-
   def can_export_entity?(entity)
     return false unless @user
     return true if is_admin?
diff --git a/spec/serializers/admin_user_list_serializer_spec.rb b/spec/serializers/admin_user_list_serializer_spec.rb
index 63bc97c..60c7dac 100644
--- a/spec/serializers/admin_user_list_serializer_spec.rb
+++ b/spec/serializers/admin_user_list_serializer_spec.rb
@@ -8,92 +8,59 @@ describe AdminUserListSerializer do
     let(:moderator) { Fabricate(:user_single_email, moderator: true, email: "moderator@email.com") }
     let(:user) { Fabricate(:user_single_email, email: "user@email.com") }
     let(:guardian) { Guardian.new(admin) }
-    let(:mod_guardian) { Guardian.new(moderator) }
 
-    let(:json) do
-      AdminUserListSerializer.new(user,
-        scope: guardian,
-        root: false
-      ).as_json
+    let(:serializer) do
+      AdminUserListSerializer.new(user, scope: guardian, root: false)
     end
 
-    let(:mod_json) do
-      AdminUserListSerializer.new(user,
-        scope: mod_guardian,
-        root: false
+    def serialize(user, viewed_by, opts = nil)
+      AdminUserListSerializer.new(
+        user,
+        scope: Guardian.new(viewed_by),
+        root: false,
+        emails_desired: opts && opts[:emails_desired]
       ).as_json
     end
 
     def fabricate_secondary_emails_for(u)
-      ["first", "second"].each do |name|
-        Fabricate(:secondary_email, user: u, email: "#{name}@email.com")
-      end
-    end
-
-    shared_examples "shown" do |email|
-      it "contains emails" do
-        expect(json[:email]).to eq(email)
-
-        expect(json[:secondary_emails]).to contain_exactly(
-          "first@email.com",
-          "second@email.com"
-        )
-      end
-    end
-
-    shared_examples "not shown" do
-      it "doesn't contain emails" do
-        expect(json[:email]).to eq(nil)
-        expect(json[:secondary_emails]).to eq(nil)
-      end
+      Fabricate(:secondary_email, user: u, email: "first@email.com")
+      Fabricate(:secondary_email, user: u, email: "second@email.com")
     end
 
-    context "with myself" do
-      let(:user) { admin }
-
-      before do
-        fabricate_secondary_emails_for(admin)
-      end
-
-      include_examples "shown", "admin@email.com"
+    it "contains an admin's own emails" do
+      fabricate_secondary_emails_for(admin)
+      json = serialize(admin, admin)
+      expect(json[:email]).to eq("admin@email.com")
+      expect(json[:secondary_emails]).to contain_exactly("first@email.com", "second@email.com")
     end
 
-    context "with a normal user" do
-      before do
-        fabricate_secondary_emails_for(user)
-      end
-
-      include_examples "not shown"
+    it "doesn't include a regular user's emails" do
+      fabricate_secondary_emails_for(user)
+      json = serialize(user, user)
+      expect(json[:email]).to eq(nil)
+      expect(json[:secondary_emails]).to eq(nil)
     end
 
-    context "when moderator makes a request with show_emails param set to true" do
-      before do
-        mod_guardian.can_see_emails = true
-        fabricate_secondary_emails_for(user)
-      end
-
-      it "doesn't contain emails" do
-        expect(mod_json[:email]).to eq(nil)
-        expect(mod_json[:secondary_emails]).to eq(nil)
-      end
+    it "doesn't return emails for a moderator request" do
+      fabricate_secondary_emails_for(user)
+      json = serialize(user, moderator, emails_desired: true)
+      expect(json[:email]).to eq(nil)
+      expect(json[:secondary_emails]).to eq(nil)
     end
 
-    context "with a normal user after clicking 'show emails'" do
-      before do
-        guardian.can_see_emails = true
-        fabricate_secondary_emails_for(user)
-      end
-
-      include_examples "shown", "user@email.com"
+    it "returns emails for admins when emails_desired is true" do
+      fabricate_secondary_emails_for(user)
+      json = serialize(user, admin, emails_desired: true)
+      expect(json[:email]).to eq("user@email.com")
+      expect(json[:secondary_emails]).to contain_exactly("first@email.com", "second@email.com")
     end
 
-    context "with a staged user" do
-      before do
-        user.staged = true
-        fabricate_secondary_emails_for(user)
-      end
-
-      include_examples "shown", "user@email.com"
+    it "returns a staged user's emails" do
+      user.staged = true
+      fabricate_secondary_emails_for(user)
+      json = serialize(user, admin)
+      expect(json[:email]).to eq("user@email.com")
+      expect(json[:secondary_emails]).to contain_exactly("first@email.com", "second@email.com")
     end
   end
 end

GitHub sha: dbe42068


#2