From 11c04b5e39759bedde9b619d64d4d57b49f9462c Mon Sep 17 00:00:00 2001
From: Ed Schiebel <eschiebel@instructure.com>
Date: Mon, 18 Nov 2019 13:52:11 -0500
Subject: [PATCH] Improve handling of the "blank" option from group selector

closes LA-141

The empty option was added when the "New Group Category" option
was converted to a button for accessibility (see OUT-57).
Now I hide the selector + its label if there are no group
sets.

test plan:
  - in a course with no group sets
  - create an assignment
  - check the "group assignment" checkbox
  > expect the "create group set" modal to appear
  - dismiss it w/o creating a group
  > expect the "group set" label and its <select> to be hidden
  - click the "New Group Category" button and create a group set
  > expect the "group set" label and its <select> to be displayed

  - editing an assignment that has at least 1 group set
  - check the group assignment checkbox
  > expect the select with "Select a group catetory" item displayed
  > expect the other options to be the existing group sets
  - try to save
  > expect an alert that you must select a group set
  - select a group set
  > expect to be able to save
  - edit again
  > expect the group to be selected

  From the ticket
  - Create a course with at least one student who is part of a group
  - Create or edit a group assignments
  - Select the group set dropdown and see that there is an extra empty selection there
  - Give your group assignment a due date and save it
  - Edit your assignment and select the empty option from the group set dropdown and try to save
  - After receiving the error message, select the actual group set
  - Change the date from being assigned to everyone to just being assigned to the student's group specifically
  - Save the assignment
  - As the student view the assignment and notice that the due date does not display.

Change-Id: I53f52f38f89bb37bd26ad0cd892570138178ad65
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/217643
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Tested-by: Jenkins
Reviewed-by: Carl Kibler <ckibler@instructure.com>
QA-Review: Carl Kibler <ckibler@instructure.com>
QA-Review: Jeremy Putnam <jeremyp@instructure.com>
Product-Review: Carl Kibler <ckibler@instructure.com>
---
 .../assignments/GroupCategorySelector.coffee  | 20 +++--
 .../GroupCategorySelector.handlebars          | 81 +++++++++----------
 .../assignments/GroupCategorySelectorSpec.js  | 31 ++++++-
 3 files changed, 78 insertions(+), 54 deletions(-)

diff --git a/app/coffeescripts/views/assignments/GroupCategorySelector.coffee b/app/coffeescripts/views/assignments/GroupCategorySelector.coffee
index 21c776dcf14..8a6f4b97510 100644
--- a/app/coffeescripts/views/assignments/GroupCategorySelector.coffee
+++ b/app/coffeescripts/views/assignments/GroupCategorySelector.coffee
@@ -29,6 +29,7 @@ export default class GroupCategorySelector extends Backbone.View
 
   template: template
 
+  GROUP_CATEGORY = '#assignment_group_category'
   GROUP_CATEGORY_ID = '#assignment_group_category_id'
   CREATE_GROUP_CATEGORY_ID = '#create_group_category_id'
   HAS_GROUP_CATEGORY = '#has_group_category'
@@ -36,6 +37,7 @@ export default class GroupCategorySelector extends Backbone.View
 
   els: do ->
     els = {}
+    els["#{GROUP_CATEGORY}"] = '$groupCategory'
     els["#{GROUP_CATEGORY_ID}"] = '$groupCategoryID'
     els["#{HAS_GROUP_CATEGORY}"] = '$hasGroupCategory'
     els["#{GROUP_CATEGORY_OPTIONS}"] = '$groupCategoryOptions'
@@ -66,11 +68,11 @@ export default class GroupCategorySelector extends Backbone.View
     if _.isEmpty(@groupCategories)
       StudentGroupStore.setSelectedGroupSet(null)
     else if !selectedID? or !_.findWhere(@groupCategories, {id: selectedID.toString()})?
-      StudentGroupStore.setSelectedGroupSet('blank')
+      StudentGroupStore.setSelectedGroupSet(null)
     else
       StudentGroupStore.setSelectedGroupSet(selectedID)
     super
-    @$groupCategoryID.toggleAccessibly !_.isEmpty(@groupCategories)
+    @$groupCategory.toggleAccessibly !_.isEmpty(@groupCategories)
 
   groupCategorySelected: =>
     newSelectedId = @$groupCategoryID.val()
@@ -83,10 +85,11 @@ export default class GroupCategorySelector extends Backbone.View
       $newCategory = $('<option>')
       $newCategory.val(group.id)
       $newCategory.text(group.name)
-      @$groupCategoryID.prepend $newCategory
+      $newCategory.prop('selected', true)
+      @$groupCategoryID.append $newCategory
       @$groupCategoryID.val(group.id)
       @groupCategories.push(group)
-      @$groupCategoryID.toggleAccessibly true
+      @$groupCategory.toggleAccessibly true
     view.open()
 
   groupDiscussionChecked: =>
@@ -114,12 +117,16 @@ export default class GroupCategorySelector extends Backbone.View
     groupCategoryFrozen = _.includes frozenAttributes, 'group_category_id'
     groupCategoryLocked = !@parentModel.canGroup()
 
+    isGroupAssignment: @parentModel.groupCategoryId() && @parentModel.groupCategoryId() != 'blank'
     groupCategoryId: @parentModel.groupCategoryId()
     groupCategories: @groupCategories
-    originalGroupRemoved: !_.chain(@groupCategories)
+    groupCategoryUnselected: !@parentModel.groupCategoryId() ||
+                              @parentModel.groupCategoryId() == 'blank' ||
+                            !_.chain(@groupCategories)
                             .pluck('id')
                             .contains(@parentModel.groupCategoryId())
                             .value() && !_.isEmpty(@groupCategories)
+
     hideGradeIndividually: @hideGradeIndividually
     gradeGroupStudentsIndividually: !@hideGradeIndividually && @parentModel.gradeGroupStudentsIndividually()
     groupCategoryLocked: groupCategoryLocked
@@ -160,7 +167,8 @@ export default class GroupCategorySelector extends Backbone.View
       data.assignment.groupCategoryId()
     else
       data.group_category_id
-    if gcid == 'blank'
+
+    if gcid == "blank"
       if _.isEmpty(@groupCategories)
         errors["newGroupCategory"] = [
           message: I18n.t 'Please create a group set'
diff --git a/app/views/jst/assignments/GroupCategorySelector.handlebars b/app/views/jst/assignments/GroupCategorySelector.handlebars
index 38064b14961..8effb09ac18 100644
--- a/app/views/jst/assignments/GroupCategorySelector.handlebars
+++ b/app/views/jst/assignments/GroupCategorySelector.handlebars
@@ -7,10 +7,10 @@
     {{!-- Group Assignment selection --}}
     <label class="checkbox flush" for="has_group_category">
       {{#if inClosedGradingPeriod}}
-        {{checkbox "has_group_category"
+      {{checkbox "has_group_category"
           id="has_group_category"
           prefix=prefix
-          checked=groupCategoryId
+          checked=isGroupAssignment
           aria-role="option"
           aria-checked=ariaChecked
           aria-controls="group_category_options"
@@ -19,10 +19,10 @@
           disabled=hasGroupCategoryDisabled
         }}
       {{else}}
-        {{checkbox "has_group_category"
+      {{checkbox "has_group_category"
           id="has_group_category"
           prefix=prefix
-          checked=groupCategoryId
+          checked=isGroupAssignment
           aria-role="option"
           aria-checked=ariaChecked
           aria-controls="group_category_options"
@@ -36,9 +36,9 @@
     <div id="group_category_options" style="{{hiddenUnless groupCategoryId}}">
       <div class="nested">
         {{#unless hideGradeIndividually}}
-          <label class="checkbox" for="assignment_grade_students_individually">
-            {{#if inClosedGradingPeriod}}
-              {{checkbox "grade_group_students_individually"
+        <label class="checkbox" for="assignment_grade_students_individually">
+          {{#if inClosedGradingPeriod}}
+          {{checkbox "grade_group_students_individually"
                 id="assignment_grade_students_individually"
                 prefix=prefix
                 checked=gradeGroupStudentsIndividually
@@ -46,60 +46,53 @@
                 aria-readonly="true"
                 disabled=gradeIndividuallyDisabled
               }}
-            {{else}}
-              {{checkbox "grade_group_students_individually"
+          {{else}}
+          {{checkbox "grade_group_students_individually"
                 id="assignment_grade_students_individually"
                 prefix=prefix
                 checked=gradeGroupStudentsIndividually
                 disabled=gradeIndividuallyDisabled
               }}
-            {{/if}}
-            {{#t "grade_group_students_individually"}}
-              Assign Grades to Each Student Individually
-            {{/t}}
-          </label>
+          {{/if}}
+          {{#t "grade_group_students_individually"}}
+          Assign Grades to Each Student Individually
+          {{/t}}
+        </label>
         {{/unless}}
 
         {{!-- Group selection --}}
-        <label id="assignment_group_category_id_label" for="group_category_id">
-          {{#t "group_set"}}Group Set{{/t}}
-        </label>
-        <select id="assignment_group_category_id"
-          name="{{#if nested}}assignment[group_category_id]{{else}}group_category_id{{/if}}"
-          {{#if inClosedGradingPeriod}}
-            readonly
-            aria-readonly="true"
-          {{/if}}
-          aria-labelledby="assignment_group_category_id_label"
-          {{disabledIf groupCategoryIdDisabled}}>
-          {{#each groupCategories}}
+        <div id="assignment_group_category">
+          <label id="assignment_group_category_id_label" for="group_category_id">
+            {{#t "group_set"}}Group Set{{/t}}
+          </label>
+          <select id="assignment_group_category_id"
+            name="{{#if nested}}assignment[group_category_id]{{else}}group_category_id{{/if}}"
+            {{#if inClosedGradingPeriod}} readonly aria-readonly="true" {{/if}}
+            aria-labelledby="assignment_group_category_id_label" {{disabledIf groupCategoryIdDisabled}}>
+            <option value="blank" {{selectedIf groupCategoryUnselected}}>{{#t}}Select a group category{{/t}}</option>
+            {{#each groupCategories}}
             <option value="{{id}}" {{selectedIf ../groupCategoryId this.id}}>
               {{name}}
             </option>
-          {{/each}}
-          <option value="blank" {{selectedIf originalGroupRemoved}}></option>
-        </select>
+            {{/each}}
+          </select>
+        </div>
         <div>
-          <button
-            class="Button"
-            type="button"
-            id="create_group_category_id"
-            {{disabledIf inClosedGradingPeriod}}
-            {{disabledIf groupCategoryIdDisabled}}
-            >
+          <button class="Button" type="button" id="create_group_category_id" {{disabledIf inClosedGradingPeriod}}
+            {{disabledIf groupCategoryIdDisabled}}>
             {{#t}}New Group Category{{/t}}
           </button>
         </div>
       </div>
     </div>
     {{#if groupCategoryLocked}}
-      <div class="group_category_locked_explanation alert assignment-edit-group-alert">
-        {{#if lockedMessage}}{{lockedMessage}}{{else}}
-          {{#t "group_category_locked_explanation"}}Students have already
-          submitted homework on this assignment, so group settings cannot be
-          changed.{{/t}}
-        {{/if}}
-      </div>
+    <div class="group_category_locked_explanation alert assignment-edit-group-alert">
+      {{#if lockedMessage}}{{lockedMessage}}{{else}}
+      {{#t "group_category_locked_explanation"}}Students have already
+      submitted homework on this assignment, so group settings cannot be
+      changed.{{/t}}
+      {{/if}}
+    </div>
     {{/if}}
   </div>
-</div>
+</div>
\ No newline at end of file
diff --git a/spec/coffeescripts/views/assignments/GroupCategorySelectorSpec.js b/spec/coffeescripts/views/assignments/GroupCategorySelectorSpec.js
index 91ea822ab1b..9bf7885da47 100644
--- a/spec/coffeescripts/views/assignments/GroupCategorySelectorSpec.js
+++ b/spec/coffeescripts/views/assignments/GroupCategorySelectorSpec.js
@@ -21,8 +21,9 @@ import Assignment from 'compiled/models/Assignment'
 import StudentGroupStore from 'jsx/due_dates/StudentGroupStore'
 import $ from 'jquery'
 
-QUnit.module('GroupCategorySelector', {
-  setup() {
+/* eslint-disable object-shorthand */
+QUnit.module('GroupCategorySelector selection', {
+  beforeEach: function() {
     this.assignment = new Assignment()
     this.assignment.groupCategoryId('1')
     this.groupCategories = [
@@ -42,15 +43,37 @@ QUnit.module('GroupCategorySelector', {
     this.groupCategorySelector.render()
     return $('#fixtures').append(this.groupCategorySelector.$el)
   },
-  teardown() {
+  afterEach: function() {
     this.groupCategorySelector.remove()
     $('#fixtures').empty()
   }
 })
 
-test("groupCategorySelected should set StudentGroupStore's group set", function() {
+QUnit.test("groupCategorySelected should set StudentGroupStore's group set", function() {
   strictEqual(StudentGroupStore.getSelectedGroupSetId(), '1')
   this.groupCategorySelector.$groupCategoryID.val(2)
   this.groupCategorySelector.groupCategorySelected()
   strictEqual(StudentGroupStore.getSelectedGroupSetId(), '2')
 })
+
+QUnit.module('GroupCategorySelector, no groups', {
+  beforeEach: function() {
+    this.assignment = new Assignment()
+    this.groupCategorySelector = new GroupCategorySelector({
+      parentModel: this.assignment,
+      groupCategories: []
+    })
+    this.groupCategorySelector.render()
+    return $('#fixtures').append(this.groupCategorySelector.$el)
+  },
+  afterEach: function() {
+    this.groupCategorySelector.remove()
+    $('#fixtures').empty()
+  }
+})
+
+QUnit.test('group category select is hidden when there are no group sets', () => {
+  const $group_category = $('#fixtures #assignment_group_category')
+  strictEqual($group_category.css('display'), 'none')
+})
+/* eslint-enable object-shorthand */