Merge pull request #38860 from code-dot-org/revert-38855-revert-38729…

…-use-feedback-script-id

Restore "use script id to find teacher feedback script level"

dashboard/app/controllers/teacher_feedbacks_controller.rb

@@ -6,14 +6,23 @@ class TeacherFeedbacksController < ApplicationController
   # Feedback from any verified teacher who has provided feedback to the current
   # student on any level
   def index
-    @feedbacks_as_student = @teacher_feedbacks.order(created_at: :desc).includes(script_level: {lesson: :script}).select do |feedback|
+    scope = {
+      level: {
+        script_levels: {
+          lesson: :script
+        }
+      }
+    }
+    @feedbacks_as_student = @teacher_feedbacks.order(created_at: :desc).includes(scope).select do |feedback|
       UserPermission.where(
         user_id: feedback.teacher_id,
         permission: 'authorized_teacher'
       )
     end
 
-    @feedbacks_as_student_with_level_info = @feedbacks_as_student.map {|feedback| feedback.attributes.merge(feedback&.script_level&.summary_for_feedback)}
+    @feedbacks_as_student_with_level_info = @feedbacks_as_student.map do |feedback|
+      feedback.attributes.merge(feedback&.get_script_level&.summary_for_feedback)
+    end
   end
 
   def set_seen_on_feedback_page_at

dashboard/app/models/teacher_feedback.rb

@@ -41,6 +41,22 @@ def validate_script_and_script_level
     end
   end
 
+  # Finds the script level associated with this object, using script id and
+  # level id.
+  def get_script_level
+    script_level = level.script_levels.find {|sl| sl.script_id == script_id}
+    return script_level if script_level
+
+    # This will be somewhat expensive, but will only be executed for feedbacks
+    # which were are associated with a Bubble Choice sublevel.
+    bubble_choice_levels = script.levels.where(type: 'BubbleChoice').all
+    parent_level = bubble_choice_levels.find {|bc| bc.sublevels.include?(level)}
+
+    script_level = parent_level.script_levels.find {|sl| sl.script_id == script_id}
+    raise "no script level found for teacher feedback #{id}" unless script_level
+    script_level
+  end
+
   def self.get_student_level_feedback(student_id, level_id, teacher_id)
     where(
       student_id: student_id,

dashboard/test/controllers/teacher_feedbacks_controller_test.rb

@@ -13,6 +13,9 @@ class TeacherFeedbacksControllerTest < ActionController::TestCase
     sign_in student
     get :index
     assert_response :success
+
+    all_feedback_data = get_all_response_feedback_data
+    assert_equal 0, all_feedback_data.count
   end
 
   test 'index: returns success if signed in user - feedback' do
@@ -21,5 +24,33 @@ class TeacherFeedbacksControllerTest < ActionController::TestCase
     sign_in feedback.student
     get :index
     assert_response :success
+
+    all_feedback_data = get_all_response_feedback_data
+    assert_equal 1, all_feedback_data.count
+    assert_equal feedback.student.id, all_feedback_data.first['student_id']
+  end
+
+  test 'index returns many feedbacks' do
+    student = create :student
+    5.times do
+      create :teacher_feedback, student: student
+    end
+    assert_equal TeacherFeedback.all.count, 5
+    sign_in student
+    assert_queries 20 do
+      get :index
+      assert_response :success
+    end
+
+    all_feedback_data = get_all_response_feedback_data
+    assert_equal 5, all_feedback_data.count
+  end
+
+  private
+
+  def get_all_response_feedback_data
+    assert_select 'script[data-feedback]', 1
+    feedback_data = JSON.parse(css_select('script[data-feedback]').first.attribute('data-feedback').to_s)
+    feedback_data['all_feedback']
   end
 end

dashboard/test/models/teacher_feedback_test.rb

@@ -179,4 +179,46 @@ class TeacherFeedbackTest < ActiveSupport::TestCase
       assert_equal datetime2, feedback.student_last_visited_at
     end
   end
+
+  test 'get_script_level finds level in script' do
+    script_level = create :script_level
+    script = script_level.script
+    level = script_level.levels.first
+    feedback = create :teacher_feedback, script: script, level: level
+    assert_queries(1) do
+      assert_equal script_level, feedback.get_script_level
+    end
+  end
+
+  test 'get_script_level finds bubble choice parent level' do
+    parent_level = create :bubble_choice_level, :with_sublevels
+    child_level = parent_level.sublevels.first
+
+    # Create these intermediate rungs of the hierarchy, so that script_level
+    # will show up in script.script_levels.
+    script = create :script
+    lesson_group = create :lesson_group, script: script
+    lesson = create :lesson, lesson_group: lesson_group, script: script
+
+    # the query count grows with the number of bubble choice levels in the script.
+    create :script_level, script: script, lesson: lesson, levels: [create(:bubble_choice_level, :with_sublevels)]
+    create :script_level, script: script, lesson: lesson, levels: [create(:bubble_choice_level, :with_sublevels)]
+    create :script_level, script: script, lesson: lesson, levels: [create(:bubble_choice_level, :with_sublevels)]
+
+    script_level = create :script_level, script: script, lesson: lesson, levels: [parent_level]
+
+    # HACK: we have to supply a script_level, because it is still a required field.
+    # According to existing validations, it has to be a script level in this script.
+    # However, we don't want to pass script_level itself, because in theory
+    # get_script_level could "cheat" and just return that value. Therefore, create
+    # a new script level within the same script as a way to prevent cheating and
+    # still pass validations.
+    # TODO: remove this hack once script_level is no longer required.
+    other_script_level = create :script_level, script: script, lesson: lesson, levels: [create(:level)]
+
+    feedback = create :teacher_feedback, script: script, level: child_level, script_level: other_script_level
+    assert_queries(7) do
+      assert_equal script_level, feedback.get_script_level
+    end
+  end
 end