From dbe42068a234a46cb398b47e4d984a448c283513 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Fri, 11 Jan 2019 11:10:02 -0500 Subject: [PATCH] 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. --- app/controllers/admin/users_controller.rb | 5 +- app/serializers/admin_user_list_serializer.rb | 2 +- lib/guardian.rb | 5 - .../admin_user_list_serializer_spec.rb | 105 ++++++------------ 4 files changed, 40 insertions(+), 77 deletions(-) diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 14627a10873..20dafe43e07 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 f318cac53a2..81d953a0e1d 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 65a1d3f103b..cb80bb781fe 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 63bc97cfd5c..60c7dac3c9a 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 + Fabricate(:secondary_email, user: u, email: "first@email.com") + Fabricate(:secondary_email, user: u, email: "second@email.com") 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 + 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 - 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 + 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 "with myself" do - let(:user) { admin } - - before do - fabricate_secondary_emails_for(admin) - end - - include_examples "shown", "admin@email.com" + 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" do - before do - fabricate_secondary_emails_for(user) - end - - include_examples "not shown" + 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 "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 - 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" - 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