application/json+canvas-string-ids Accept header

lukfugl · lukfugl · commit 3f0968bb526e · 2013-10-02T18:05:31.000Z
fixes CNVS-7597 when given this header, data structures passed to render :json are pre-processed before serialization to json such that any integer values in 'id', 'foo_id', or 'foo_ids' fields (also 'ids' fields, but we don't have any of those currently that I'm aware off), at any level (e.g. within a nested data structure), are cast to strings. test-plan: - full regression testing of both the UI (as it makes API calls) and the documented API - no UI behavior should change - API behavior should change only as described above Change-Id: I4e0a68957038be063cf488dd9ec2262452dea3cf Reviewed-on: https://gerrit.instructure.com/23956 Reviewed-by: Jacob Fugal <jacob@instructure.com> Product-Review: Jacob Fugal <jacob@instructure.com> QA-Review: Jacob Fugal <jacob@instructure.com> Tested-by: Jenkins <jenkins@instructure.com>
diff --git a/app/coffeescripts/gradebook2/Gradebook.coffee b/app/coffeescripts/gradebook2/Gradebook.coffee
@@ -57,6 +57,7 @@ define [
       @rows = []
       @assignmentsToHide = userSettings.contextGet('hidden_columns') || []
       @sectionToShow = userSettings.contextGet 'grading_show_only_section'
+      @sectionToShow = @sectionToShow && String(@sectionToShow)
       @show_attendance = userSettings.contextGet 'show_attendance'
       @include_ungraded_assignments = userSettings.contextGet 'include_ungraded_assignments'
       @userFilterRemovedRows = []
@@ -586,7 +587,7 @@ define [
         )()
         $('#section_to_show').after($sectionToShowMenu).show().kyleMenu()
         $sectionToShowMenu.bind 'menuselect', (event, ui) =>
-          @sectionToShow = Number($sectionToShowMenu.find('[aria-checked="true"] input[name="section_to_show_radio"]').val()) || undefined
+          @sectionToShow = $sectionToShowMenu.find('[aria-checked="true"] input[name="section_to_show_radio"]').val()
           userSettings[ if @sectionToShow then 'contextSet' else 'contextRemove']('grading_show_only_section', @sectionToShow)
           updateSectionBeingShownText()
           @buildRows()
diff --git a/app/coffeescripts/views/DraggableCollectionView.coffee b/app/coffeescripts/views/DraggableCollectionView.coffee
@@ -73,7 +73,8 @@ define [
     # Internal: get an item's ID from the ui element
     # Assumes that the first child DOM element will have the id in the data-item-id attribute
     _getItemId: (item) ->
-      parseInt item.children(":first").data('item-id')
+      id = item.children(":first").data('item-id')
+      id && String(id)
 
     # Internal: When an item is moved from one group to another
     # Assumes that the first child DOM element will have the id of the model in the data-item-id attribute
diff --git a/app/coffeescripts/views/collaborations/CollaboratorPickerView.coffee b/app/coffeescripts/views/collaborations/CollaboratorPickerView.coffee
@@ -63,8 +63,9 @@ define [
     #
     # Returns nothing.
     createLists: ->
+      currentUser = ENV.current_user_id && String(ENV.current_user_id)
       @userList   = new ListView
-        currentUser: ENV.current_user_id
+        currentUser: currentUser
         el: @$userList
         fetchOptions: @fetchOptions
         type: 'user'
@@ -73,7 +74,7 @@ define [
         fetchOptions: @fetchOptions
         type: 'group'
       @memberList = new MemberListView
-        currentUser: ENV.current_user_id
+        currentUser: currentUser
         el: @$memberList
 
     # Internal: Trigger initial fetch actions on each collection.
diff --git a/app/coffeescripts/views/content_migrations/subviews/CourseFindSelectView.coffee b/app/coffeescripts/views/content_migrations/subviews/CourseFindSelectView.coffee
@@ -70,7 +70,7 @@ define [
 
     # Build a list of courses that our template and autocomplete can use
     # objects look like
-    #   {label: 'Plant Science', value: 'Plant Science', id: 42}
+    #   {label: 'Plant Science', value: 'Plant Science', id: '42'}
     # @api private
 
     autocompleteCourses: ->
@@ -93,7 +93,7 @@ define [
     # @api private
 
     updateSearch: (event) => 
-      value = parseInt(event.target.value, 10)
+      value = event.target.value && String(event.target.value)
       @setSourceCourseId value
 
       courses = @autocompleteCourses()
diff --git a/app/coffeescripts/views/conversations/AutocompleteView.coffee b/app/coffeescripts/views/conversations/AutocompleteView.coffee
@@ -181,6 +181,7 @@ define [
     #
     # Returns a model object.
     _getModel: (id) ->
+      id = id && String(id)
       result = @resultCollection.find((model) -> model.id == id)
 
     # Internal: Remove the "selected" class from result list items.
diff --git a/app/coffeescripts/views/courses/roster/EditSectionsView.coffee b/app/coffeescripts/views/courses/roster/EditSectionsView.coffee
@@ -71,7 +71,7 @@ define [
 
       enrollment = @model.findEnrollmentByRole(@model.currentRole)
       currentIds = _.map @model.sectionEditableEnrollments(), (en) -> en.course_section_id
-      sectionIds = _.map $('#user_sections').find('input'), (i) -> parseInt($(i).val().split('_')[1])
+      sectionIds = _.map $('#user_sections').find('input'), (i) -> $(i).val().split('_')[1]
       newSections = _.reject sectionIds, (i) => _.include currentIds, i
       newEnrollments = []
       deferreds = []
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
@@ -1406,11 +1406,15 @@ def set_layout_options
     (@body_classes ||= []) << 'embedded' if @embedded_view
   end
 
+  def stringify_json_ids?
+    request.headers['Accept'] =~ %r{application/json\+canvas-string-ids}
+  end
+
   def render(options = nil, extra_options = {}, &block)
     set_layout_options
     if options && options.key?(:json)
       json = options.delete(:json)
-      json = ActiveSupport::JSON.encode(json) unless json.is_a?(String)
+      json = ActiveSupport::JSON.encode(json, stringify_json_ids: stringify_json_ids?) unless json.is_a?(String)
       # prepend our CSRF protection to the JSON response, unless this is an API
       # call that didn't use session auth, or a non-GET request.
       if prepend_json_csrf?
diff --git a/app/controllers/context_controller.rb b/app/controllers/context_controller.rb
@@ -312,7 +312,7 @@ def roster
       load_all_contexts(:context => @context)
       js_env({
         :ALL_ROLES => all_roles,
-        :SECTIONS => sections.map { |s| { :id => s.id, :name => s.name } },
+        :SECTIONS => sections.map { |s| { :id => s.id.to_s, :name => s.name } },
         :USER_LISTS_URL => polymorphic_path([@context, :user_lists], :format => :json),
         :ENROLL_USERS_URL => course_enroll_users_url(@context),
         :SEARCH_URL => search_recipients_url,
diff --git a/app/controllers/conversations_controller.rb b/app/controllers/conversations_controller.rb
@@ -153,8 +153,11 @@ def index
         load_all_contexts :permissions => [:manage_user_notes]
         notes_enabled = @current_user.associated_accounts.any?{|a| a.enable_user_notes }
         can_add_notes_for_account = notes_enabled && @current_user.associated_accounts.any?{|a| a.grants_right?(@current_user, nil, :manage_students) }
+
+        current_user_json = conversation_user_json(@current_user, @current_user, session, :include_participant_avatars => true)
+        current_user_json[:id] = current_user_json[:id].to_s
         hash = {:CONVERSATIONS => {
-            :USER => conversation_users_json([@current_user], @current_user, session, :include_participant_contexts => false).first,
+            :USER => current_user_json,
             :CONTEXTS => @contexts,
             :NOTES_ENABLED => notes_enabled,
             :CAN_ADD_NOTES_FOR_ACCOUNT => can_add_notes_for_account,
diff --git a/app/views/gradebooks/speed_grader.html.erb b/app/views/gradebooks/speed_grader.html.erb
@@ -3,7 +3,7 @@
   jammit_css :speed_grader
   js_env({
     :RUBRIC_ASSESSMENT => {
-      :assessor_id => @current_user ? @current_user.id : "null",
+      :assessor_id => @current_user.id.to_s,
       :assessment_type => can_do(@assignment, @current_user, :grade) ? "grading" : "peer_review"
     }
   })
diff --git a/config/initializers/json.rb b/config/initializers/json.rb
@@ -42,6 +42,7 @@ def self.encode(value, options = nil)
     depth = (options[:recursion_depth] ||= 1)
     raise CircularReferenceError, 'something references itself (probably)' if depth > 1000
     options[:recursion_depth] = depth + 1
+    Api.stringify_json_ids(value) if options[:stringify_json_ids]
     value.to_json(options)
   ensure
     options[:recursion_depth] = depth
diff --git a/lib/api.rb b/lib/api.rb
@@ -545,4 +545,20 @@ def api_endpoint_info(protocol, host, url)
     {}
   end
 
+  def self.stringify_json_ids(value)
+    return unless value.is_a?(Hash)
+    value.keys.each do |key|
+      if key =~ /(^|_)id$/
+        # id, foo_id, etc.
+        value[key] = stringify_json_id(value[key])
+      elsif key =~ /(^|_)ids$/ && value[key].is_a?(Array)
+        # ids, foo_ids, etc.
+        value[key].map!{ |id| stringify_json_id(id) }
+      end
+    end
+  end
+
+  def self.stringify_json_id(id)
+    id.is_a?(Integer) ? id.to_s : id
+  end
 end
diff --git a/public/javascripts/gradebooks.js b/public/javascripts/gradebooks.js
@@ -71,6 +71,7 @@ define([
       $courseSections  = $(".outer_student_name .course_section"),
       contextId = $("#current_context_code").text().split("_")[1],
       sectionToShow = userSettings.contextGet('grading_show_only_section');
+  sectionToShow = sectionToShow && String(sectionToShow);
 
   $courseSections.each(function(){
     possibleSections[$(this).data('course_section_id')] = $(this).attr('title'); 
diff --git a/public/javascripts/instructure.js b/public/javascripts/instructure.js
@@ -61,6 +61,14 @@ define([
     if (ENV.AUTHENTICITY_TOKEN) xhr.setRequestHeader('X-CSRF-Token', ENV.AUTHENTICITY_TOKEN);
   }
 
+  // indicate we want stringified IDs for JSON responses
+  $.ajaxPrefilter("json", function( options, originalOptions, jqXHR ) {
+    if (options.accepts.json)
+      options.accepts.json = options.accepts.json + ', application/json+canvas-string-ids';
+    else
+      options.accepts.json = 'application/json+canvas-string-ids';
+  });
+
   $.ajaxPrefilter(function( options, originalOptions, jqXHR ) {
     if ( !options.crossDomain ) CSRFProtection(jqXHR);
 
diff --git a/public/javascripts/speed_grader.js b/public/javascripts/speed_grader.js
@@ -162,6 +162,10 @@ define([
       this.submission = $.grep(jsonData.submissions, function(submission, i){
         return submission.user_id === student.id;
       })[0];
+      $.each(visibleRubricAssessments, function(i, rubricAssessment) {
+        rubricAssessment.user_id = rubricAssessment.user_id && String(rubricAssessment.user_id);
+        rubricAssessment.assessor_id = rubricAssessment.assessor_id && String(rubricAssessment.assessor_id);
+      });
       this.rubric_assessments = $.grep(visibleRubricAssessments, function(rubricAssessment, i){
         return rubricAssessment.user_id === student.id;
       });
@@ -172,6 +176,7 @@ define([
     // the sectionToShow will be remembered for a given user in a given browser across all assignments in this course
     if (!jsonData.GROUP_GRADING_MODE) {
       sectionToShow = userSettings.contextGet('grading_show_only_section');
+      sectionToShow = sectionToShow && String(sectionToShow);
     }
     if (sectionToShow) {
       var tempArray  = $.grep(jsonData.studentsWithSubmissions, function(student, i){
@@ -189,28 +194,39 @@ define([
     //by defaut the list is sorted alphbetically by student last name so we dont have to do any more work here, 
     // if the cookie to sort it by submitted_at is set we need to sort by submitted_at.
     var hideStudentNames = utils.shouldHideStudentNames();
+    var compareBy = function(f) {
+      return function(a, b) {
+        a = f(a);
+        b = f(b);
+        if ((!a && !b) || a === b) { return 0; }
+        if (!a || a > b) { return +1; }
+        else { return -1; }
+      };
+    };
     if(hideStudentNames) {
-      jsonData.studentsWithSubmissions.sort(function(a,b){
-        return ((a && a.submission && a.submission.id) || Number.MAX_VALUE) - 
-               ((b && b.submission && b.submission.id) || Number.MAX_VALUE);
-      });          
+      jsonData.studentsWithSubmissions.sort(compareBy(function(student) {
+        return student &&
+          student.submission &&
+          student.submission.id;
+      }));
     } else if (userSettings.get("eg_sort_by") == "submitted_at") {
-      jsonData.studentsWithSubmissions.sort(function(a,b){
-        return ((a && a.submission && a.submission.submitted_at && $.parseFromISO(a.submission.submitted_at).timestamp) || Number.MAX_VALUE) - 
-               ((b && b.submission && b.submission.submitted_at && $.parseFromISO(b.submission.submitted_at).timestamp) || Number.MAX_VALUE);
-      });
+      jsonData.studentsWithSubmissions.sort(compare_by(function(student){
+        return student &&
+          student.submission &&
+          student.submission.submitted_at &&
+          $.parseFromISO(student.submission.submitted_at).timestamp;
+      }));
     } else if (userSettings.get("eg_sort_by") == "submission_status") {
-      jsonData.studentsWithSubmissions.sort(function(a,b) {
-        var states = {
-          "not_graded": 1,
-          "resubmitted": 2,
-          "not_submitted": 3,
-          "graded": 4
-        };
-        var stateA = submissionStateName(a.submission);
-        var stateB = submissionStateName(b.submission);
-        return states[stateA] - states[stateB];
-      });
+      var states = {
+        "not_graded": 1,
+        "resubmitted": 2,
+        "not_submitted": 3,
+        "graded": 4
+      };
+      jsonData.studentsWithSubmissions.sort(compareBy(function(student){
+        return student &&
+          states[submissionStateName(student.submission)];
+      }));
     }
   }
 
@@ -962,7 +978,7 @@ define([
                       hash.student_id;
 
       // choose the first ungraded student if the requested one doesn't exist
-      if (typeof(studentId) != "number" || !jsonData.studentMap[studentId]) {
+      if (!jsonData.studentMap[studentId]) {
         studentId = jsonData.studentsWithSubmissions[0].id;
         for (var i = 0, max = jsonData.studentsWithSubmissions.length; i < max; i++){
           if (typeof jsonData.studentsWithSubmissions[i].submission !== 'undefined' && jsonData.studentsWithSubmissions[i].submission.workflow_state !== 'graded'){
@@ -1002,7 +1018,7 @@ define([
     },
 
     handleStudentChanged: function(){
-      var id = parseInt( $selectmenu.val(), 10 );
+      var id = $selectmenu.val();
       this.currentStudent = jsonData.studentMap[id];
       document.location.hash = "#" + encodeURIComponent(JSON.stringify({
         "student_id": this.currentStudent.id
diff --git a/spec/apis/general_api_spec.rb b/spec/apis/general_api_spec.rb
@@ -110,4 +110,72 @@ def @course.filter_attributes_for_user(hash, user, session)
       @submission.submission_comments.first.comment.should == "yay"
     end
   end
+
+  describe "application/json+canvas-string-ids" do
+    it "should stringify 'id' fields" do
+      account_admin_user(active_all: true)
+      json = api_call(:get, "/api/v1/accounts/#{Account.default.id}",
+        { controller: 'accounts', action: 'show', id: Account.default.to_param, format: 'json' },
+        {}, { 'Accept' => 'application/json+canvas-string-ids' })
+      json['id'].should == Account.default.id.to_s
+    end
+
+    it "should not stringify 'id' fields without Accept header" do
+      account_admin_user(active_all: true)
+      json = api_call(:get, "/api/v1/accounts/#{Account.default.id}",
+        { controller: 'accounts', action: 'show', id: Account.default.to_param, format: 'json' })
+      json['id'].should == Account.default.id
+    end
+
+    it "should stringify 'something_id' fields" do
+      account = Account.default.sub_accounts.create!
+      account_admin_user(active_all: true, account: account)
+      json = api_call(:get, "/api/v1/accounts/#{account.id}",
+        { controller: 'accounts', action: 'show', id: account.to_param, format: 'json' },
+        {}, { 'Accept' => 'application/json+canvas-string-ids' })
+      json['root_account_id'].should == Account.default.id.to_s
+    end
+
+    it "should not stringify 'something_id' fields without Accept header" do
+      account = Account.default.sub_accounts.create!
+      account_admin_user(active_all: true, account: account)
+      json = api_call(:get, "/api/v1/accounts/#{account.id}",
+        { controller: 'accounts', action: 'show', id: account.to_param, format: 'json' })
+      json['root_account_id'].should == Account.default.id
+    end
+
+    it "should pass through non-integer 'something_id' fields" do
+      account_admin_user(active_all: true)
+      json = api_call(:get, "/api/v1/accounts/#{Account.default.id}",
+        { controller: 'accounts', action: 'show', id: Account.default.to_param, format: 'json' },
+        {}, { 'Accept' => 'application/json+canvas-string-ids' })
+      json['root_account_id'].should be_nil
+    end
+
+    it "should stringify nested ids" do
+      user_with_pseudonym(active_user: true)
+      course_with_teacher(user: @user, active_all: true)
+      student_in_course(course: @course, active_all: true)
+      @user = @teacher
+
+      json = api_call(:post, "/api/v1/conversations",
+        { controller: 'conversations', action: 'create', format: 'json' },
+        { recipients: [@student.id], body: "test" },
+        { 'Accept' => 'application/json+canvas-string-ids' })
+      json.first["participants"].first["id"].should == @teacher.id.to_s
+    end
+
+    it "should stringify all ids in a 'something_ids' field" do
+      user_with_pseudonym(active_user: true)
+      course_with_teacher(user: @user, active_all: true)
+      student_in_course(course: @course, active_all: true)
+      @user = @teacher
+
+      json = api_call(:post, "/api/v1/conversations",
+        { controller: 'conversations', action: 'create', format: 'json' },
+        { recipients: [@student.id], body: "test" },
+        { 'Accept' => 'application/json+canvas-string-ids' })
+      json.first["messages"].first["participating_user_ids"].sort.should == [@teacher.id, @student.id].map{ |id| id.to_s }.sort
+    end
+  end
 end
diff --git a/spec/selenium/content_migrations_spec.rb b/spec/selenium/content_migrations_spec.rb
@@ -340,7 +340,7 @@ def test_selective_content(source_course=nil)
       submit
 
       cm = @course.content_migrations.last
-      cm.migration_settings["source_course_id"].should == @copy_from.id
+      cm.migration_settings["source_course_id"].should == @copy_from.id.to_s
 
       source_link = f('.migrationProgressItem .sourceLink a')
       source_link.text.should == @copy_from.name

Original file line number	Diff line number	Diff line change
`@@ -181,6 +181,7 @@ define [`
`181`	`181`	`#`
`182`	`182`	`# Returns a model object.`
`183`	`183`	`_getModel: (id) ->`
	`184`	`+ id = id && String(id)`
`184`	`185`	`result = @resultCollection.find((model) -> model.id == id)`
`185`	`186`
`186`	`187`	`# Internal: Remove the "selected" class from result list items.`
Original file line number	Diff line number	Diff line change
`@@ -3,7 +3,7 @@`
`3`	`3`	`jammit_css :speed_grader`
`4`	`4`	`js_env({`
`5`	`5`	`:RUBRIC_ASSESSMENT => {`
`6`		`- :assessor_id => @current_user ? @current_user.id : "null",`
	`6`	`+ :assessor_id => @current_user.id.to_s,`
`7`	`7`	`:assessment_type => can_do(@assignment, @current_user, :grade) ? "grading" : "peer_review"`
`8`	`8`	`}`
`9`	`9`	`})`