Merge pull request #2129 from DigitalCurationCentre/feature/reviewer_…

…role

Feature/reviewer role

app/controllers/org_admin/plans_controller.rb

@@ -10,8 +10,10 @@ def index
       raise Pundit::NotAuthorizedError
     end
 
-    feedback_ids = Role.where(user_id: current_user.id).reviewer.pluck(:plan_id).uniq
-    @feedback_plans = Plan.where(id: feedback_ids)
+    feedback_ids = Role.creator.joins(:user,:plan)
+      .where('users.org_id = ? AND plans.feedback_requested is TRUE',
+              current_user.org_id).pluck(:plan_id)
+    @feedback_plans = Plan.find(feedback_ids)
     @plans = current_user.org.plans.page(1)
   end
 

app/controllers/plans_controller.rb

@@ -258,8 +258,7 @@ def share
     @plan = Plan.find(params[:id])
     if @plan.present?
       authorize @plan
-      # Get the roles where the user is not a reviewer
-      @plan_roles = @plan.roles.select { |r| !r.reviewer? }
+      @plan_roles = @plan.roles
     else
       redirect_to(plans_path)
     end

app/helpers/mailer_helper.rb

@@ -15,13 +15,7 @@ def privileges_list(user)
 
   # Returns the messaging for the specified role
   def role_text(role)
-    if role.reviewer?
-      {
-        type: _('reviewer'),
-        placeholder1: _('read the plan and provide feedback.'),
-        placeholder2: nil
-      }
-    elsif role.administrator?
+    if role.administrator?
       {
         type: _('co-owner'),
         placeholder1: _('write and edit the plan in a collaborative manner.'),
@@ -41,4 +35,4 @@ def role_text(role)
       }
     end
   end
-end
+end

app/helpers/perms_helper.rb

@@ -2,14 +2,15 @@ module PermsHelper
   # Returns a hash whose keys are the names associated to Perms and values are the text to be displayed to the end user
   def name_and_text
     {
-      :add_organisations => _('Add organisations'),
-      :change_org_affiliation => _('Change affiliation'),
-      :grant_permissions => _('Manage user privileges'),
-      :modify_templates => _('Manage templates'),
-      :modify_guidance => _('Manage guidance'),
-      :use_api => _('API rights'),
-      :change_org_details => _('Manage organisation details'),
-      :grant_api_to_orgs => _('Grant API to organisations')
+      add_organisations:  _('Add organisations'),
+      change_org_affiliation: _('Change affiliation'),
+      grant_permissions: _('Manage user privileges'),
+      modify_templates: _('Manage templates'),
+      modify_guidance: _('Manage guidance'),
+      use_api: _('API rights'),
+      change_org_details: _('Manage organisation details'),
+      grant_api_to_orgs:  _('Grant API to organisations'),
+      review_org_plans: _('')
     }
   end
-end
+end

app/models/perm.rb

@@ -60,4 +60,8 @@ def self.change_org_details
   def self.grant_api
     Perm.find_by(name: 'grant_api_to_orgs')
   end
+
+  def self.review_plans
+    Perm.find_by(name: 'review_org_plans')
+  end
 end

app/models/plan.rb

@@ -141,8 +141,7 @@ class Plan < ActiveRecord::Base
   # Retrieves any plan in which the user has an active role and
   # is not a reviewer
   scope :active, lambda { |user|
-    plan_ids = Role.where(active: true, user_id: user.id)
-                   .not_reviewer.pluck(:plan_id)
+    plan_ids = Role.where(active: true, user_id: user.id).pluck(:plan_id)
 
     includes(:template, :roles)
     .where(id: plan_ids)
@@ -278,13 +277,6 @@ def request_feedback(user)
     Plan.transaction do
       begin
         self.feedback_requested = true
-        # Share the plan with each org admin as the reviewer role
-        admins = user.org.org_admins
-        admins.each do |admin|
-          unless admin == user
-            add_user!(admin.id, :reviewer)
-          end
-        end
         if save!
           # Send an email to the org-admin contact
           if user.org.contact_email.present?
@@ -311,10 +303,6 @@ def complete_feedback(org_admin)
     Plan.transaction do
       begin
         self.feedback_requested = false
-
-        # Remove the org admins reviewer role from the plan
-        roles.delete(Role.where(plan: self).reviewer)
-
         if save!
           # Send an email confirmation to the owners and co-owners
           deliver_if(recipients: owner_and_coowners,
@@ -376,7 +364,7 @@ def readable_by?(user_id)
   #
   # Returns Boolean
   def commentable_by?(user_id)
-    Role.commenter.where(plan_id: id, user_id: user_id, active: true).any?
+    Role.commenter.where(plan_id: id, user_id: user_id, active: true).any? || reviewable_by?(user_id)
   end
 
   # determines if the plan is administerable by the specified user
@@ -394,7 +382,10 @@ def administerable_by?(user_id)
   #
   # Returns Boolean
   def reviewable_by?(user_id)
-    Role.reviewer.where(plan_id: id, user_id: user_id, active: true).any?
+    reviewer = User.find(user_id)
+    feedback_requested? &&
+    reviewer.org_id == owner.org_id &&
+    reviewer.can_review_plans?
   end
 
   # the datetime for the latest update of this plan
@@ -440,8 +431,6 @@ def add_user!(user_id, access_type = :commenter)
         role.editor = true
       when :editor
         role.editor = true
-      when :reviewer
-        role.reviewer = true
       end
       role.commenter = true
       role.save

app/models/user.rb

@@ -295,6 +295,15 @@ def can_grant_api_to_orgs?
     perms.include? Perm.grant_api
   end
 
+
+  ##
+  # Can the user review their organisation's plans?
+  #
+  # Returns Boolean
+  def can_review_plans?
+    perms.include? Perm.review_plans
+  end
+
   # Removes the api_token from the user
   #
   # Returns nil

app/policies/note_policy.rb

@@ -13,7 +13,7 @@ def create?
   end
 
   def update?
-    Plan.find(@note.answer.plan_id).commentable_by?(@user.id)
+    Plan.find(@note.answer.plan_id).commentable_by?(@user.id) && @note.user_id = @user.id
   end
 
   def archive?

app/views/paginable/users/_index.html.erb

@@ -41,7 +41,7 @@
                   </td>
                   <td class="text-center">
                       <% unless user.roles.nil? %>
-                      <%= user.roles.where(Role.not_reviewer_condition).length %>
+                      <%= user.roles.length %>
                       <% end %>
                   </td>
                   <%# The content of this column get updated through AJAX whenever the permission for an user are updated %>

app/views/plans/_share_form.html.erb

@@ -125,22 +125,21 @@
   <% end %>
 
 
-   <div class="col-xs-12">
-     <% if plan.owner_and_coowners.include?(current_user) && current_user.org.present? && current_user.org.feedback_enabled? %>
-       <h2><%= _('Request expert feedback') %></h2>
-       <p><%= _('Click below to give data management staff at your organisation access to read and comment on your plan.') %></p>
-       <div class="well well-sm">
-         <%= sanitize current_user.org.feedback_email_msg.to_s % { user_name: current_user.name(false), plan_name: plan.title } %>
-       </div>
-       <p><%= _('You can continue to edit and download the plan in the interim.') %></p>
-
-       <div class="form-group col-xs-8">
-         <%= link_to _('Request feedback'),
+  <div class="col-xs-12">
+    <% if plan.owner_and_coowners.include?(current_user) && plan.owner.org.feedback_enabled? %>
+      <h2><%= _('Request expert feedback') %></h2>
+      <p><%= _("Click below to give data management staff at #{plan.owner.org.name}, the Plan Owner's org, access to read and comment on your plan.") %></p>
+      <div class="well well-sm">
+        <%= sanitize plan.owner.org.feedback_email_msg.to_s % { user_name: current_user.name(false), plan_name: plan.title } %>
+      </div>
+      <p><%= _('You can continue to edit and download the plan in the interim.') %></p>
+      <div class="form-group col-xs-8">
+        <%= link_to _('Request feedback'),
                      feedback_requests_path(plan_id: @plan.id),
                      data: { method: 'post' },
                      class: "btn btn-default#{' disabled' if @plan.feedback_requested?}" %>
-         <span><%= _("Feedback has been requested.") if @plan.feedback_requested? %></span>
-       </div>
+        <span><%= _("Feedback has been requested.") if @plan.feedback_requested? %></span>
+      </div>
     <% end %>
   </div>
 <% end %>

app/views/users/_admin_grant_permissions.html.erb

@@ -2,19 +2,19 @@
   <div class="modal-content">
 
     <div class="modal-header">
-      <button type="button" class="close" 
-              data-dismiss="modal" 
+      <button type="button" class="close"
+              data-dismiss="modal"
               aria-label="Close">
               <span aria-hidden="true">&times;</span></button>
       <h2 class="modal-title"><%= _('Editing privileges for %{username}') % { username: user.name(false) } %></h2>
     </div>
-    
+
     <%= form_for user, url: admin_update_permissions_user_path(user), html: { method: :put, remote: true, class: 'admin_update_permissions' } do |f| %>
       <div class="modal-body" style="text-align: left">
         <ul class="list-group">
          <input type="checkbox" name="org_admin_privileges" id="org_admin_privileges" />
           <%= label_tag :organisational_admin_privileges %>
-          <% perms.each do |perm| %>           
+          <% perms.each do |perm| %>
             <% case perm.name when 'grant_permissions' %>
               <li class="list-group-item" style="border: none" title="<%= _('Allows the user to assign permissions to other users within the same organisation. Users can only assign permissions they own themselves') %>">
                 <%= check_box_tag "perm_ids[]", perm.id, user.perms.include?(perm), class: 'org_grant_privileges' %>
@@ -35,13 +35,17 @@
               <li class="list-group-item" style="border: none" title="<%= _('Allows the user to amend the organisation details (name, URL etc) and add basic branding such as the logo') %>">
                 <%= check_box_tag "perm_ids[]", perm.id, user.perms.include?(perm), class: 'org_grant_privileges' %>
                 <%= _('Manage organisation details') %></li>
+            <% when 'review_org_plans' %>
+              <li class="list-group-item" style="border: none" title="<%= _('Allows the user to review plans submitted by organisational users') %>">
+                <%= check_box_tag "perm_ids[]", perm.id, user.perms.include?(perm), class: 'org_grant_privileges' %>
+                <%= _('Manage organisation details') %></li>
             <% end %>
           <% end %>
 
 
           <% if current_user.can_super_admin? %>
             <input type="checkbox" name="super_admin_privileges" id="super_admin_privileges" />
-            <%= label_tag :super_admin_privileges %> 
+            <%= label_tag :super_admin_privileges %>
             <% perms.each do |perm| %>
               <% case perm.name when 'add_organisations' %>
                 <li class="list-group-item" style="border: none" title="<%= _('Allows the user to create new organisations') %>">
@@ -62,8 +66,8 @@
       </div>
       <div class="modal-footer">
         <%= submit_tag _('Save'), class: "btn btn-primary" %>
-        <%= button_tag(_('Cancel'), class: 'btn btn-primary', 'data-dismiss': 'modal') %>      
+        <%= button_tag(_('Cancel'), class: 'btn btn-primary', 'data-dismiss': 'modal') %>
       </div>
     <% end %>
   </div>
-</div>
+</div>

db/seeds.rb

@@ -182,7 +182,8 @@
   {name: 'modify_guidance'},
   {name: 'use_api'},
   {name: 'change_org_details'},
-  {name: 'grant_api_to_orgs'}
+  {name: 'grant_api_to_orgs'},
+  {name: 'review_org_plans'}
 ]
 
 perms.map{ |p| create(:perm, p) }

lib/tasks/upgrade.rake

@@ -4,6 +4,9 @@ namespace :upgrade do
   desc "Upgrade to v2.1.3"
   task v2_1_3: :environment do
     Rake::Task['upgrade:fill_blank_plan_identifiers'].execute
+    Rake::Task["upgrade:add_reviewer_perm"].execute
+    Rake::Task["upgrade:add_reviewer_to_existing_admin_perms"].execute
+    Rake::Task["upgrade:migrate_reviewer_roles"].execute
   end
 
   desc "Upgrade to v2.1.2:"
@@ -662,11 +665,37 @@ namespace :upgrade do
     end
   end
 
+
   desc "Fill blank or nil plan identifiers with plan_id"
   task fill_blank_plan_identifiers: :environment do
     Plan.where(identifier: ["",nil]).update_all('identifier = id')
   end
 
+  desc "Adds a new permission for plan reviewers"
+  task add_reviewer_perm: :environment do
+    perm_name = 'review_org_plans'
+    unless Perm.find_by(name: perm_name).present?
+      Perm.create(name: perm_name)
+    end
+  end
+
+  desc "adds the new reviewer perm to all existing admin perms"
+  task add_reviewer_to_existing_admin_perms: :environment do
+    Perm.change_org_details.users.each do |u|
+      u.perms << Perm.review_plans
+    end
+  end
+
+  desc "remove the old reviewer roles and ensure these are marked feedback-enabled"
+  task migrate_reviewer_roles: :environment do
+    # remove all roles with nil plan_id
+    Role.reviewer.where(plan_id: nil).destroy_all
+    # Pluck all other plan_ids
+    review_plan_ids = Role.reviewer.pluck(:plan_id).uniq
+    Plan.where(id: review_plan_ids).update_all(feedback_requested: true)
+    Role.reviewer.destroy_all
+  end
+
   private
 
   def fuzzy_match?(text_a, text_b, min = 3)

spec/factories/perms.rb

@@ -51,5 +51,10 @@
       name { 'grant_api_to_orgs' }
       initialize_with { Perm.find_or_create_by(name: name) }
     end
+
+    trait :review_org_plans do
+      name { 'review_org_plans' }
+      initialize_with { Perm.find_or_create_by(name: name) }
+    end
   end
 end