Address code review feedback

openassessment/templates/openassessmentblock/oa_rubric.html

@@ -58,14 +58,14 @@
         {% endfor %}
         <li class="wrapper--input field field--textarea assessment__rubric__question assessment__rubric__question--feedback">
             <label class="question__title" for="{{ rubric_type }}__assessment__rubric__question--feedback__value">
-                <span class="question__title__copy">{{ rubric_feedback_prompt }}</span>
+                <span class="question__title__copy">{% trans rubric_feedback_prompt %}</span>
             </label>
 
             <div class="wrapper--input">
                 <textarea
                     id="{{ rubric_type }}__assessment__rubric__question--feedback__value"
                     class="assessment__rubric__question--feedback__value"
-                    placeholder="{{ rubric_feedback_default_text }}"
+                    placeholder="{% trans rubric_feedback_default_text %}"
                     maxlength="500"
                 >
                 </textarea>

openassessment/templates/openassessmentblock/oa_uploaded_file.html

@@ -21,7 +21,7 @@
                 {% trans "View the file associated with this submission." %}
             </a>
             {% if show_warning %}
-                <p class="submission_file_warning">{% trans "(Caution: This file was uploaded by another course learner and has not been verified, screened, approved, reviewed or endorsed by edX. If you decide to access it, you do so at your own risk.)" %}</p>
+                <p class="submission_file_warning">{% trans "(Caution: This file was uploaded by another course learner and has not been verified, screened, approved, reviewed, or endorsed by edX. If you decide to access it, you do so at your own risk.)" %}</p>
             {% endif %}
         {% endif %}
     </div>

openassessment/templates/openassessmentblock/response/oa_response_cancelled.html

@@ -3,7 +3,7 @@
 {% load tz %}
 
 {% block list_item %}
-<li id="openassessment__response" class="openassessment__steps__step step--response ui-toggle-visibility has--error">
+    <li id="openassessment__response" class="openassessment__steps__step step--response ui-toggle-visibility has--error">
 {% endblock %}
 
 {% block title %}
@@ -28,23 +28,14 @@
                         {% blocktrans with removed_datetime=workflow_cancellation.cancelled_at|utc|date:"N j, Y H:i e" removed_by_username=workflow_cancellation.cancelled_by %}
                             Your submission has been cancelled by {{ removed_by_username }} on {{ removed_datetime }}
                         {% endblocktrans %}
-                    <br>
+                    </p>
+                    <p>
                         <!-- Comments: Reason for Cancellation-->
                         {% blocktrans with comments=workflow_cancellation.comments %}
                             Comments: {{ comments }}
                         {% endblocktrans %}
                     </p>
                 </div>
-
-                <div class="step__content">
-                    <article class="submission__answer__display">
-                        <h3 class="submission__answer__display__title">{% trans "Your Response" %}</h3>
-
-                        <div class="submission__answer__display__content">
-                            {{ student_submission.answer.text|linebreaks }}
-                        </div>
-                    </article>
-                </div>
             </div>
         </div>
     </div>

openassessment/templates/openassessmentblock/staff_area/oa_staff_assessment.html

-{% load tz %}
 {% load i18n %}
 {% spaceless %}
 
@@ -6,7 +5,7 @@
     <div class="ui-toggle-visibility__content">
         <div class="wrapper--staff-assessment">
             <div class="step__instruction">
-                <p>{% trans "Allows you to override the current learner's grade using the problem's rubric." %}</p>
+                <p>{% trans "Override this learner's current grade using the problem's rubric." %}</p>
             </div>
 
             <div class="step__content">
@@ -42,6 +41,8 @@
                         <button type="submit" class="action action--submit is--disabled">
                             <span class="copy">{% trans "Submit your assessment" %}</span>
                         </button>
+
+                        <div class="staff-override-error"></div>
                     </li>
                 </ul>
             </div>

openassessment/templates/openassessmentblock/staff_area/oa_student_info.html

@@ -16,7 +16,7 @@
         <div class="ui-staff ui-toggle-visibility is--collapsed">
             <h2 class="staff-info__title ui-staff__subtitle ui-toggle-visibility__control">
                 <i class="icon fa fa-caret-right" aria-hidden="true"></i>
-                <span>{% trans "Learner Response" %}</span>
+                <span>{% trans "Learner's Response" %}</span>
             </h2>
             <div class="ui-toggle-visibility__content">
                 {% if workflow_cancellation %}
@@ -39,13 +39,13 @@
                     <a href="{{ submission.file_url }}" class="submission--file">
                         {% trans "The file associated with this response." %}
                     </a>
-                    <span>{%  trans "Caution: This file was uploaded by another course learner and has not been verified, screened, approved, reviewed or endorsed by edX. If you decide to access it, you do so at your own risk." %}</span>
+                    <span>{%  trans "Caution: This file was uploaded by another course learner and has not been verified, screened, approved, reviewed, or endorsed by edX. If you decide to access it, you do so at your own risk." %}</span>
                 {% endif %}
             </div>
         </div>
     </div>
 
-    {% if peer_assessments %}
+    {% if peer_assessments != None %}
         <div class="staff-info__status ui-staff__content__section wrapper--ui--collapse">
             <div class="ui-staff ui-toggle-visibility is--collapsed">
                 <h2 class="staff-info__title ui-staff__subtitle ui-toggle-visibility__control">
@@ -53,6 +53,7 @@
                     <span>{% trans "Peer Assessments for This Learner" %}</span>
                 </h2>
                 <div class="ui-toggle-visibility__content">
+                    {% if peer_assessments %}
                         {% for assessment in peer_assessments %}
                             {% with peer_num=forloop.counter %}
                                 <h4 class="title--sub"> {% trans "Peer" %} {{ peer_num }}: </h4>
@@ -87,12 +88,15 @@
                                 </div>
                             {% endwith %}
                         {%  endfor %}
+                    {% else %}
+                        <p>{% trans "This learner currently has no peer assessments." %}</p>
+                    {% endif %}
                 </div>
             </div>
         </div>
     {% endif %}
 
-    {% if submitted_assessments %}
+    {% if submitted_assessments != None %}
         <div class="staff-info__status ui-staff__content__section wrapper--ui--collapse">
             <div class="ui-staff ui-toggle-visibility is--collapsed">
                 <h2 class="staff-info__title ui-staff__subtitle ui-toggle-visibility__control">
@@ -100,6 +104,7 @@
                     <span>{% trans "Peer Assessments Completed by This Learner" %}</span>
                 </h2>
                 <div class="ui-toggle-visibility__content">
+                    {% if submitted_assessments %}
                         {% for assessment in submitted_assessments %}
                             {% with peer_num=forloop.counter %}
                                 <h4 class="title--sub">{% trans "Assessment" %} {{ peer_num }}:</h4>
@@ -136,6 +141,9 @@
                                 </div>
                             {% endwith %}
                         {%  endfor %}
+                    {% else %}
+                        <p>{% trans "This learner has not completed any peer assessments." %}</p>
+                    {% endif %}
                 </div>
             </div>
         </div>
@@ -225,15 +233,17 @@
             </h2>
             <div class="ui-toggle-visibility__content">
                 {% if workflow_status == "done" %}
+                    <p>
                     {% with points_earned_string=score.points_earned|stringformat:"s" points_possible_string=score.points_possible|stringformat:"s" %}
                         {% blocktrans with points_earned='<span class="grade__value__earned">'|safe|add:points_earned_string|add:'</span>'|safe points_possible='<span class="grade__value__potential">'|safe|add:points_possible_string|add:'</span>'|safe %}
                             Final grade: {{ points_earned }} out of {{ points_possible }}
                         {% endblocktrans %}
                     {% endwith %}
+                    </p>
                 {% elif workflow_status == "waiting" %}
                     <p>{% trans "The submission is waiting for assessments." %}</p>
                 {% elif workflow_status == "cancelled" %}
-                    <p>{% trans "The learner's submission has been removed from peer assessment. The learner receives a grade of zero unless you reset the learner's attempts for the problem to allow them to resubmit a response." %}</p>
+                    <p>{% trans "The learner's submission has been removed from peer assessment. The learner receives a grade of zero unless you delete the learner's state for the problem to allow them to resubmit a response." %}</p>
                 {% elif workflow_status == None %}
                     <p>{% trans "The problem has not been started." %}</p>
                 {% else %}

openassessment/xblock/staff_area_mixin.py

@@ -231,7 +231,8 @@ class StaffAreaMixin(object):
 
         Args:
             student_username (unicode): The username of the student to report.
-
+            expanded_view (str): An optional view to be shown initially expanded.
+                The default is None meaning that all views are shown collapsed.
         """
         submission_uuid = None
         submission = None
@@ -269,8 +270,8 @@ class StaffAreaMixin(object):
 
         example_based_assessment = None
         self_assessment = None
-        peer_assessments = []
-        submitted_assessments = []
+        peer_assessments = None
+        submitted_assessments = None
 
         if "peer-assessment" in assessment_steps:
             peer_assessments = peer_api.get_assessments(submission_uuid)

openassessment/xblock/static/js/fixtures/templates.json

@@ -758,6 +758,68 @@
         "output": "oa_staff_cancelled_submission.html"
     },
     {
+        "template": "openassessmentblock/staff_area/oa_student_info.html",
+        "context": {
+            "rubric_criteria": [
+                {
+                    "name": "vocabulary",
+                    "prompt": "vocabulary",
+                    "order_num": 0,
+                    "feedback": "optional",
+                    "options": [
+                        {
+                            "order_num": 0,
+                            "points": 0,
+                            "name": "Bad"
+                        },
+                        {
+                            "order_num": 1,
+                            "points": 1,
+                            "name": "Good"
+                        }
+                    ]
+                },
+                {
+                    "name": "grammar",
+                    "prompt": "grammar",
+                    "order_num": 1,
+                    "options": [
+                        {
+                            "order_num": 0,
+                            "points": 0,
+                            "name": "Bad"
+                        },
+                        {
+                            "order_num": 1,
+                            "points": 1,
+                            "name": "Good"
+                        }
+                    ]
+                },
+                {
+                    "name": "feedback_only",
+                    "prompt": "Feedback only, no options!",
+                    "order_num": 2,
+                    "feedback": "required",
+                    "options": []
+                }
+            ],
+            "submission": {
+                "image_url": "/test-url",
+                "answer": {
+                    "text": "testing response text"
+                }
+            },
+            "score": {
+                "points_earned": 1,
+                "points_possible": 2
+            },
+            "workflow_status": "done",
+            "expanded_view": "final-grade"
+        },
+        "output": "oa_staff_graded_submission.html"
+    },
+    {
         "template": "openassessmentblock/peer/oa_peer_assessment.html",
         "context": {
             "rubric_criteria": [

openassessment/xblock/static/js/spec/lms/oa_staff_area.js

@@ -4,6 +4,18 @@
 describe('OpenAssessment.StaffAreaView', function() {
     'use strict';
 
+    var successPromise = $.Deferred(
+        function(defer) { defer.resolve(); }
+    ).promise();
+
+    var failWith = function(owner, result) {
+        return function() {
+            return $.Deferred(function(defer) {
+                defer.rejectWith(owner, [result]);
+            }).promise();
+        };
+    };
+
     // Stub server that returns dummy data for the staff info view
     var StubServer = function() {
         this.studentTemplate = 'oa_student_info.html';
@@ -43,10 +55,6 @@ describe('OpenAssessment.StaffAreaView', function() {
             }).promise();
         };
 
-        var successPromise = $.Deferred(
-            function(defer) { defer.resolve(); }
-        ).promise();
-
         this.cancelSubmission = function() {
             return successPromise;
         };
@@ -300,21 +308,42 @@ describe('OpenAssessment.StaffAreaView', function() {
                     'Comments: Cancelled!'
                 );
             });
+
+            it('shows an error message when a cancel submission request fails', function() {
+                // Show the staff area for the test student
+                var staffArea = createStaffArea(),
+                    serverErrorMessage = 'Mock server error';
+                chooseStudent(staffArea, 'testStudent');
+
+                // Cancel the student's submission but return a server error
+                staffArea.comment('Cancellation reason.');
+                server.cancelSubmission = failWith(server, serverErrorMessage);
+                staffArea.cancelSubmission('Bob');
+
+                // Verify that the error message is shown
+                expect($('.cancel-submission-error', staffArea.element).first().text().trim()).toBe(serverErrorMessage);
+            });
         });
 
         describe('Staff Grade Override', function() {
             var fillAssessment = function($assessment) {
                 $('#staff__assessment__rubric__question--2__feedback', $assessment).val('Text response');
-                $('.question__answers', $assessment).each(function(element) {
+                $('.question__answers', $assessment).each(function() {
                     $('input[type="radio"]', this).first().click();
                 });
             };
 
+            var submitAssessment = function(staffArea) {
+                var $assessment = $('.wrapper--staff-assessment', staffArea.element),
+                    $submitButton = $('.action--submit', $assessment);
+                $submitButton.click();
+            };
+
             it('enables the submit button when all required fields are specified', function() {
                 var staffArea = createStaffArea(),
                     $assessment, $submitButton;
                 chooseStudent(staffArea, 'testStudent');
-                $assessment = $('.wrapper--staff-assessment', staffArea.element)
+                $assessment = $('.wrapper--staff-assessment', staffArea.element);
                 $submitButton = $('.action--submit', $assessment);
                 expect($submitButton).toHaveClass('is--disabled');
                 fillAssessment($assessment);
@@ -323,24 +352,39 @@ describe('OpenAssessment.StaffAreaView', function() {
 
             it('can submit a staff grade override', function() {
                 var staffArea = createStaffArea(),
-                    $assessment, $submitButton;
+                    $assessment, $gradeSection;
                 chooseStudent(staffArea, 'testStudent');
-                $assessment = $('.wrapper--staff-assessment', staffArea.element)
-                $submitButton = $('.action--submit', $assessment);
+                $assessment = $('.wrapper--staff-assessment', staffArea.element);
                 fillAssessment($assessment);
 
                 // Submit the assessment
-                server.studentTemplate = 'oa_staff_cancelled_submission.html';
-                $submitButton.click();
-
-                // Verify that the student info reflects the update
-                expect($($('.staff-info__student__response p', staffArea.element)[0]).text().trim()).toBe(
-                    'Learner submission removed by staff on October 1, 2015 04:53 UTC'
-                );
-                expect($($('.staff-info__student__response p', staffArea.element)[1]).text().trim()).toBe(
-                    'Comments: Cancelled!'
+                server.studentTemplate = 'oa_staff_graded_submission.html';
+                submitAssessment(staffArea);
+
+                // Verify that the student info is visible and shows the correct score
+                $gradeSection = $('.staff-info__student__grade', staffArea.element);
+                expect($('.ui-toggle-visibility', $gradeSection)).not.toHaveClass('is--collapsed');
+                expect($('p', $gradeSection).first().text().trim()).toBe(
+                    'Final grade: 1 out of 2'
                 );
             });
+
+            it('shows an error message when a grade override request fails', function() {
+                var staffArea = createStaffArea(),
+                    serverErrorMessage = 'Mock server error',
+                    $assessment;
+                chooseStudent(staffArea, 'testStudent');
+                $assessment = $('.wrapper--staff-assessment', staffArea.element);
+                fillAssessment($assessment);
+
+                // Submit the assessment but return a server error message
+                staffArea.comment('Cancellation reason.');
+                server.staffAssess = failWith(server, serverErrorMessage);
+                submitAssessment(staffArea);
+
+                // Verify that the error message is shown
+                expect($('.staff-override-error', staffArea.element).first().text().trim()).toBe(serverErrorMessage);
+            });
         });
     });
 

openassessment/xblock/static/js/src/lms/oa_peer.js

@@ -150,6 +150,7 @@ OpenAssessment.PeerView.prototype = {
             return !button.hasClass('is--disabled');
         } else {
             button.toggleClass('is--disabled', !enabled);
+            return enabled;
         }
     },
 

openassessment/xblock/static/js/src/lms/oa_response.js

@@ -169,6 +169,7 @@ OpenAssessment.ResponseView.prototype = {
             return !sel.hasClass('is--disabled');
         } else {
             sel.toggleClass('is--disabled', !enabled);
+            return enabled;
         }
     },
 

openassessment/xblock/static/js/src/lms/oa_self.js

@@ -91,6 +91,7 @@ OpenAssessment.SelfView.prototype = {
             return !button.hasClass('is--disabled');
         } else {
             button.toggleClass('is--disabled', !enabled);
+            return enabled;
         }
     },
 

openassessment/xblock/static/js/src/lms/oa_staff_area.js

@@ -30,16 +30,14 @@
             // for us to render the staff area into.  If that doesn't exist,
             // then we're not staff, so we don't need to send the AJAX request.
             if ($('.openassessment__staff-area', view.element).length > 0) {
-                this.server.render('staff_area')
-                    .done(function(html) {
+                this.server.render('staff_area').done(function(html) {
                     // Load the HTML and install event handlers
                     $('.openassessment__staff-area', view.element).replaceWith(html);
                     view.server.renderLatex($('.openassessment__staff-area', view.element));
                     view.installHandlers();
                 }).fail(function() {
                     view.baseView.showLoadError('staff_area');
-                    }
-                );
+                });
             }
         },
 
@@ -66,8 +64,7 @@
             $('.openassessment__student-info', view.element).text('');
 
             if (student_username.trim()) {
-                this.server.studentInfo(student_username, options)
-                    .done(function(html) {
+                this.server.studentInfo(student_username, options).done(function(html) {
                     // Clear any error message
                     showFormError('');
 
@@ -108,8 +105,7 @@
                         );
                     }
                     deferred.resolve();
-                    })
-                    .fail(function() {
+                }).fail(function() {
                     showFormError(gettext('Unexpected server error.'));
                     deferred.reject();
                 });
@@ -203,8 +199,7 @@
          */
         scheduleTraining: function() {
             var view = this;
-            this.server.scheduleTraining()
-                .done(function(msg) {
+            this.server.scheduleTraining().done(function(msg) {
                 $('.schedule_training_message', view.element).text(msg);
             }).fail(function(errMsg) {
                 $('.schedule_training_message', view.element).text(errMsg);
@@ -218,8 +213,7 @@
          */
         rescheduleUnfinishedTasks: function() {
             var view = this;
-            this.server.rescheduleUnfinishedTasks()
-                .done(function(msg) {
+            this.server.rescheduleUnfinishedTasks().done(function(msg) {
                 $('.reschedule_unfinished_tasks_message', view.element).text(msg);
             }).fail(function(errMsg) {
                 $('.reschedule_unfinished_tasks_message', view.element).text(errMsg);
@@ -234,16 +228,14 @@
             this.cancelSubmissionEnabled(false);
             var view = this;
             var comments = $('.cancel_submission_comments', this.element).val();
-            this.server.cancelSubmission(submissionUUID, comments)
-                .done(function(msg) {
-                    $('.cancel-submission-error').html('');
-                    view.loadStudentInfo({expanded_view: 'final-grade'})
-                        .done(function() {
-                            $('.openassessment__staff-info__cancel__submission', view.element).html(msg);
-                        });
-                })
-                .fail(function(errMsg) {
-                    $('.cancel-submission-error').html(errMsg);
+            this.server.cancelSubmission(submissionUUID, comments).done(function() {
+                // Note: we ignore any message returned from the server and instead
+                // re-render the student info with the "Learner's Final Grade"
+                // section expanded. This section will show that the learner's
+                // submission was cancelled.
+                view.loadStudentInfo({expanded_view: 'final-grade'});
+            }).fail(function(errorMessage) {
+                $('.cancel-submission-error').html(errorMessage);
             });
         },
 
@@ -315,6 +307,7 @@
                 return !button.hasClass('is--disabled');
             } else {
                 button.toggleClass('is--disabled', !enabled);
+                return enabled;
             }
         },
 
@@ -332,12 +325,15 @@
 
             this.server.staffAssess(
                 rubric.optionsSelected(), rubric.criterionFeedback(), rubric.overallFeedback(), submissionID
-                )
-                .done(function() {
+            ).done(function() {
+                // Note: we ignore any message returned from the server and instead
+                // re-render the student info with the "Learner's Final Grade"
+                // section expanded. This section will show the learner's
+                // final grade and in the future should include details of
+                // the staff override itself.
                 view.loadStudentInfo({expanded_view:  'final-grade'});
-                })
-                .fail(function(errorMessage) {
-                    baseView.toggleActionError('staff', errorMessage);
+            }).fail(function(errorMessage) {
+                $('.staff-override-error').html(errorMessage);
                 view.staffSubmitEnabled(true);
             });
         }

openassessment/xblock/test/test_staff_area.py

@@ -231,7 +231,7 @@ class TestCourseStaff(XBlockHandlerTestCase):
 
         path, context = xblock.get_student_info_path_and_context("Bob")
         self.assertEquals("Bob Answer 1", context['submission']['answer']['parts'][0]['text'])
-        self.assertEquals([], context['peer_assessments'])
+        self.assertEquals(None, context['peer_assessments'])
         self.assertEquals("openassessmentblock/staff_area/oa_student_info.html", path)
 
     @scenario('data/basic_scenario.xml', user_id='Bob')

test/acceptance/tests.py

@@ -399,7 +399,7 @@ class StaffAreaTest(OpenAssessmentTest):
 
         self.staff_area_page.verify_learner_final_score(
             "The learner's submission has been removed from peer assessment. "
-            "The learner receives a grade of zero unless you reset the learner's attempts for the "
+            "The learner receives a grade of zero unless you delete the learner's state for the "
             "problem to allow them to resubmit a response."
         )