ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Handle rubric criteria/ratings with large IDs 

When handling rubric creation requests via the API, do not use the ID of
incoming rubric criteria (or ratings on an individual criterion) as an
index to store the item in an array. Doing so caused IDs like 297203574
to create an array with 297 million elements to store rubric criteria
and then operate on that array before throwing away the empty values
(and doing the same thing when storing ratings for specific criteria),
which slowed the process to a crawl.

fixes EVAL-1029
flag=none

Test plan:
- Make a POST request to the rubric creation API endpoint for an
  existing course, using excessively large values for the IDs (for a
  sample request see APPENDIX below):
  - /api/v1/courses/<courseID>/rubrics
- This should complete quickly (within a couple seconds if Rails doesn't
  have to reload) and certainly should not take over a minute
- Adding additional criteria should not cause a noticeable increase in
  the time taken
- Test that updating an existing rubric (using a PUT request) does not
  take forever
- Test that creating/updating a rubric via Canvas itself still works as
  before

APPENDIX: sample request that took over a minute without this patchset
but should finish very quickly with it

{
  "rubric": {
    "title": "Canvas Test Rubric - edit",
    "free_form_criterion_comments": false,
    "criteria": {
      "297203574": {
        "id": "297203574",
        "points": 5.0,
        "description": "First item",
        "long_description": "",
        "criterion_use_range": false,
        "ratings": {
          "229567777": { "id": "229567777", "points": 5.0, "description": "Very High", "long_description": "no" },
          "229567778": { "id": "229567778", "points": 4.0, "description": "High", "long_description": "no" },
          "229567779": { "id": "229567779", "points": 3.0, "description": "Achievement Standard", "long_description": "no" },
          "229567780": { "id": "229567780", "points": 2.0, "description": "Developing", "long_description": "no" },
          "229567781": { "id": "229567781", "points": 1.0, "description": "Emerging", "long_description": "no" }
        }
      },
      "297203575": {
        "id": "297203575",
        "points": 5.0,
        "description": "Second item",
        "long_description": "",
        "criterion_use_range": false,
        "ratings": {
          "229568289": { "id": "229568289", "points": 5.0, "description": "Very High", "long_description": "no" },
          "229568290": { "id": "229568290", "points": 4.0, "description": "High", "long_description": "no" },
          "229568291": { "id": "229568291", "points": 3.0, "description": "Achievement Standard", "long_description": "no" },
          "229568292": { "id": "229568292", "points": 2.0, "description": "Developing", "long_description": "no" },
          "229568293": { "id": "229568293", "points": 1.0, "description": "Emerging", "long_description": "no" }
        }
      },
      "297203576": {
        "id": "297203576",
        "points": 5.0,
        "description": "third item",
        "long_description": "",
        "criterion_use_range": false,
        "ratings": {
          "229568801": { "id": "229568801", "points": 5.0, "description": "Very High", "long_description": "no" },
          "229568802": { "id": "229568802", "points": 4.0, "description": "High", "long_description": "no" },
          "229568803": { "id": "229568803", "points": 3.0, "description": "Achievement Standard", "long_description": "no" },
          "229568804": { "id": "229568804", "points": 2.0, "description": "Developing", "long_description": "no" },
          "229568805": { "id": "229568805", "points": 1.0, "description": "Emerging", "long_description": "no" }
        }
      },
      "297203577": {
        "id": "297203577",
        "points": 5.0,
        "description": "fourth item",
        "long_description": "",
        "criterion_use_range": false,
        "ratings": {
          "229569313": { "id": "229569313", "points": 5.0, "description": "Very High", "long_description": "no" },
          "229569314": { "id": "229569314", "points": 4.0, "description": "High", "long_description": "no" },
          "229569315": { "id": "229569315", "points": 3.0, "description": "Achievement Standard", "long_description": "no" },
          "229569316": { "id": "229569316", "points": 2.0, "description": "Developing", "long_description": "no" },
          "229569317": { "id": "229569317", "points": 1.0, "description": "Emerging", "long_description": "no" }
        }
      },
      "297203578": {
        "id": "297203578",
        "points": 5.0,
        "description": "fifth item",
        "long_description": "",
        "criterion_use_range": false,
        "ratings": {
          "229569825": { "id": "229569825", "points": 5.0, "description": "Very High", "long_description": "no" },
          "229569826": { "id": "229569826", "points": 4.0, "description": "High", "long_description": "no" },
          "229569827": { "id": "229569827", "points": 3.0, "description": "Achievement Standard", "long_description": "no" },
          "229569828": { "id": "229569828", "points": 2.0, "description": "Developing", "long_description": "no" },
          "229569829": { "id": "229569829", "points": 1.0, "description": "Emerging", "long_description": "no" }
        }
      }
    }
  }
}

Change-Id: I33a6cb77c8cda3a4ce0ea949da83281d822e0a12
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/249271
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Product-Review: Syed Hussain <shussain@instructure.com>
Reviewed-by: Gary Mei <gmei@instructure.com>
Reviewed-by: Spencer Olson <solson@instructure.com>
QA-Review: Kai Bjorkman <kbjorkman@instructure.com>
This commit is contained in:
Adrian Packel 2020-10-05 12:12:02 -05:00
parent a0e230a850
commit bb6abb0c38
2 changed files with 78 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
app/models/rubric.rb
View File

@ -346,7 +346,6 @@ class Rubric < ActiveRecord::Base
criterion_data[:id] = nil if criterion_data[:id] && criterion_data[:id].empty?
criterion[:id] = unique_item_id(criterion_data[:id])
criterion[:criterion_use_range] = [true, 'true'].include?(criterion_data[:criterion_use_range])
ratings = []
if criterion_data[:learning_outcome_id].present?
outcome = LearningOutcome.where(id: criterion_data[:learning_outcome_id]).first
criterion[:long_description] = outcome&.description || ''
@ -356,16 +355,20 @@ class Rubric < ActiveRecord::Base
criterion[:ignore_for_scoring] = criterion_data[:ignore_for_scoring] == '1'
end
end
(criterion_data[:ratings] || {}).each do |jdx, rating_data|
ratings = (criterion_data[:ratings] || {}).values.map do |rating_data|
rating_data[:id]&.strip!
rating = criterion_rating(rating_data, criterion[:id])
ratings[jdx.to_i] = rating
criterion_rating(rating_data, criterion[:id])
end
criterion[:ratings] = ratings.select{|r| r}.sort_by{|r| [-1 * (r[:points] || 0), r[:description] || CanvasSort::First]}
criterion[:ratings] = ratings.sort_by { |r| [-1 * (r[:points] || 0), r[:description] || CanvasSort::First] }
criterion[:points] = criterion[:ratings].map{|r| r[:points]}.max || 0
criteria[idx.to_i] = criterion
# Record both the criterion data and the original ID that was passed in
# (we'll use the ID when we sort the criteria below)
criteria.push([idx, criterion])
end
criteria = criteria.compact
criteria = criteria.sort_by { |criterion| criterion.first&.to_i || CanvasSort::First }.
map(&:second)
points_possible = total_points_from_criteria(criteria)&.round(POINTS_POSSIBLE_PRECISION)
CriteriaData.new(criteria, points_possible, title)
end

68
spec/models/rubric_spec.rb
View File

@ -526,6 +526,74 @@ describe Rubric do
expect(@rubric.criteria[1][:long_description]).to eq '<p>This is <b>awesome</b>.</p>'
end
end
describe "ordering of contents" do
let(:rubric) { Rubric.new }
it "sorts criteria based on the numerical values of their hash keys" do
rubric.update_criteria(
criteria: {
"206000" => {
description: "aaaaa",
ratings: { '0' => { description: "" } }
},
"106215" => {
description: "bbbbb",
ratings: { '0' => { description: "" } }
},
"6043341" => {
description: "ccccc",
ratings: { '0' => { description: "" } }
},
"fred" => {
description: "ddddd",
ratings: { '0' => { description: "" } }
}
},
title: "my rubric"
)
expect(rubric.criteria.pluck(:description)).to eq ["ddddd", "bbbbb", "aaaaa", "ccccc"]
end
it "sorts ratings within each criterion by the number of points in descending order" do
rubric.update_criteria(
criteria: {
"1" => {
description: "aaaaa",
ratings: {
"0" => { description: "ok", points: 5 },
"1" => { description: "good", points: 10 },
"2" => { description: "bad" }
}
}
},
title: "my rubric"
)
criterion = rubric.criteria.first
expect(criterion[:ratings].pluck(:description)).to eq ["good", "ok", "bad"]
end
it "sorts ratings with the same number of points by description" do
rubric.update_criteria(
criteria: {
"1" => {
description: "aaaaa",
ratings: {
"0" => { description: "ok", points: 5 },
"1" => { description: "also ok", points: 5 },
"2" => { description: "ok too", points: 5 }
}
}
},
title: "my rubric"
)
criterion = rubric.criteria.first
expect(criterion[:ratings].pluck(:description)).to eq ["also ok", "ok", "ok too"]
end
end
end
describe 'create' do