modified the plan.readable_by? method and added wider test coverage (#2416)

Author: xsrust

app/models/plan.rb

@@ -344,19 +344,17 @@ def editable_by?(user_id)
   #
   # Returns Boolean
   def readable_by?(user_id)
+    return true if commentable_by?(user_id)
     current_user = User.find(user_id)
-    if current_user.present?
-      # If the user is a super admin and the config allows for supers to view plans
-      if current_user.can_super_admin? &&
-          Branding.fetch(:service_configuration, :plans, :super_admins_read_all)
-        true
-      # If the user is an org admin and the config allows for org admins to view plans
-      elsif current_user.can_org_admin? &&
-          Branding.fetch(:service_configuration, :plans, :org_admins_read_all)
-        owner_and_coowners.map(&:org_id).include?(current_user.org_id)
-      else
-        commentable_by?(user_id)
-      end
+    return false unless current_user.present?
+    # If the user is a super admin and the config allows for supers to view plans
+    if current_user.can_super_admin? &&
+        Branding.fetch(:service_configuration, :plans, :super_admins_read_all)
+      true
+    # If the user is an org admin and the config allows for org admins to view plans
+    elsif current_user.can_org_admin? &&
+        Branding.fetch(:service_configuration, :plans, :org_admins_read_all)
+      owner_and_coowners.map(&:org_id).include?(current_user.org_id)
     else
       false
     end
@@ -388,6 +386,7 @@ def administerable_by?(user_id)
   def reviewable_by?(user_id)
     reviewer = User.find(user_id)
     feedback_requested? &&
+    reviewer.present? &&
     reviewer.org_id == owner.org_id &&
     reviewer.can_review_plans?
   end

spec/models/plan_spec.rb

@@ -856,6 +856,36 @@
       end
     end
 
+    context "explicit sharing does not conflict with admin-viewing" do
+
+      it "super admins" do
+        Branding.expects(:fetch)
+                .with(:service_configuration, :plans, :super_admins_read_all)
+                .at_most_once
+                .returns(false)
+
+        user.perms << create(:perm, name: "add_organisations")
+        role = subject.roles.commenter.first
+        role.user_id = user.id
+        role.save!
+
+        expect(subject.readable_by?(user.id)).to eql(true)
+      end
+
+      it "org admins" do
+        Branding.expects(:fetch)
+                .with(:service_configuration, :plans, :org_admins_read_all)
+                .at_most_once
+                .returns(false)
+
+        user.perms << create(:perm, name: "modify_guidance")
+        role = subject.roles.commenter.first
+        role.user_id = user.id
+        role.save!
+
+        expect(subject.readable_by?(user.id)).to eql(true)
+      end
+    end
   end
 
   describe "#commentable_by?" do