Skip to content

Commit 07fbf66

Browse files
simonistasimonista
committed
don't allow editing rubric rows linked to outcomes
fixes CNVS-1789 test plan: - create an outcome - create a rubric with two rows, one normal and one linked to the outcome - save, and then edit again - you should be able to edit the normal one but not the linked one Change-Id: Id8a99dab45bf1cc79cc51957de9ce527bd819265 Reviewed-on: https://gerrit.instructure.com/23073 Tested-by: Jenkins <jenkins@instructure.com> Reviewed-by: Cameron Matheson <cameron@instructure.com> QA-Review: Amber Taniuchi <amber@instructure.com> Product-Review: Simon Williams <simon@instructure.com>
1 parent 89ae60b commit 07fbf66

6 files changed

Lines changed: 61 additions & 23 deletions

File tree

‎app/models/rubric.rb‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,7 @@ def will_change_with_update?(params)
200200
false
201201
end
202202

203+
CriteriaData = Struct.new(:criteria, :points_possible, :title)
203204
def generate_criteria(params)
204205
@used_ids = {}
205206
title = params[:title] || t('context_name_rubric', "%{course_name} Rubric", :course_name => context.name)
@@ -239,8 +240,8 @@ def generate_criteria(params)
239240
points_possible += criterion[:points] unless criterion[:ignore_for_scoring]
240241
criteria[idx.to_i] = criterion
241242
end
242-
criteria = criteria.select{|c| c}
243-
OpenObject.new(:criteria => criteria, :points_possible => points_possible, :title => title)
243+
criteria = criteria.compact
244+
CriteriaData.new(criteria, points_possible, title)
244245
end
245246

246247
def update_assessments_for_new_criteria(new_criteria)

‎app/views/shared/_rubric.html.erb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@
5959
<% end %>
6060
</div>
6161
<div class="displaying">
62-
<span class="title"><%= rubric.title rescue "Title" %></span>
62+
<span class="title"><%= rubric.try(:title) || t(:rubric_title, "Title") %></span>
6363
</div>
6464
</th>
6565
</tr>

‎app/views/shared/_rubric_criterion.html.erb‎

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,28 @@
1-
<% criterion = rubric_criterion || nil; assessing ||= false; rubric ||= nil; rubric_association ||= nil; edit_view ||= false; assessment_rating ||= nil %>
2-
<tr id="criterion_<%= criterion ? criterion.id : "blank" %>" class="criterion <%= "blank" unless criterion %> <%= 'ignore_criterion_for_scoring' if criterion && criterion.ignore_for_scoring %> <%= 'learning_outcome_criterion' if criterion && criterion.learning_outcome_id %>" style="<%= hidden unless criterion %>">
1+
<%
2+
criterion = rubric_criterion || nil
3+
assessing ||= false
4+
rubric ||= nil
5+
rubric_association ||= nil
6+
edit_view ||= false
7+
assessment_rating ||= nil
8+
9+
learning_outcome_criterion = criterion.try(:learning_outcome_id)
10+
description = criterion.try(:description) || t('defaults.description', "Description of criterion")
11+
%>
12+
13+
<tr id="criterion_<%= criterion ? criterion.id : "blank" %>" class="criterion <%= "blank" unless criterion %> <%= 'ignore_criterion_for_scoring' if criterion && criterion.ignore_for_scoring %> <%= 'learning_outcome_criterion' if learning_outcome_criterion %>" style="<%= hidden unless criterion %>">
314
<td class="criterion_description hover-container">
415
<div class="container">
516
<div class="links editing">
617
<a href="#" class="edit_criterion_link hide-till-hover"><i class="icon-edit standalone-icon"></i></a>
718
</div>
819
<%= image_tag "flagged_question.png", :class => "learning_outcome_flag", :title => t('titles.linked_to_learning_outcome', "This criterion is linked to a Learning Outcome") %>
9-
<span class="description criterion_description_value"><%= criterion.description rescue t('defaults.description', "Description of criterion") %></span>
10-
<span class="learning_outcome_id" style="display: none;"><%= criterion.learning_outcome_id rescue nbsp %></span>
11-
<span class="criterion_id" style="display: none;"><%= criterion && criterion.id %></span>
12-
<div class="long_description_holder editing <%= 'empty' if !criterion || !criterion.long_description || criterion.long_description.empty? %>">
20+
<span class="description criterion_description_value"><%= description %></span>
21+
<span class="learning_outcome_id" style="display: none;"><%= criterion.try(:learning_outcome_id) %></span>
22+
<span class="criterion_id" style="display: none;"><%= criterion.try(:id) %></span>
23+
<div class="long_description_holder editing <%= 'empty' if !criterion || criterion.long_description.blank? %>">
1324
<a href="#" class="long_description_link"><%= t 'links.view_longer_description', "view longer description" %></a>
14-
<textarea class="long_description" style="display: none;"><%= h((criterion.long_description rescue '')) %></textarea>
25+
<textarea class="long_description" style="display: none;"><%= h((criterion.try(:long_description) || '')) %></textarea>
1526
</div>
1627
<div class="threshold">
1728
<%= before_label :threshold, "threshold" %>

‎public/javascripts/edit_rubric.js‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,7 @@ define([
160160
},
161161
editRating: function($rating) {
162162
if(!$rating.parents(".rubric").hasClass('editing')) { return; }
163+
if($rating.parents(".criterion").hasClass('learning_outcome_criterion')) { return; }
163164
rubricEditing.hideEditRating(true);
164165
rubricEditing.hideCriterionAdd($rating.parents(".rubric"));
165166
var height = Math.max(40, $rating.find(".rating").height());
@@ -184,6 +185,7 @@ define([
184185
},
185186
editCriterion: function($criterion) {
186187
if(!$criterion.parents(".rubric").hasClass('editing')) { return; }
188+
if($criterion.hasClass('learning_outcome_criterion')) { return; }
187189
rubricEditing.hideEditCriterion(true);
188190
var $td = $criterion.find(".criterion_description");
189191
var height = Math.max(40, $td.find(".description").height());
@@ -735,10 +737,12 @@ define([
735737
rubricEditing.editCriterion($criterion);
736738
return false;
737739
}).delegate('.criterion_description_value', 'click', function(event) {
738-
rubricEditing.editCriterion($(this).parents(".criterion"));
740+
var $criterion = $(this).parents(".criterion")
741+
rubricEditing.editCriterion($criterion);
739742
return false;
740743
}).delegate('.edit_criterion_link', 'click', function(event) {
741-
rubricEditing.editCriterion($(this).parents(".criterion"));
744+
var $criterion = $(this).parents(".criterion")
745+
rubricEditing.editCriterion($criterion);
742746
return false;
743747
}).delegate('.delete_criterion_link', 'click', function(event) {
744748
var $criterion = $(this).parents(".criterion");

‎spec/selenium/helpers/rubrics_common.rb‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ def should_delete_a_rubric
3838
ff('#rubrics .rubric').each { |rubric| rubric.should_not be_displayed }
3939
end
4040

41-
def should_edit_a_rubric
41+
def should_edit_a_rubric
4242
edit_title = 'edited rubric'
4343
create_rubric_with_criterion_points "5"
4444
rubric = Rubric.last
@@ -80,3 +80,17 @@ def should_pick_the_lower_value_when_splitting_without_room_for_an_integer
8080
ffj(".rubric .criterion:visible .rating .points").count.should == 3
8181
ffj(".rubric .criterion:visible .rating .points")[1].text.should == '0'
8282
end
83+
84+
def import_outcome
85+
f('.edit_rubric_link').click
86+
wait_for_ajaximations
87+
f('.rubric.editing tr.criterion .delete_criterion_link').click
88+
wait_for_ajaximations
89+
f('.rubric.editing .find_outcome_link').click
90+
wait_for_ajaximations
91+
f('.outcome-link').click
92+
wait_for_ajaximations
93+
f('.ui-dialog .btn-primary').click
94+
accept_alert
95+
wait_for_ajaximations
96+
end

‎spec/selenium/teacher_rubrics_spec.rb‎

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -83,17 +83,9 @@
8383
outcome_model(:context => @course)
8484

8585
get "/courses/#{@course.id}/rubrics/#{@rubric.id}"
86-
f('.edit_rubric_link').click
87-
wait_for_ajaximations
88-
f('.rubric.editing tr.criterion .delete_criterion_link').click
89-
wait_for_ajaximations
90-
f('.rubric.editing .find_outcome_link').click
91-
wait_for_ajaximations
92-
f('.outcome-link').click
93-
wait_for_ajaximations
94-
f('.ui-dialog .btn-primary').click
95-
accept_alert
9686
wait_for_ajaximations
87+
import_outcome
88+
9789
f('tr.learning_outcome_criterion .criterion_description .description').text.should == @outcome.title
9890
ff('tr.learning_outcome_criterion td.rating .description').map(&:text).should == @outcome.data[:rubric_criterion][:ratings].map { |c| c[:description] }
9991
ff('tr.learning_outcome_criterion td.rating .points').map(&:text).should == @outcome.data[:rubric_criterion][:ratings].map { |c| c[:points].to_s }
@@ -104,6 +96,22 @@
10496
rubric.data.first[:ratings].map { |r| r[:points] }.should == @outcome.data[:rubric_criterion][:ratings].map { |c| c[:points] }
10597
end
10698

99+
it "should not allow editing a criterion row linked to an outcome" do
100+
rubric_association_model(:user => @user, :context => @course, :purpose => "grading")
101+
outcome_model(:context => @course)
102+
rubric = Rubric.last
103+
104+
get "/courses/#{@course.id}/rubrics/#{@rubric.id}"
105+
wait_for_ajaximations
106+
import_outcome
107+
108+
f('.edit_rubric_link').click
109+
wait_for_ajaximations
110+
111+
links = ffj("#rubric_#{rubric.id}.editing .ratings:first .edit_rating_link")
112+
links.any?(&:displayed?).should be_false
113+
end
114+
107115
it "should not show 'use for grading' as an option" do
108116
course_with_teacher_logged_in
109117
get "/courses/#{@course.id}/rubrics"

0 commit comments

Comments
 (0)