Submitter:	Gabriel Roy-Lortie	Reviewers
Branch:		Groups:	markus_developers
Bugs:	400, 448	People:
Change Number:	None	Repository:	MarkUs Source Code Repository

Description:

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.

Testing Done:

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 3

This is not the most recent revision of the diff. The latest diff is revision 4. See what's changed.

Jump to revision:	1 2 3 4
Changes between r3 and:	1 2 3 4

Files 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/functional/results_controller_test.rb:

     class ResultsController < ApplicationController
-  before_filter      :authorize_only_for_admin, :except => [:codeviewer,
+  before_filter      :authorize_only_for_admin, :except => [:codeviewer, :edit, :update_mark, :view_marks,
-  :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]
+                        :create, :add_extra_mark, :next_grouping, :update_overall_comment, :expand_criteria,
-  before_filter      :authorize_for_ta_and_admin, :only => [:edit,
+                        :collapse_criteria, :remove_extra_mark, :expand_unmarked_criteria, :update_marking_state,
-  :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]
+                        :download]
       before_filter      :authorize_for_ta_and_admin, :only => [:edit, :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]
       before_filter      :authorize_for_user, :only => [:codeviewer]
       before_filter      :authorize_for_student, :only => [:view_marks]
-      def create
-        # Create new Result for this Submission
-        @submission_id = params[:id]
-        @submission = Submission.find(@submission_id)
-        # Is there already a result for this Submission?
-        if @submission.has_result?
-          # If so, our new Result needs to have a version number greater than the
-          # old result version.  We're also going to set this new result to be current.
-          old_result = @submission.get_result_used
-          old_result.result_version_used = false
-          old_result.save
-        end
-        new_result = Result.new
-        new_result.submission = @submission
-        new_result.marking_state = Result::MARKING_STATES[:partial]
-        new_result.save
-        redirect_to :action => 'edit', :id => new_result.id
-      end
       def index
       end
       def edit
         result_id = params[:id]
         @group = @grouping.group
         @files = @submission.submission_files
         @first_file = @files.first
         @extra_marks_points = @result.extra_marks.points
         @extra_marks_percentage = @result.extra_marks.percentage
-    @marks_map = []
+    @marks_map = Hash.new
         @rubric_criteria.each do |criterion|
           mark = Mark.find_or_create_by_result_id_and_rubric_criterion_id(@result.id, criterion.id)
           mark.save(false)
           @marks_map[criterion.id] = mark
         end
         @file = SubmissionFile.find(@submission_file_id)
         @result = @file.submission.result
         # Is the current user a student?
         if current_user.student?
-      # The Student does not have access to this file.  Render nothing.
+      # The Student does not have access to this file. Display an error.
           if @file.submission.grouping.membership_status(current_user).nil?
-        raise "No access to submission file with id #{@submission_file_id}"
+        render :partial => 'shared/handle_error',
                    :locals => {:error => I18n.t('submission_file.error.no_access', :submission_file_id => @submission_file_id)}
             return
           end
         end
         @annots = @file.annotations
         @all_annots = @file.submission.annotations
         begin
           @file_contents = retrieve_file(@file)
         rescue Exception => e
-      render :update do |page|
+      render :partial => 'shared/handle_error',
-        page.call "alert", e.message
+             :locals => {:error => e.message}
-          end
           return
         end
         @code_type = @file.get_file_type
         render :action => 'results/common/codeviewer'
       end
       def update_mark
         result_mark = Mark.find(params[:mark_id])
         mark_value = params[:mark]
         result_mark.mark = mark_value
         if !result_mark.save
-      render :update do |page|
+      render :partial => 'shared/handle_error', :locals => {:error => I18n.t('mark.error.save') + result_mark.errors}
-            page.call 'alert', 'Could not save this mark!: ' + result_mark.errors
-          end
         else
-      render :update do |page|
+      render :partial => 'results/marker/update_mark',
-        page.call 'select_mark', result_mark.id, mark_value
+             :locals => { :result_mark => result_mark, :mark_value => mark_value}
-            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"]}"
-            page.replace_html "mark_#{result_mark.id.to_s}_summary_mark", result_mark.mark
-            page.replace_html "mark_#{result_mark.id.to_s}_summary_mark_after_weight", (result_mark.mark * result_mark.rubric_criterion.weight)
-            page.replace_html "current_subtotal_div", result_mark.result.get_subtotal
-            page.call "update_total_mark", result_mark.result.total_mark
-          end
         end
       end
       def view_marks
         @assignment = Assignment.find(params[:id])
         @grouping = current_user.accepted_grouping_for(@assignment.id)
         if @grouping.nil?
           redirect_to :controller => 'assignments', :action => 'student_interface', :id => params[:id]
           return
         end
         if !@grouping.has_submission?
           render 'results/student/no_submission'
           return
         end
         @submission = @grouping.get_submission_used
         @group = @grouping.group
         @files = @submission.submission_files
         @first_file = @files.first
         @extra_marks_points = @result.extra_marks.points
         @extra_marks_percentage = @result.extra_marks.percentage
-    @marks_map = []
+    @marks_map = Hash.new
         @rubric_criteria.each do |criterion|
           mark = Mark.find_or_create_by_result_id_and_rubric_criterion_id(@result.id, criterion.id)
           mark.save(false)
           @marks_map[criterion.id] = mark
         end
       end
       def add_extra_mark
         @result = Result.find(params[:id])
         if request.post?
           @extra_mark = ExtraMark.new
           @extra_mark.result = @result
-          @extra_mark.update_attributes(params[:extra_mark])
           @extra_mark.unit = ExtraMark::UNITS[:points]
-      if !@extra_mark.save
+      if !@extra_mark.update_attributes(params[:extra_mark])
             render :action => 'results/marker/add_extra_mark_error'
           else
             render :action => 'results/marker/insert_extra_mark'
           end
           return
         @extra_mark.destroy
         #need to recalculate total mark
         @result = @extra_mark.result
         render :action => 'results/marker/remove_extra_mark'
       end
-      #update the mark and/or description of the extra mark
-      def update_extra_mark
-        extra_mark = ExtraMark.find(params[:id])
-        #the attribute to be changed - description or mark
-        type = params[:type]
-        #the new attribute value
-        val = params[:value]
-        #change the value
-        extra_mark[type] = val
-        #save it
-        if extra_mark.valid? && extra_mark.save
-          #need to update the total mark
-          result = Result.find(extra_mark.result_id)
-          result.calculate_total
-          render :update do |page|
-            #The following divs need to be changed
-            #1 the display of the extra mark
-            page.replace_html("extra_mark_title_#{extra_mark.id}_" + type, val)
-            #2 the display of the total mark
-            page.replace_html("current_total_mark_div", result.total_mark)
-            #3 the divs containing deductions/bonuses
-            page.replace_html("extra_marks_bonus", result.get_bonus_marks)
-            page.replace_html("extra_marks_deducted", result.get_deductions)
-            #4 the div containing the total mark at the top of the page
-            page.replace_html("current_mark_div", result.total_mark)
-          end
-        else
-          output = {'status' => 'error'}
-          render :json => output.to_json
-        end
-      end
-      def update_overall_comment
-        @result = Result.find(params[:id])
-        @result.overall_comment = params[:result][:overall_comment]
-        @result.save
-      end
       def expand_criteria
         @assignment = Assignment.find(params[:aid])
         @rubric_criteria = @assignment.rubric_criteria
         render :partial => 'results/marker/expand_criteria', :locals => {:rubric_criteria => @rubric_criteria}

trunk/app/views/results/common/_annotations.js.erb
Revision 1069	New Change

trunk/app/views/results/marker/_update_mark.rjs
	New File

trunk/app/views/shared/_handle_error.rjs
	New File

trunk/test/functional/results_controller_test.rb
Revision 1069	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/functional/results_controller_test.rb:

You have a pending review.

Review Board 1.5.5

This change has been marked as submitted.

ResultsController is now tested

Diff revision 3

Other reviews

Your comment