fix comments showing twice on grades page

fixes OUT-2943

test plan:
- as a teacher, create a rubric and attach it
  to an assignment
- as a student, submit to the assignment
- as a teacher, grade the assignment in speedgrader, provide a
  comment on at least one criterion
- go to the gradebook, then click on the student's name to go
  to their individual student view
- expand the rubric for the assignment, the comments left should
  only appear under their respective critera rows, not in the sidebar
  underneath the description of the criterion
- in addition to the individual student's grade page, check the following
  pages to ensure consistency and comments being properly visible:
    - speedgrader
    - student submission show page

Change-Id: Ida8b0b2dbd9263a885f17b98e85a2b71c08f9137
Reviewed-on: https://gerrit.instructure.com/177886
Tested-by: Jenkins
Reviewed-by: Neil Gupta <ngupta@instructure.com>
Reviewed-by: Frank Murphy III <fmurphy@instructure.com>
QA-Review: Dariusz Dzien <ddzien@instructure.com>
Product-Review: Sidharth Oberoi <soberoi@instructure.com>

app/jsx/rubrics/Comments.js

@@ -113,12 +113,23 @@ FreeFormComments.defaultProps = {
   saveLater: false
 }
 
-const commentElement = (assessment) => (
-  assessment.comments_html ?
-    // eslint-disable-next-line react/no-danger
-    <div dangerouslySetInnerHTML={{ __html: assessment.comments_html }} />
-    : assessment.comments
-)
+const commentElement = (assessment) => {
+  if (assessment.comments_html || assessment.comments) {
+    return (
+      <div>
+        <Text size="small" weight="bold">{I18n.t("Comments")}</Text>
+        { assessment.comments_html ?
+          // eslint-disable-next-line react/no-danger
+          <div dangerouslySetInnerHTML={{ __html: assessment.comments_html }} />
+          : assessment.comments }
+      </div>
+    )
+  }
+  else {
+    return null
+  }
+}
+
 
 export const CommentText = ({ assessment, placeholder, weight }) => (
   <span className="react-rubric-break-words">

app/jsx/rubrics/Criterion.js

@@ -221,7 +221,6 @@ export default class Criterion extends React.Component {
       <CommentButton onClick={editComments} />
     ) : null
 
-    const noComments = _.isEmpty(_.get(assessment, 'comments'))
     const longDescription = criterion.long_description
     const threshold = criterion.mastery_points
 
@@ -249,14 +248,6 @@ export default class Criterion extends React.Component {
           {
             !(hidePoints || _.isNil(threshold)) ? <Threshold threshold={threshold} /> : null
           }
-          {
-            (freeForm || assessing || isSummary || noComments) ? null : (
-              <div className="assessment-comments">
-                <Text size="x-small" weight="normal">{I18n.t('Instructor Comments')}</Text>
-                {pointsComment()}
-              </div>
-            )
-          }
         </th>
         <td className="ratings">
           {ratings}

app/jsx/rubrics/__tests__/Comments.test.js

@@ -44,7 +44,7 @@ describe('The Comments component', () => {
   })
 
   it('directly renders comments_html', () => {
-    const el = rating({ editing: false }).findWhere((e) => e.children().length === 0)
+    const el = rating({ editing: false }).findWhere((e) => e.children().length === 0).last()
     expect(el.html()).toMatchSnapshot()
   })
 

app/jsx/rubrics/__tests__/Criterion.test.js

@@ -97,29 +97,6 @@ describe('Criterion', () => {
     expectState(false)
   })
 
-  it('only shows instructor comments in sidebar when relevant', () => {
-    // in particular, we only show the comment sidebar when:
-    // - there are comments
-    // - the rubric is not free-form
-    // - we are not assessing the rubric
-    // - the rubric is not in summary mode
-    const comments = (changes) => shallow(
-      <Criterion
-        assessment={assessments.points.data[1]}
-        criterion={rubrics.points.criteria[1]}
-        freeForm={false}
-        {...changes}
-      />
-    ).find('CommentText')
-
-    expect(comments()).toHaveLength(1)
-    const noComments = { ...assessments.points.data[1], comments: undefined }
-    expect(comments({ assessment: noComments })).toHaveLength(0)
-    expect(comments({ freeForm: true })).toHaveLength(0)
-    expect(comments({ onAssessmentChange: () => {} })).toHaveLength(0)
-    expect(comments({ isSummary: true })).toHaveLength(0)
-  })
-
   it('does not have a threshold when mastery_points is null / there is no outcome', () => {
     const nullified = { ...rubrics.points.criteria[1], mastery_points: null }
     const el = shallow(

app/jsx/rubrics/__tests__/__snapshots__/Criterion.test.js.snap

@@ -1134,39 +1134,6 @@ exports[`Point Rubric with a custom criterion by default renders the root compon
         open={false}
       />
     </div>
-    <div
-      className="assessment-comments"
-    >
-      <Text
-        as="span"
-        letterSpacing="normal"
-        size="x-small"
-        weight="normal"
-      >
-        Instructor Comments
-      </Text>
-      <CommentText
-        assessment={
-          Object {
-            "comments": "i'd like to say some things",
-            "comments_enabled": true,
-            "criterion_id": "_1384",
-            "description": "bleh",
-            "editComments": true,
-            "id": "7_4778",
-            "learning_outcome_id": null,
-            "points": Object {
-              "text": null,
-              "valid": true,
-              "value": 3.2,
-            },
-          }
-        }
-        key="comment"
-        placeholder=""
-        weight="light"
-      />
-    </div>
   </th>
   <td
     className="ratings"
@@ -1761,40 +1728,6 @@ exports[`Point Rubric with a outcome criterion by default renders the root compo
     <Threshold
       threshold={3}
     />
-    <div
-      className="assessment-comments"
-    >
-      <Text
-        as="span"
-        letterSpacing="normal"
-        size="x-small"
-        weight="normal"
-      >
-        Instructor Comments
-      </Text>
-      <CommentText
-        assessment={
-          Object {
-            "above_threshold": false,
-            "comments": "here too",
-            "comments_enabled": true,
-            "criterion_id": "7_391",
-            "description": "Naturligvis hadde han ogsaa længe forstaat hvorfor Inger hadde.",
-            "editComments": true,
-            "id": "7_6639",
-            "learning_outcome_id": "612",
-            "points": Object {
-              "text": null,
-              "valid": true,
-              "value": 3,
-            },
-          }
-        }
-        key="comment"
-        placeholder=""
-        weight="light"
-      />
-    </div>
   </th>
   <td
     className="ratings"

app/stylesheets/components/_rubric.scss

@@ -101,7 +101,7 @@
 }
 
 .react-rubric .rubric-freeform {
-  padding: 0px 5px;
+  padding: 5px;
 }
 
 .react-rubric .edit-freeform-comments-large {

spec/selenium/assignments/assignments_rubrics_spec.rb

@@ -649,7 +649,7 @@ def mark_rubric_for_grading(rubric, expect_confirmation, expect_dialog = true)
       expect(f(".ui-dialog div.long_description").text).to eq "This is awesome."
     end
 
-    it "should show criterion comments", priority: "2", test_id: 220333 do
+    it "should show criterion comments and only render when necessary", priority: "2", test_id: 220333 do
       # given
       comment = 'a comment'
       teacher_in_course(course: @course)
@@ -671,9 +671,9 @@ def mark_rubric_for_grading(rubric, expect_confirmation, expect_dialog = true)
       get "/courses/#{@course.id}/assignments/#{assignment.id}/submissions/#{@student.id}"
       f('.assess_submission_link').click
       # expect
-      comments = ff('.description-header')
-      expect(comments.first).to include_text('Instructor Comments').and include_text(comment)
-      expect(comments.second).not_to include_text('Instructor Comments')
+      comments = ff('.rubric-freeform')
+      expect(comments.length).to eq 1
+      expect(comments.first).to include_text(comment)
     end
 
     it "shouldn't show 'update description' button in long description dialog", priority: "2", test_id: 220334 do

spec/selenium/grades/speedgrader/speedgrader_rubric_spec.rb

@@ -114,7 +114,7 @@
       true
     }
     wait_for_ajaximations
-    expect(Speedgrader.rubric_comment_for_row("no outcome row").text).to eq to_comment
+    expect(Speedgrader.rubric_comment_for_row("no outcome row")).to include_text to_comment
   end
 
   it "should not convert invalid text to 0", priority: "2", test_id: 283751 do

spec/selenium/grades/student_grades_summary/grades_spec.rb

@@ -251,7 +251,7 @@
       expect(fj('.rubric_assessments:visible .selected')).to include_text('10')
 
       # check rubric comment
-      expect(fj('.assessment-comments:visible div').text).to eq 'cool, yo'
+      expect(fj('.rubric-freeform:visible div')).to include_text 'cool, yo'
 
       # close first rubric
       scroll_into_view("a:contains('Close Rubric'):visible")
@@ -420,7 +420,7 @@
       expect(fj("span[data-selenium='rubric_total']:visible")).to include_text('2')
 
       # check rubric comment
-      expect(fj('.assessment-comments:visible div').text).to eq 'not bad, not bad'
+      expect(fj('.rubric-freeform:visible div')).to include_text 'not bad, not bad'
     end
 
     context "with outcome gradebook enabled" do