test: enhance feedback chip tests

Author: macite

app/api/feedback/feedback_chip_api.rb

@@ -30,10 +30,8 @@ class FeedbackChipApi < Grape::API
       unless authorise? current_user, User, :get_feedback_chips
         error!({ error: 'You are not authorised to view feedback chips globally.' }, 403)
       end
-      learning_outcomes = LearningOutcome.global_outcomes
-      feedback_chips = learning_outcomes.includes(:feedback_chips).map(&:feedback_chips).flatten
 
-      present feedback_chips, with: Feedback::Entities::FeedbackChipEntity
+      present Feedback::FeedbackChip.global_chips, with: Feedback::Entities::FeedbackChipEntity
     end
 
     desc 'Add a feedback chip to a learning outcome'
@@ -125,11 +123,12 @@ class FeedbackChipApi < Grape::API
       requires :id, type: Integer, desc: 'The ID of the feedback chip'
     end
     delete '/feedback_chips/:id' do
-      unless authorise? current_user, User, :feedback_chips
+      chip = FeedbackChip.find(params[:id])
+
+      unless authorise? current_user, chip, :delete_feedback_chips
         error!({ error: 'You are not authorised to delete feedback chips.' }, 403)
       end
 
-      chip = FeedbackChip.find(params[:id])
       chip.destroy
       nil
     end

app/models/feedback/feedback_chip.rb

@@ -37,11 +37,13 @@ class FeedbackChip < ApplicationRecord
 
     def self.permissions
       convenor_role_permissions = [
-        :update_chip
+        :update_chip,
+        :delete_feedback_chips
       ]
 
       admin_role_permissions = [
-        :update_chip
+        :update_chip,
+        :delete_feedback_chips
       ]
 
       nil_role_permissions = []
@@ -64,6 +66,10 @@ def track_usage_by(tutor)
       analytics.save
     end
 
+    def self.global_chips
+      Feedback::FeedbackChip.joins(:learning_outcome).where(learning_outcome: {context_type: nil, context_id: nil})
+    end
+
     def children
       FeedbackChip.where(parent_chip_id: self.id, learning_outcome_id: self.learning_outcome_id)
     end

test/api/feedback/feedback_chip_api_consolidated_test.rb

@@ -42,7 +42,7 @@ def test_auth_for_create_edit_unit_feedback_chips
     }
 
     data_to_put = {
-      chip_text: 'Updated text',
+      chip_text: 'Updated text'
     }
 
     learning_outcomes.each do |lo|
@@ -243,6 +243,79 @@ def test_auth_for_get_global_feedback_chips
     end
   end
 
+  def test_auth_for_delete_feedback_chips
+    start_inside = DateTime.now - Doubtfire::Application.config.auditor_unit_access_years + 1.week
+    end_inside = DateTime.now - 1.week
+
+    unit = FactoryBot.create(:unit, student_count: 1, start_date: start_inside, end_date: end_inside)
+    admin = FactoryBot.create(:user, :admin)
+    tutor = FactoryBot.create(:user, :tutor)
+    auditor = FactoryBot.create(:user, :auditor)
+    student = unit.students.first
+
+
+    learning_outcomes = [
+      {
+        learning_outcome: unit.learning_outcomes.first,
+        context: 'ULO'
+      },
+      {
+        learning_outcome: unit.task_definitions.first.learning_outcomes.first,
+        context: 'TLO'
+      }
+    ]
+
+    unit.employ_staff(tutor, Role.tutor)
+
+    users_can = [
+      unit.main_convenor_user,
+      admin
+    ]
+    users_cant = [
+      tutor,
+      auditor,
+      FactoryBot.create(:user, :student),
+      FactoryBot.create(:user, :tutor),
+      FactoryBot.create(:user, :convenor),
+      student.user
+    ]
+
+    assert_equal Role.auditor, unit.role_for(auditor)
+
+    users_can.each do |user|
+      add_auth_header_for user: user
+      learning_outcomes.each do |lo_data|
+        lo = lo_data[:learning_outcome]
+        context = lo_data[:context]
+
+        chip = FactoryBot.create(:feedback_template_chip, learning_outcome_id: lo.id)
+
+        chip_count = lo.feedback_chips.count
+
+        delete "api/feedback_chips/#{chip.id}"
+        assert_equal 204, last_response.status, "User #{user.role.name} should be able to delete #{context} feedback chips"
+        assert_equal chip_count - 1, lo.feedback_chips.count
+      end
+    end
+
+    users_cant.each do |user|
+      add_auth_header_for user: user
+
+      learning_outcomes.each do |lo_data|
+        lo = lo_data[:learning_outcome]
+        context = lo_data[:context]
+
+        chip = FactoryBot.create(:feedback_template_chip, learning_outcome_id: lo.id)
+
+        chip_count = lo.feedback_chips.count
+
+        delete "api/feedback_chips/#{chip.id}"
+        assert_equal 403, last_response.status, "User #{user.role.name} should not be able to delete #{context} feedback chips"
+        assert_equal chip_count, lo.feedback_chips.count
+      end
+    end
+  end
+
   def test_create_feedback_group_chip
     learning_outcome = FactoryBot.create(:learning_outcome)
     data_to_post = {
@@ -323,7 +396,7 @@ def test_create_feedback_template_chip_with_parent
   end
 
   def test_get_all_feedback_chips_for_a_context
-    unit = Unit.first
+    unit = FactoryBot.create(:unit, with_students: false, task_count: 0, outcome_count: 0)
     learning_outcome = FactoryBot.create(:learning_outcome, context_id: unit.id, context_type: 'Unit')
     learning_outcome2 = FactoryBot.create(:learning_outcome, context_id: unit.id, context_type: 'Unit')
     group_chip_lo1 = FactoryBot.create(:feedback_group_chip, learning_outcome_id: learning_outcome.id)
@@ -335,70 +408,98 @@ def test_get_all_feedback_chips_for_a_context
     add_auth_header_for user: User.first
     get "api/units/#{unit.id}/feedback_chips"
     assert_equal 200, last_response.status
+
+    assert_equal 6, last_response_body.count, last_response_body
+
+    chips = [learning_outcome.feedback_chips.pluck(:id), learning_outcome2.feedback_chips.pluck(:id)].flatten
+
+    last_response_body.each do |line|
+      assert chips.include?(line['id']), "Found unknown chip #{line}"
+    end
   end
 
   def test_update_feedback_template_chip
     learning_outcome = FactoryBot.create(:learning_outcome)
     group_chip = FactoryBot.create(:feedback_group_chip, learning_outcome_id: learning_outcome.id)
     template_chip = FactoryBot.create(:feedback_template_chip, chip_text: 'chippy', description: 'blah blah', comment_text: 'your work is horrible', summary_text: 'just plain bad', task_status: TaskStatus.complete.name, learning_outcome_id: learning_outcome.id, parent_chip_id: group_chip.id)
-    data_to_post = {
+    data_to_put = {
       chip_text: 'updated chip',
       description: 'updated description'
     }
     add_auth_header_for user: User.first
-    put_json "api/feedback_chips/#{template_chip.id}", data_to_post
+    put_json "api/feedback_chips/#{template_chip.id}", data_to_put
     assert_equal 200, last_response.status
+
+    template_chip.reload
+    assert_json_matches_model template_chip, last_response_body, %w[id chip_text description]
+    assert_json_matches_model template_chip, data_to_put, %w[chip_text description]
   end
 
   def test_update_feedback_group_chip
     learning_outcome = FactoryBot.create(:learning_outcome)
     group_chip = FactoryBot.create(:feedback_group_chip, learning_outcome_id: learning_outcome.id)
-    data_to_post = {
+    data_to_put = {
       chip_text: 'updated chip text',
       description: 'updated description'
     }
     add_auth_header_for user: User.first
-    put_json "api/feedback_chips/#{group_chip.id}", data_to_post
+    put_json "api/feedback_chips/#{group_chip.id}", data_to_put
     assert_equal 200, last_response.status
+
+    group_chip.reload
+    assert_json_matches_model group_chip, last_response_body, %w[id chip_text description]
+    assert_json_matches_model group_chip, data_to_put, %w[chip_text description]
   end
 
   def test_delete_feedback_template_chip
     learning_outcome = FactoryBot.create(:learning_outcome)
     group_chip = FactoryBot.create(:feedback_group_chip, learning_outcome_id: learning_outcome.id)
     template_chip = FactoryBot.create(:feedback_template_chip, chip_text: 'chippy', description: 'blah blah', comment_text: 'your work is horrible', summary_text: 'just plain bad', task_status: TaskStatus.complete.name, learning_outcome_id: learning_outcome.id, parent_chip_id: group_chip.id)
     add_auth_header_for user: User.first
+
+    chip_count = learning_outcome.feedback_chips.count
+
     delete "api/feedback_chips/#{template_chip.id}"
     assert_equal 204, last_response.status
+
+    assert_equal chip_count - 1, learning_outcome.feedback_chips.count
   end
 
   def test_delete_feedback_group_chip
     learning_outcome = FactoryBot.create(:learning_outcome)
     group_chip = FactoryBot.create(:feedback_group_chip, learning_outcome_id: learning_outcome.id)
     add_auth_header_for user: User.first
+
+    chip_count = learning_outcome.feedback_chips.count
+
     delete "api/feedback_chips/#{group_chip.id}"
     assert_equal 204, last_response.status
+
+    assert_equal chip_count - 1, learning_outcome.feedback_chips.count
   end
 
-  def test_change_chip_type_t_to_g
+  def test_cannot_change_chip_type_t_to_g
     learning_outcome = FactoryBot.create(:learning_outcome)
     group_chip = FactoryBot.create(:feedback_group_chip, learning_outcome_id: learning_outcome.id)
     template_chip = FactoryBot.create(:feedback_template_chip, chip_text: 'chippy', description: 'blah blah', comment_text: 'your work is horrible', summary_text: 'just plain bad', task_status: TaskStatus.complete.name, learning_outcome_id: learning_outcome.id, parent_chip_id: group_chip.id)
-    # puts template_chip.inspect
-    data_to_post = {
+
+    data_to_put = {
       type: 'group',
       chip_text: 'Sample chip text',
       description: 'Sample description'
     }
     add_auth_header_for user: User.first
-    put_json "api/feedback_chips/#{template_chip.id}", data_to_post
-    # puts last_response.body
+    put_json "api/feedback_chips/#{template_chip.id}", data_to_put
     assert_equal 200, last_response.status
+
+    template_chip.reload
+    assert_equal 'Feedback::FeedbackTemplateChip', template_chip.type
   end
 
-  def test_change_chip_type_g_to_t
+  def test_cannot_change_chip_type_g_to_t
     learning_outcome = FactoryBot.create(:learning_outcome)
     group_chip = FactoryBot.create(:feedback_group_chip, learning_outcome_id: learning_outcome.id)
-    # puts group_chip.inspect
+
     data_to_post = {
       type: 'template',
       chip_text: 'Sample chip text',
@@ -411,21 +512,11 @@ def test_change_chip_type_g_to_t
     }
     add_auth_header_for user: User.first
     put_json "api/feedback_chips/#{group_chip.id}", data_to_post
-    # puts last_response.body
-    assert_equal 200, last_response.status
-  end
 
-  def test_update_without_type
-    learning_outcome = FactoryBot.create(:learning_outcome)
-    group_chip = FactoryBot.create(:feedback_group_chip, learning_outcome_id: learning_outcome.id)
-    data_to_post = {
-      chip_text: 'updated chip text',
-      description: 'updated description'
-    }
-    add_auth_header_for user: User.first
-    put_json "api/feedback_chips/#{group_chip.id}", data_to_post
-    # puts last_response.body
     assert_equal 200, last_response.status
+
+    group_chip.reload
+    assert_equal 'Feedback::FeedbackGroupChip', group_chip.type
   end
 
   def test_get_global_context_chips
@@ -437,7 +528,16 @@ def test_get_global_context_chips
     feedback_template_chip2 = FactoryBot.create(:feedback_template_chip, learning_outcome_id: learning_outcome2.id, parent_chip_id: feedback_group_chip2.id)
     add_auth_header_for user: User.first
     get 'api/global/feedback_chips'
-    # puts last_response.body
+
+    global_chips = Feedback::FeedbackChip.global_chips
+    chip_count = global_chips.count
+
     assert_equal 200, last_response.status
+    assert_equal chip_count, last_response_body.count
+
+    ids = global_chips.pluck(:id)
+    last_response_body.each do |line|
+      assert ids.include?(line['id'])
+    end
   end
 end

test/factories/units_factory.rb

@@ -17,7 +17,7 @@
     tutorial_stream           { unit.tutorial_streams.sample }
 
     transient do
-      outcome_count             { 2 }
+      outcome_count { 2 }
     end
 
     after(:create) do |td, eval|

test/helpers/json_helper.rb

@@ -36,26 +36,31 @@ def delete_json(endpoint)
     # - key data is either a hash that maps response to model keys, or a list of keys to match
     def assert_json_matches_model(model, response_json, keys_data)
       if keys_data.instance_of? Hash
-        response_keys = keys_data.keys.map {|k| k.to_s }
+        response_keys = keys_data.keys.map(&:to_s)
         keys = keys_data
       else
         response_keys = keys_data
-        keys = keys_data.map { |i| [i, i] }.to_h
+        keys = keys_data.to_h { |i| [i, i] }
       end
-      response_keys.each { |k| assert response_json.key?(k), "Response missing key #{k} - #{response_json}" }
-      response_keys.each { |k|
+      response_keys.each do |k|
+        assert response_json.key?(k) || response_json.key?(k.to_sym), "Response missing key #{k} - #{response_json}"
         mk = keys[k] || keys[k.to_sym]
-        value = model.is_a?(Hash) ? (model[mk].nil? ? model[mk.to_sym] : model[mk]) : model.send(mk)
-        if ! value.nil?
-          assert_equal value, response_json[k], "Values for model key #{mk} does not match value of response key #{k} - #{response_json}"
+        value = if model.is_a?(Hash)
+                  model[mk].nil? ? model[mk.to_sym] : model[mk]
+                else
+                  model.send(mk)
+                end
+        model_value = response_json.key?(k) ? response_json[k] : response_json[k.to_sym]
+        if value.present?
+          assert_equal value, model_value, "Values for model key #{mk} does not match value of response key #{k} - #{response_json}"
         else
-          assert_nil response_json[k], "Values for key #{k} is not nil - #{response_json}"
+          assert_nil model_value, "Values for key #{k} is not nil - #{response_json}"
         end
-      }
+      end
     end
 
     def assert_json_limit_keys_to(keys, response_json)
-      response_json.keys.each do |k|
+      response_json.each_key do |k|
         assert keys.include?(k), "Unexpected key in response: #{k} -- #{response_json.inspect}"
       end
     end