Quiz Stats - Whitelist exposed question data

Changes:

  - drops unused attributes from the api output, the only things we
    expose now are question id, type, text, and position (for sorting)
  - drops logic in ember that is no longer necessary
  - stringifies IDs in the serializer output

Closes CNVS-13388

TEST PLAN
---- ----

  - BEFORE YOU CHECK THIS OUT:
    - create a quiz with all question types
    - hit the stats API and save the JSON output in some text file
  - now check this patch out:
    - use the same quiz, hit the stats api
    - verify that each question document in "question_statistics" has
      the following fields beside the metrics:
      "id", "question_type", "question_name", and "position"
    - verify that MChoice/TF questions still contain the "item analysis"
      report data (like point_biserials, etc.)
    - check out the ember quiz stats and make sure everything is OK

Note: i found a small tool that can help you see the differences in the
API output if you push the old JSON and the new one into it: try it out
maybe it will make things easier: http://tlrobinson.net/

Note 2: the docs are really out-of-date now and need to be adjusted.
There is CNVS-13387 for that.

Change-Id: I2d2e8c4dcb0e406378b50cd63f5aba14efe8c2ef
Reviewed-on: https://gerrit.instructure.com/35739
Reviewed-by: Jason Madsen <jmadsen@instructure.com>
QA-Review: Caleb Guanzon <cguanzon@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>
Product-Review: Ahmad Amireh <ahmad@instructure.com>

app/coffeescripts/ember/quizzes/models/question_statistics.coffee

@@ -14,16 +14,14 @@ define [
     questionName: attr()
     questionText: attr()
     position: attr()
-    answers: attr()
-    pointBiserials: attr()
-    responses: attr()
-    responseValues: attr()
-    unexpectedResponseValues: attr()
 
+    # Shared
+    answers: attr()
+    responses: attr()
     correct: attr('number')
     partiallyCorrect: attr('number')
 
-    # MC/TF stats
+    # Multiple-Choice & True/False
     topStudentCount: attr()
     middleStudentCount: attr()
     bottomStudentCount: attr()
@@ -34,15 +32,30 @@ define [
     correctTopStudentCount: attr()
     correctMiddleStudentCount: attr()
     correctBottomStudentCount: attr()
+    pointBiserials: attr()
+    discriminationIndex: (->
+      if pointBiserials = @get('pointBiserials')
+        pointBiserials.findBy('correct', true).point_biserial
+    ).property('pointBiserials')
 
-    # Essay stats
-    fullCredit: attr()
+    # Essay
     graded: attr()
     pointDistribution: attr()
-
     speedGraderUrl: alias('quizStatistics.quiz.speedGraderUrl').readOnly()
+
+    # Essay & Numerical
+    fullCredit: attr()
+
+    # File Upload
     quizSubmissionsZipUrl: alias('quizStatistics.quiz.quizSubmissionsZipUrl').readOnly()
 
+    # Multiple-Dropdowns, FIMB, Matching
+    answerSets: (->
+      sets = @get('_data.answer_sets') || []
+      sets.map (set) ->
+        Em.Object.create(set)
+    ).property('_data.answer_sets')
+
     # Helper for calculating the ratio of correct responses for this question.
     #
     # Usage: @get('questionStatistics.ratioCalculator.ratio')
@@ -50,12 +63,7 @@ define [
       ResponseRatioCalculator.create({ content: this })
     ).property('answers')
 
-    answerSets: (->
-      sets = @get('_data.answer_sets') || []
-      sets.map (set) ->
-        Em.Object.create(set)
-    ).property('_data.answer_sets')
-
+    # @internal
     renderableType: (->
       switch @get('questionType')
         when 'multiple_choice_question', 'true_false_question'
@@ -73,10 +81,3 @@ define [
         else
           'generic'
     ).property('questionType')
-
-    discriminationIndex: (->
-      if pointBiserials = @get('pointBiserials')
-        pointBiserials.findBy('correct', true).point_biserial
-      else
-        return undefined
-    ).property('pointBiserials')

app/coffeescripts/ember/quizzes/models/question_statistics/response_ratio_calculator.coffee

@@ -9,8 +9,6 @@ define [ 'ember' ], (Em) ->
   # from you.
   #
   # Usage: see QuestionStatistics#ratioCalculator.
-  #
-  # TODO: this *may* be better done on the API, investigate
   Calculator = Em.ObjectProxy.extend
     participantCount: Em.computed.alias('quizStatistics.uniqueCount')
 

app/coffeescripts/ember/quizzes/serializers/quiz_statistics_serializer.coffee

@@ -4,25 +4,35 @@ define [
   'i18n!quiz_statistics'
   '../shared/util'
 ], (Ember, DS, I18n, Util) ->
-  {round} = Util
-  participantCount = 0
-
-  calculateResponseRatio = (answer) ->
-    if participantCount > 0 # guard against div by zero
-      round(answer.responses / participantCount * 100)
-    else
-      0
-
   decorate = (quiz_statistics) ->
+    {round} = Util
     participantCount = quiz_statistics.submission_statistics.unique_count
 
-    quiz_statistics.question_statistics.forEach (question_statistics) ->
-      question_statistics.id = "#{question_statistics.id}"
+    calculateResponseRatio = (answer) ->
+      if participantCount > 0 # guard against div by zero
+        round(answer.responses / participantCount * 100)
+      else
+        0
 
+    decorateAnswers = (answers) ->
+      answers.forEach (answer) ->
+        # the ratio of responses for each answer out of the question's total responses
+        answer.ratio = calculateResponseRatio(answer)
+
+        if answer.id == 'none'
+          answer.text = I18n.t('no_answer', 'No Answer')
+        else if answer.id == 'other'
+          answer.text = I18n.t('unknown_answer', 'Something Else')
+
+    decorateAnswerSet = (answerSet) ->
+      decorateAnswers answerSet.answers || []
+
+    quiz_statistics.question_statistics.forEach (question_statistics) ->
       # assign a FK between question and quiz statistics
       question_statistics.quiz_statistics_id = quiz_statistics.id
 
-      decorateAnswers(question_statistics.answers)
+      if question_statistics.answers
+        decorateAnswers(question_statistics.answers)
 
       if question_statistics.answer_sets
         question_statistics.answer_sets.forEach(decorateAnswerSet)
@@ -31,27 +41,6 @@ define [
     quiz_statistics.question_statistics_ids =
       Ember.A(quiz_statistics.question_statistics).mapBy('id')
 
-  decorateAnswers = (answers) ->
-    (answers || []).forEach (answer) ->
-      # the ratio of responses for each answer out of the question's total responses
-      answer.ratio = calculateResponseRatio(answer)
-
-      # define a "correct" boolean so we can use it with bind-attr:
-      if answer.correct == undefined and answer.weight != undefined
-        answer.correct = answer.weight == 100
-        # remove the weight to make sure the front-end isn't using this because
-        # it will go away from the API output soon:
-        delete answer.weight
-
-  decorateAnswerSet = (answerSet) ->
-    (answerSet.answers || []).forEach (answer, i) ->
-      answer.ratio = calculateResponseRatio(answer)
-
-      if answer.id == 'none'
-        answer.text = I18n.t('no_answer', 'No Answer')
-      else if answer.id == 'other'
-        answer.text = I18n.t('unknown_answer', 'Something Else')
-
   DS.ActiveModelSerializer.extend
     extractArray: (store, type, payload, id, requestType) ->
       decorate(payload.quiz_statistics[0])

app/models/quizzes/quiz_statistics/student_analysis.rb

@@ -21,6 +21,14 @@ require 'csv'
 class Quizzes::QuizStatistics::StudentAnalysis < Quizzes::QuizStatistics::Report
   CQS = CanvasQuizStatistics
 
+  # The question_data attributes that we'll expose with the question statistics
+  CQS_QuestionDataFields = %w[
+    id
+    question_type
+    question_text
+    position
+  ].map(&:to_sym).freeze
+
   include HtmlTextHelper
 
   def readable_type
@@ -360,13 +368,16 @@ class Quizzes::QuizStatistics::StudentAnalysis < Quizzes::QuizStatistics::Report
   #   "multiple_responses"=>false}],
   def stats_for_question(question, responses, legacy=true)
     if !legacy && CQS.can_analyze?(question)
+      output = {}
+
       # the gem expects all hash keys to be symbols:
       question = CQS::Util.deep_symbolize_keys(question.to_hash)
       responses = responses.map(&CQS::Util.method(:deep_symbolize_keys))
 
-      analysis = CQS.analyze(question, responses)
+      output.merge! question.slice(*CQS_QuestionDataFields)
+      output.merge! CQS.analyze(question, responses)
 
-      return question.merge(analysis).with_indifferent_access
+      return output
     end
 
     question[:responses] = 0

app/serializers/quizzes/quiz_statistics_serializer.rb

@@ -1,6 +1,6 @@
 module Quizzes
   class QuizStatisticsSerializer < Canvas::APISerializer
-    root :quiz_statistics
+    SubmissionStatisticsExtractor = /^submission_(.+)/
 
     # Utilizes both Student and Item analysis to generate a compound document of
     # quiz statistics.
@@ -10,6 +10,8 @@ module Quizzes
       include ActiveModel::SerializerSupport
     end
 
+    root :quiz_statistics
+
     attributes *[
       # the id is really only included in JSON-API and only because the spec
       # requires it, this is because the output of this serializer is a mix of
@@ -81,32 +83,26 @@ module Quizzes
       # we're going to merge the item analysis for applicable questions into the
       # generic question statistics from the student analysis
       question_statistics.each do |question|
-        question_id = question[:id]
+        question_id = question[:id] = "#{question[:id]}"
         question_item = item_analysis_report.detect do |question_item|
-          question_item[:question_id] == question_id
+          "#{question_item[:question_id]}" == question_id
         end
 
         if question_item.present?
-          question_item.delete :question_id
-          question.merge! question_item
+          question.merge! question_item.except(:question_id)
         end
-
-        remove_unwanted_fields question
       end
 
       question_statistics
     end
 
     def submission_statistics
-      report = student_analysis_report
-      extractor = /^submission_(.+)/
-
       {}.tap do |out|
-        report.each_pair do |key, statistic|
-          out[$1] = statistic if key =~ extractor
+        student_analysis_report.each_pair do |key, statistic|
+          out[$1] = statistic if key =~ SubmissionStatisticsExtractor
         end
 
-        out[:unique_count] = report[:unique_submission_count]
+        out[:unique_count] = student_analysis_report[:unique_submission_count]
       end
     end
 
@@ -131,17 +127,5 @@ module Quizzes
     def item_analysis_report
       @item_analysis_report ||= object[:item_analysis].report.generate(false)
     end
-
-    def remove_unwanted_fields(question)
-      # this is always empty
-      question.delete(:unexpected_response_values)
-
-      (question[:answers] || []).each do |answer|
-        # this is 99% of the time empty, see CNVS-12170
-        answer.delete(:html)
-        # this is unnecessary
-        answer.delete(:comments)
-      end
-    end
   end
 end

spec/models/quizzes/quiz_statistics/student_analysis_spec.rb

@@ -332,7 +332,7 @@ describe Quizzes::QuizStatistics::StudentAnalysis do
       output.should == {
         question_type: 'essay_question',
         some_metric: 5
-      }.with_indifferent_access
+      }
     end
 
     it "shouldn't proxy if the legacy flag is on" do

spec/serializers/quizzes/quiz_statistics_serializer_spec.rb

@@ -100,4 +100,28 @@ describe Quizzes::QuizStatisticsSerializer do
     @json['links'].should be_present
     @json['links']['quiz'].should == 'http://example.com/api/v1/courses/1/quizzes/1'
   end
+
+  it 'stringifies question_statistics ids' do
+    subject.stubs(student_analysis_report: {
+      questions: [ ['question', { id: 5 }] ]
+    })
+
+    json = subject.as_json[:quiz_statistics]
+    json[:question_statistics].should be_present
+    json[:question_statistics][0][:id].should == "5"
+  end
+
+  it 'munges item_analysis with question_statistics' do
+    subject.stubs(student_analysis_report: {
+      questions: [ ['question', { id: 5 }] ]
+    })
+
+    subject.stubs(item_analysis_report: [
+      { question_id: 5, foo: 'bar' }
+    ])
+
+    json = subject.as_json[:quiz_statistics]
+    json[:question_statistics].should be_present
+    json[:question_statistics][0][:foo].should == "bar"
+  end
 end