feat: Project Admins can manage Materials Collections [PT-187061879]

Adds the ability for project admins to manage materials collections.

rails/app/controllers/api/v1/materials_collections_controller.rb

@@ -1,10 +1,8 @@
 class API::V1::MaterialsCollectionsController < API::APIController
-  include RestrictedController
-  before_action :admin_only
 
-  def sort_materials
-    materials_collection = MaterialsCollection.find(params[:id])
+  before_action :find_and_authorize_material_collection
 
+  def sort_materials
     item_ids = params['item_ids']
     if !item_ids
       return error("Missing item_ids parameter")
@@ -13,7 +11,7 @@ def sort_materials
     items = item_ids.map { |i| MaterialsCollectionItem.find(i) }
     position = 0
     items.each do |item|
-      if item.materials_collection_id == materials_collection.id
+      if item.materials_collection_id == @materials_collection.id
         item.position = position
         position = position + 1
         item.save
@@ -23,15 +21,12 @@ def sort_materials
   end
 
   def remove_material
-    id = params[:id]
-    materials_collection = MaterialsCollection.find(id)
-
     item_id = params[:item_id]
     if !item_id
       return error("Missing item_id parameter")
     end
 
-    item = MaterialsCollectionItem.where(id: item_id, materials_collection_id: id).first
+    item = MaterialsCollectionItem.where(id: item_id, materials_collection_id: @materials_collection.id).first
     if !item
       error("Invalid item id: #{item_id}")
     elsif !item.destroy
@@ -46,4 +41,9 @@ def remove_material
   def render_ok
     render :json => { success: true }, :status => :ok
   end
+
+  def find_and_authorize_material_collection
+    @materials_collection = MaterialsCollection.find(params[:id])
+    authorize @materials_collection
+  end
 end

rails/app/controllers/external_activities_controller.rb

@@ -263,7 +263,7 @@ def copy
   def edit_collections
     authorize @external_activity
 
-    @collections = MaterialsCollection.includes(:materials_collection_items).order(:name).all
+    @collections = policy_scope(MaterialsCollection).includes(:materials_collection_items).order(:name).all
 
     @material = [@external_activity]
     @assigned_collections = @collections.select{|c| c.has_materials(@material) }

rails/app/controllers/materials_collections_controller.rb

@@ -1,12 +1,15 @@
 class MaterialsCollectionsController < ApplicationController
-  include RestrictedController
-  before_action :admin_only
+
+  before_action :find_and_authorize_material_collection, only: ['show', 'edit', 'update', 'destroy']
+  before_action :load_projects
 
   # GET /materials_collections
   # GET /materials_collections.json
   def index
-    filtered = params[:project_id].to_s.length > 0 ? MaterialsCollection.where({project_id: params[:project_id]}) : MaterialsCollection
-    @materials_collections = filtered.search(params[:search], params[:page], nil)
+    authorize MaterialsCollection
+    search_scope = policy_scope(MaterialsCollection)
+    search_scope = search_scope.where(project_id: params[:project_id]) if params[:project_id].to_s.length > 0
+    @materials_collections = MaterialsCollection.search(params[:search], params[:page], nil, nil, search_scope)
     respond_to do |format|
       format.html # index.html.haml
       format.json { render json: @materials_collections }
@@ -16,7 +19,6 @@ def index
   # GET /materials_collections/1
   # GET /materials_collections/1.json
   def show
-    @materials_collection = MaterialsCollection.find(params[:id])
     respond_to do |format|
       format.html # show.html.haml
       format.json { render json: @materials_collection }
@@ -26,6 +28,7 @@ def show
   # GET /materials_collections/new
   # GET /materials_collections/new.json
   def new
+    authorize MaterialsCollection
     @materials_collection = MaterialsCollection.new
     respond_to do |format|
       format.html # new.html.haml
@@ -35,7 +38,6 @@ def new
 
   # GET /materials_collections/1/edit
   def edit
-    @materials_collection = MaterialsCollection.find(params[:id])
     # renders edit.html.haml
   end
 
@@ -58,7 +60,6 @@ def create
   # PATCH/PUT /materials_collections/1
   # PATCH/PUT /materials_collections/1.json
   def update
-    @materials_collection = MaterialsCollection.find(params[:id])
     respond_to do |format|
       if @materials_collection.update(materials_collection_strong_params(params[:materials_collection]))
         format.html { redirect_to @materials_collection, notice: 'Materials Collection was successfully updated.' }
@@ -73,7 +74,6 @@ def update
   # DELETE /materials_collections/1
   # DELETE /materials_collections/1.json
   def destroy
-    @materials_collection = MaterialsCollection.find(params[:id])
     @materials_collection.destroy
     respond_to do |format|
       format.html { redirect_to materials_collections_url }
@@ -84,4 +84,16 @@ def destroy
   def materials_collection_strong_params(params)
     params && params.permit(:description, :name, :project_id)
   end
+
+  private
+
+  def find_and_authorize_material_collection
+    @materials_collection = MaterialsCollection.find(params[:id])
+    authorize @materials_collection
+  end
+
+  def load_projects
+    @projects = policy_scope(Admin::Project)
+  end
+
 end

rails/app/policies/external_activity_policy.rb

@@ -74,11 +74,11 @@ def unarchive?
   end
 
   def edit_collections?
-    admin?
+    admin_or_project_admin?
   end
 
   def update_collections?
-    admin?
+    admin_or_project_admin?
   end
 
 end

rails/app/policies/materials_collection_policy.rb

@@ -1,2 +1,60 @@
 class MaterialsCollectionPolicy < ApplicationPolicy
+
+  class Scope < Scope
+    def resolve
+      if user && user.has_role?('admin')
+        all
+      elsif user
+        # prevents a bunch of unnecessary model loads by not using the user#admin_for_project_cohorts method
+        scope
+          .joins("INNER JOIN admin_project_users __apu_scope ON __apu_scope.project_id = materials_collections.project_id")
+          .where("__apu_scope.user_id = ? AND __apu_scope.is_admin = 1", user.id)
+      else
+        none
+      end
+    end
+  end
+
+  def new?
+    admin_or_project_admin?
+  end
+
+  def create?
+    check_project
+  end
+
+  def edit?
+    check_project
+  end
+
+  def update?
+    check_project
+  end
+
+  def show?
+    check_project
+  end
+
+  def destroy?
+    check_project
+  end
+
+  def sort_materials?
+    check_project
+  end
+
+  def remove_material?
+    check_project
+  end
+
+  private
+
+  def check_project
+    if(record.project)
+      admin? || user.is_project_admin?(record.project)
+    else
+      admin_or_project_admin?
+    end
+  end
+
 end

rails/app/views/external_activities/edit_collections.html.haml

@@ -28,7 +28,7 @@
                           %br
                           (Already assigned as part of "#{parent_name}")
           - else
-            .messagetext{:style=>"padding-left:5px"} This material is assigned to all the collections.
+            .messagetext{:style=>"padding-left:5px"} This material is assigned to all the collections to which you have access.
           - if @assigned_collections.length > 0
             %br
             %br

rails/app/views/home/admin.html.haml

@@ -42,6 +42,7 @@
         %li= link_to 'Permission Forms', admin_permission_forms_path
       - if is_admin_or_project_admin
         %li= link_to 'Projects', admin_projects_path
+        %li= link_to 'Materials Collections', materials_collections_path
         %li= link_to 'Users', users_path
 
     - if current_visitor.has_role?('admin', 'manager','researcher') || current_visitor.is_project_admin? || current_visitor.is_project_researcher?

rails/app/views/materials_collections/_form.html.haml

@@ -9,7 +9,7 @@
           =f.text_field :name
         %li
           Project:
-          =f.collection_select :project_id, Admin::Project.order("name ASC"), :id, :name, :prompt => "Select project..."
+          =f.collection_select :project_id, @projects, :id, :name, :prompt => "Select project..."
         %li
           Description:
           =f.text_area :description

rails/app/views/materials_collections/index.html.haml

@@ -1,4 +1,4 @@
-- extra_search_fields = select_tag :project_id, options_from_collection_for_select(Admin::Project.order("name ASC"), "id", "name", selected: params[:project_id]), prompt: "Select project..."
+- extra_search_fields = select_tag :project_id, options_from_collection_for_select(@projects, "id", "name", selected: params[:project_id]), prompt: "Select project..."
 = render :partial => 'shared/collection_menu', :locals => { :collection => @materials_collections, :collection_class => MaterialsCollection, :extra_collection_search_fields => extra_search_fields }
 = render :partial => 'list_show', :collection => @materials_collections, :as => :materials_collection
 

rails/lib/materials/data_helpers.rb

@@ -460,7 +460,7 @@ def links_for_material( material,
         }
       end
 
-      if current_visitor.has_role?('admin') && material.respond_to?(:materials_collections)
+      if (current_visitor.has_role?('admin') || current_visitor.is_project_admin?()) && material.respond_to?(:materials_collections)
         links[:assign_collection] = {
           text: "Add to Collection",
           target: "_blank",

rails/spec/controllers/api/v1/materials_collections_controller_spec.rb

@@ -1,18 +1,21 @@
 require 'spec_helper'
 
 describe API::V1::MaterialsCollectionsController do
-  let(:non_admin) { FactoryBot.create(:confirmed_user) }
-  let(:admin)     { FactoryBot.generate(:admin_user) }
+  let(:non_admin)     { FactoryBot.create(:confirmed_user) }
+  let(:admin)         { FactoryBot.generate(:admin_user) }
+  let(:project_admin) { FactoryBot.generate(:author_user) }
 
   describe "As a non-admin" do
+    let (:collection) { FactoryBot.create(:materials_collection_with_items) }
+
     before(:each) do
       sign_in non_admin
     end
 
     describe "each api endpoint" do
       [:sort_materials, :remove_material].each do |method|
         it("should fail") do
-          post method, params: {id: 1}
+          post method, params: {id: collection.id}
           expect(response.status).to eql(403)
           expect(response.body).to eql('{"success":false,"message":"Not authorized"}')
         end
@@ -85,4 +88,72 @@
     end
   end
 
+
+  describe "As a project admin" do
+    let (:project) { FactoryBot.create(:project) }
+    let (:collection) { FactoryBot.create(:materials_collection_with_items, project: project) }
+
+    before(:each) do
+      sign_in project_admin
+      project_admin.add_role_for_project('admin', project)
+    end
+
+    describe "sort_materials" do
+      it "should fail with a valid id" do
+        post :sort_materials, params: {id: 0}
+        expect(response.status).to eql(404)
+      end
+
+      it "should fail without item ids" do
+        post :sort_materials, params: { id: collection.id }
+        expect(response.status).to eql(400)
+        expect(response.body).to eql('{"success":false,"response_type":"ERROR","message":"Missing item_ids parameter"}')
+      end
+
+      it "should succeed" do
+        # generate randomly sorted ids
+        randomized_item_ids = collection.materials_collection_items.map{|mci| mci.id}.shuffle
+        post :sort_materials, params: { id: collection.id, item_ids: randomized_item_ids }
+        expect(response.status).to eql(200)
+        expect(response.body).to eql('{"success":true}')
+
+        # ensure the randomly sorted ids were saved
+        collection.materials_collection_items.reload
+        ids = collection.materials_collection_items.map{|mci| mci.id}
+        expect(ids).to eql(randomized_item_ids)
+      end
+    end
+
+    describe "remove_material" do
+      it "should fail with an invalid id" do
+        post :remove_material, params: {id: 0}
+        expect(response.status).to eql(404)
+      end
+
+      it "should fail without an item id" do
+        post :remove_material, params: { id: collection.id }
+        expect(response.status).to eql(400)
+        expect(response.body).to eql('{"success":false,"response_type":"ERROR","message":"Missing item_id parameter"}')
+      end
+
+      it "should fail with an invalid item id" do
+        post :remove_material, params: { id: collection.id, item_id: 0 }
+        expect(response.status).to eql(400)
+        expect(response.body).to eql('{"success":false,"response_type":"ERROR","message":"Invalid item id: 0"}')
+      end
+
+      it "should succeed" do
+        item = collection.materials_collection_items[0]
+        length_before_delete = collection.materials_collection_items.length
+
+        post :remove_material, params: { id: collection.id, item_id: item.id }
+        expect(response.status).to eql(200)
+        expect(response.body).to eql('{"success":true}')
+
+        collection.materials_collection_items.reload
+        expect(collection.materials_collection_items.length).to eq(length_before_delete - 1)
+      end
+    end
+  end
+
 end