ResultsController is now tested
Updated 2 years, 2 months ago
| Gabriel Roy-Lortie | Reviewers | ||
| markus_developers | |||
| 400, 448 | |||
| None | MarkUs Source Code Repository | ||
Relates to tickets #400 and #448 I'm not implying that it's a thorough test suite, but It's a lot better than it was (empty). It also allowed me to figure out, and remove a couple of bugs. Regarding #448: The three methods were effectively dead code. They've been removed. The new fixture files will ship with my first review to get a "ship it". This one, or the FlexibleCriteriaController one.
This review request is all about writing tests. Unit tests are still top shape. I have to say, for the record, that, by modifying *many* fixture files I introduced a failure in one of the "functional" test. Before: 29 failures, 28 errors After: 30 failures, 24 errors Unfortunately, I can't figure out where it is. Which one, of the 30 failures, is the new one? I'm sorry that I can't tell. What I can tell, in revenge, is that: 1) all recently developed tests are still working; and 2) there's no test that features only one failure -- Implying that no test suite went from clean to broken... those that worked, still does, and those that were broken still are. It's surely not perfect. But I believe that letting this code through to the repository has more advantages than inconvenients. Let me state once again that I don't like fixtures. I am still looking in a replacement. I had some issues with factory_girl and am on to look for machinist. I'll keep you posted.
Diff revision 2
This is not the most recent revision of the diff. The latest diff is revision 4. See what's changed.
- trunk/app/controllers/results_controller.rb:
- trunk/app/views/results/common/_annotations.js.erb:
- trunk/app/views/results/marker/_update_mark.rjs:
- trunk/app/views/shared/_handle_error.rjs:
- trunk/test/fixtures/assignments.yml:
- trunk/test/fixtures/extra_marks.yml:
- trunk/test/fixtures/flexible_criteria.yml:
- trunk/test/fixtures/groupings.yml:
- trunk/test/fixtures/groups.yml:
- trunk/test/fixtures/marks.yml:
- trunk/test/fixtures/memberships.yml:
- trunk/test/fixtures/results.yml:
- trunk/test/fixtures/rubric_criteria.yml:
- trunk/test/fixtures/submission_files.yml:
- trunk/test/fixtures/submission_rules.yml:
- trunk/test/fixtures/submissions.yml:
- trunk/test/fixtures/users.yml:
- trunk/test/functional/results_controller_test.rb:
Expand changes
Hide Extra Whitespace| trunk/app/controllers/results_controller.rb | |||
|---|---|---|---|
| Revision 1066 | New Change | ||
| 1 |
class ResultsController < ApplicationController |
1 |
class ResultsController < ApplicationController |
| 2 |
before_filter :authorize_only_for_admin, :except => [:codeviewer, |
2 |
before_filter :authorize_only_for_admin, :except => [:codeviewer, |
| 3 |
:edit, :update_mark, :view_marks, :create, :add_extra_mark, :next_grouping, :update_overall_comment, :expand_criteria, :collapse_criteria, :remove_extra_mark, :expand_unmarked_criteria, :update_marking_state] |
3 |
:edit, :update_mark, :view_marks, :create, :add_extra_mark, :next_grouping, :update_overall_comment, :expand_criteria, :collapse_criteria, :remove_extra_mark, :expand_unmarked_criteria, :update_marking_state] |
| 4 |
before_filter :authorize_for_ta_and_admin, :only => [:edit, |
4 |
before_filter :authorize_for_ta_and_admin, :only => [:edit, |
| 5 |
:update_mark, :create, :add_extra_mark, :download, :next_grouping, :update_overall_comment, :expand_criteria, :collapse_criteria, :remove_extra_mark, :expand_unmarked_criteria, :update_marking_state] |
5 |
:update_mark, :create, :add_extra_mark, :download, :next_grouping, :update_overall_comment, :expand_criteria, :collapse_criteria, :remove_extra_mark, :expand_unmarked_criteria, :update_marking_state] |
| 6 |
before_filter :authorize_for_user, :only => [:codeviewer] |
6 |
before_filter :authorize_for_user, :only => [:codeviewer] |
| 7 |
before_filter :authorize_for_student, :only => [:view_marks] |
7 |
before_filter :authorize_for_student, :only => [:view_marks] |
| 8 | 8 | ||
| 9 |
def create |
||
| 10 |
# Create new Result for this Submission
|
||
| 11 |
@submission_id = params[:id] |
||
| 12 |
@submission = Submission.find(@submission_id) |
||
| 13 |
|
||
| 14 |
# Is there already a result for this Submission?
|
||
| 15 |
if @submission.has_result? |
||
| 16 |
# If so, our new Result needs to have a version number greater than the
|
||
| 17 |
# old result version. We're also going to set this new result to be current.
|
||
| 18 |
old_result = @submission.get_result_used |
||
| 19 |
old_result.result_version_used = false |
||
| 20 |
old_result.save |
||
| 21 |
end
|
||
| 22 |
|
||
| 23 |
new_result = Result.new |
||
| 24 |
new_result.submission = @submission |
||
| 25 |
new_result.marking_state = Result::MARKING_STATES[:partial] |
||
| 26 |
new_result.save |
||
| 27 |
redirect_to :action => 'edit', :id => new_result.id |
||
| 28 |
end
|
||
| 29 |
|
||
| 30 |
def index |
9 |
def index |
| 31 |
end
|
10 |
end
|
| 32 |
|
11 |
|
| 33 |
def edit |
12 |
def edit |
| 34 |
result_id = params[:id] |
13 |
result_id = params[:id] |
| ... | 6 lines hidden [Expand] | ||
def edit |
|||
| 41 |
@group = @grouping.group |
20 |
@group = @grouping.group |
| 42 |
@files = @submission.submission_files |
21 |
@files = @submission.submission_files |
| 43 |
@first_file = @files.first |
22 |
@first_file = @files.first |
| 44 |
@extra_marks_points = @result.extra_marks.points |
23 |
@extra_marks_points = @result.extra_marks.points |
| 45 |
@extra_marks_percentage = @result.extra_marks.percentage |
24 |
@extra_marks_percentage = @result.extra_marks.percentage |
| 46 | @marks_map = [] |
25 | @marks_map = Hash.new |
| 47 |
@rubric_criteria.each do |criterion| |
26 |
@rubric_criteria.each do |criterion| |
| 48 |
mark = Mark.find_or_create_by_result_id_and_rubric_criterion_id(@result.id, criterion.id) |
27 |
mark = Mark.find_or_create_by_result_id_and_rubric_criterion_id(@result.id, criterion.id) |
| 49 |
mark.save(false) |
28 |
mark.save(false) |
| 50 |
@marks_map[criterion.id] = mark |
29 |
@marks_map[criterion.id] = mark |
| 51 |
end
|
30 |
end
|
| ... | 79 lines hidden [Expand] | ||
def codeviewer |
|||
| 131 |
|
110 |
|
| 132 |
@file = SubmissionFile.find(@submission_file_id) |
111 |
@file = SubmissionFile.find(@submission_file_id) |
| 133 |
@result = @file.submission.result |
112 |
@result = @file.submission.result |
| 134 |
# Is the current user a student?
|
113 |
# Is the current user a student?
|
| 135 |
if current_user.student? |
114 |
if current_user.student? |
| 136 | # The Student does not have access to this file. Render nothing. |
115 | # The Student does not have access to this file. Display an error. |
| 137 |
if @file.submission.grouping.membership_status(current_user).nil? |
116 |
if @file.submission.grouping.membership_status(current_user).nil? |
| 138 | raise "No access to submission file with id #{@submission_file_id}" |
117 | render :partial => 'shared/handle_error', |
| 118 |
:locals => {:error => I18n.t('submission_file.error.no_access', :submission_file_id => @submission_file_id)} |
||
| 119 |
return
|
||
| 139 |
end
|
120 |
end
|
| 140 |
end
|
121 |
end
|
| 141 | 122 | ||
| 142 |
@annots = @file.annotations |
123 |
@annots = @file.annotations |
| 143 |
@all_annots = @file.submission.annotations |
124 |
@all_annots = @file.submission.annotations |
| 144 | 125 | ||
| 145 |
begin
|
126 |
begin
|
| 146 |
@file_contents = retrieve_file(@file) |
127 |
@file_contents = retrieve_file(@file) |
| 147 |
rescue Exception => e |
128 |
rescue Exception => e |
| 148 | render :update do |page| |
129 | render :partial => 'shared/handle_error', |
| 149 | page.call "alert", e.message |
130 | :locals => {:error => e.message} |
| 150 |
end
|
||
| 151 |
return
|
131 |
return
|
| 152 |
end |
132 |
end |
| 153 |
|
133 |
|
| 154 |
@code_type = @file.get_file_type |
134 |
@code_type = @file.get_file_type |
| 155 |
render :action => 'results/common/codeviewer' |
135 |
render :action => 'results/common/codeviewer' |
| 156 |
end
|
136 |
end
|
| 157 |
|
137 |
|
| 158 |
def update_mark |
138 |
def update_mark |
| 159 |
result_mark = Mark.find(params[:mark_id]) |
139 |
result_mark = Mark.find(params[:mark_id]) |
| 160 |
mark_value = params[:mark] |
140 |
mark_value = params[:mark] |
| 161 |
result_mark.mark = mark_value |
141 |
result_mark.mark = mark_value |
| 162 |
if !result_mark.save |
142 |
if !result_mark.save |
| 163 | render :update do |page| |
143 | render :partial => 'shared/handle_error', :locals => {:error => I18n.t('mark.error.save') + result_mark.errors} |
| 164 |
page.call 'alert', 'Could not save this mark!: ' + result_mark.errors |
||
| 165 |
end
|
||
| 166 |
else
|
144 |
else
|
| 167 | render :update do |page| |
145 | render :partial => 'results/marker/update_mark', |
| 168 | page.call 'select_mark', result_mark.id, mark_value |
146 | :locals => { :result_mark => result_mark, :mark_value => mark_value} |
| 169 |
page.replace_html "rubric_criterion_title_#{result_mark.id.to_s}_mark", "<b> #{result_mark.mark}: #{result_mark.rubric_criterion["level_" + result_mark.mark.to_s + "_name"]}</b> #{result_mark.rubric_criterion["level_" + result_mark.mark.to_s + "_description"]}" |
||
| 170 |
page.replace_html "mark_#{result_mark.id.to_s}_summary_mark", result_mark.mark |
||
| 171 |
page.replace_html "mark_#{result_mark.id.to_s}_summary_mark_after_weight", (result_mark.mark * result_mark.rubric_criterion.weight) |
||
| 172 |
page.replace_html "current_subtotal_div", result_mark.result.get_subtotal |
||
| 173 |
page.call "update_total_mark", result_mark.result.total_mark |
||
| 174 |
end
|
||
| 175 |
end
|
147 |
end
|
| 176 |
end
|
148 |
end
|
| 177 |
|
149 |
|
| 178 |
def view_marks |
150 |
def view_marks |
| 179 |
@assignment = Assignment.find(params[:id]) |
151 |
@assignment = Assignment.find(params[:id]) |
| 180 |
@grouping = current_user.accepted_grouping_for(@assignment.id) |
152 |
@grouping = current_user.accepted_grouping_for(@assignment.id) |
| 181 | 153 | ||
| 182 |
if @grouping.nil? |
154 |
if @grouping.nil? |
| 183 |
redirect_to :controller => 'assignments', :action => 'student_interface', :id => params[:id] |
155 |
redirect_to :controller => 'assignments', :action => 'student_interface', :id => params[:id] |
| 184 |
return
|
156 |
return
|
| 185 |
end
|
157 |
end
|
| 186 |
|
||
| 187 |
if !@grouping.has_submission? |
158 |
if !@grouping.has_submission? |
| 188 |
render 'results/student/no_submission' |
159 |
render 'results/student/no_submission' |
| 189 |
return
|
160 |
return
|
| 190 |
end
|
161 |
end
|
| 191 |
@submission = @grouping.get_submission_used |
162 |
@submission = @grouping.get_submission_used |
| ... | 11 lines hidden [Expand] | ||
def view_marks |
|||
| 203 |
@group = @grouping.group |
174 |
@group = @grouping.group |
| 204 |
@files = @submission.submission_files |
175 |
@files = @submission.submission_files |
| 205 |
@first_file = @files.first |
176 |
@first_file = @files.first |
| 206 |
@extra_marks_points = @result.extra_marks.points |
177 |
@extra_marks_points = @result.extra_marks.points |
| 207 |
@extra_marks_percentage = @result.extra_marks.percentage |
178 |
@extra_marks_percentage = @result.extra_marks.percentage |
| 208 | @marks_map = [] |
179 | @marks_map = Hash.new |
| 209 |
@rubric_criteria.each do |criterion| |
180 |
@rubric_criteria.each do |criterion| |
| 210 |
mark = Mark.find_or_create_by_result_id_and_rubric_criterion_id(@result.id, criterion.id) |
181 |
mark = Mark.find_or_create_by_result_id_and_rubric_criterion_id(@result.id, criterion.id) |
| 211 |
mark.save(false) |
182 |
mark.save(false) |
| 212 |
@marks_map[criterion.id] = mark |
183 |
@marks_map[criterion.id] = mark |
| 213 |
end
|
184 |
end
|
| 214 |
end
|
185 |
end
|
| 215 |
|
186 |
|
| 216 |
def add_extra_mark |
187 |
def add_extra_mark |
| 217 |
@result = Result.find(params[:id]) |
188 |
@result = Result.find(params[:id]) |
| 218 |
if request.post? |
189 |
if request.post? |
| 219 |
@extra_mark = ExtraMark.new |
190 |
@extra_mark = ExtraMark.new |
| 220 |
@extra_mark.result = @result |
191 |
@extra_mark.result = @result |
| 221 |
@extra_mark.update_attributes(params[:extra_mark]) |
||
| 222 |
@extra_mark.unit = ExtraMark::UNITS[:points] |
192 |
@extra_mark.unit = ExtraMark::UNITS[:points] |
| 223 | if !@extra_mark.save |
193 | if !@extra_mark.update_attributes(params[:extra_mark]) |
| 224 |
render :action => 'results/marker/add_extra_mark_error' |
194 |
render :action => 'results/marker/add_extra_mark_error' |
| 225 |
else
|
195 |
else
|
| 226 |
render :action => 'results/marker/insert_extra_mark' |
196 |
render :action => 'results/marker/insert_extra_mark' |
| 227 |
end
|
197 |
end
|
| 228 |
return
|
198 |
return
|
| ... | 9 lines hidden [Expand] | ||
def remove_extra_mark |
|||
| 238 |
@extra_mark.destroy |
208 |
@extra_mark.destroy |
| 239 |
#need to recalculate total mark
|
209 |
#need to recalculate total mark
|
| 240 |
@result = @extra_mark.result |
210 |
@result = @extra_mark.result |
| 241 |
render :action => 'results/marker/remove_extra_mark' |
211 |
render :action => 'results/marker/remove_extra_mark' |
| 242 |
end
|
212 |
end
|
| 243 | |||
| 244 |
#update the mark and/or description of the extra mark
|
||
| 245 |
def update_extra_mark |
||
| 246 |
extra_mark = ExtraMark.find(params[:id]) |
||
| 247 |
#the attribute to be changed - description or mark
|
||
| 248 |
type = params[:type] |
||
| 249 |
#the new attribute value
|
||
| 250 |
val = params[:value] |
||
| 251 |
#change the value
|
||
| 252 |
extra_mark[type] = val |
||
| 253 | |||
| 254 |
#save it
|
||
| 255 |
if extra_mark.valid? && extra_mark.save |
||
| 256 |
#need to update the total mark
|
||
| 257 |
result = Result.find(extra_mark.result_id) |
||
| 258 |
result.calculate_total |
||
| 259 |
render :update do |page| |
||
| 260 |
#The following divs need to be changed
|
||
| 261 |
#1 the display of the extra mark
|
||
| 262 |
page.replace_html("extra_mark_title_#{extra_mark.id}_" + type, val) |
||
| 263 |
#2 the display of the total mark
|
||
| 264 |
page.replace_html("current_total_mark_div", result.total_mark) |
||
| 265 |
#3 the divs containing deductions/bonuses
|
||
| 266 |
page.replace_html("extra_marks_bonus", result.get_bonus_marks) |
||
| 267 |
page.replace_html("extra_marks_deducted", result.get_deductions) |
||
| 268 |
#4 the div containing the total mark at the top of the page
|
||
| 269 |
page.replace_html("current_mark_div", result.total_mark) |
||
| 270 |
end
|
||
| 271 |
else
|
||
| 272 |
output = {'status' => 'error'} |
||
| 273 |
render :json => output.to_json |
||
| 274 |
end
|
||
| 275 |
end
|
||
| 276 |
|
||
| 277 |
def update_overall_comment |
||
| 278 |
@result = Result.find(params[:id]) |
||
| 279 |
@result.overall_comment = params[:result][:overall_comment] |
||
| 280 |
@result.save |
||
| 281 |
end
|
||
| 282 |
|
213 |
|
| 283 |
def expand_criteria |
214 |
def expand_criteria |
| 284 |
@assignment = Assignment.find(params[:aid]) |
215 |
@assignment = Assignment.find(params[:aid]) |
| 285 |
@rubric_criteria = @assignment.rubric_criteria |
216 |
@rubric_criteria = @assignment.rubric_criteria |
| 286 |
render :partial => 'results/marker/expand_criteria', :locals => {:rubric_criteria => @rubric_criteria} |
217 |
render :partial => 'results/marker/expand_criteria', :locals => {:rubric_criteria => @rubric_criteria} |
| ... | 32 lines hidden [Expand] | ||
| trunk/app/views/results/common/_annotations.js.erb | |
|---|---|
| Revision 1066 | New Change |
| trunk/app/views/results/marker/_update_mark.rjs | |
|---|---|
| New File | |
| trunk/app/views/shared/_handle_error.rjs | |
|---|---|
| New File | |
| trunk/test/fixtures/assignments.yml | |
|---|---|
| Revision 1066 | New Change |
| trunk/test/fixtures/extra_marks.yml | |
|---|---|
| Revision 1066 | New Change |
| trunk/test/fixtures/flexible_criteria.yml | |
|---|---|
| Revision 1066 | New Change |
| trunk/test/fixtures/groupings.yml | |
|---|---|
| Revision 1066 | New Change |
| trunk/test/fixtures/groups.yml | |
|---|---|
| Revision 1066 | New Change |
| trunk/test/fixtures/marks.yml | |
|---|---|
| Revision 1066 | New Change |
| trunk/test/fixtures/memberships.yml | |
|---|---|
| Revision 1066 | New Change |
| trunk/test/fixtures/results.yml | |
|---|---|
| Revision 1066 | New Change |
| trunk/test/fixtures/rubric_criteria.yml | |
|---|---|
| Revision 1066 | New Change |
| trunk/test/fixtures/submission_files.yml | |
|---|---|
| Revision 1066 | New Change |
| trunk/test/fixtures/submission_rules.yml | |
|---|---|
| Revision 1066 | New Change |
| trunk/test/fixtures/submissions.yml | |
|---|---|
| Revision 1066 | New Change |
| trunk/test/fixtures/users.yml | |
|---|---|
| Revision 1066 | New Change |
| trunk/test/functional/results_controller_test.rb | |
|---|---|
| Revision 1066 | New Change |
- trunk/app/controllers/results_controller.rb:
- trunk/app/views/results/common/_annotations.js.erb:
- trunk/app/views/results/marker/_update_mark.rjs:
- trunk/app/views/shared/_handle_error.rjs:
- trunk/test/fixtures/assignments.yml:
- trunk/test/fixtures/extra_marks.yml:
- trunk/test/fixtures/flexible_criteria.yml:
- trunk/test/fixtures/groupings.yml:
- trunk/test/fixtures/groups.yml:
- trunk/test/fixtures/marks.yml:
- trunk/test/fixtures/memberships.yml:
- trunk/test/fixtures/results.yml:
- trunk/test/fixtures/rubric_criteria.yml:
- trunk/test/fixtures/submission_files.yml:
- trunk/test/fixtures/submission_rules.yml:
- trunk/test/fixtures/submissions.yml:
- trunk/test/fixtures/users.yml:
- trunk/test/functional/results_controller_test.rb:
Other reviews