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>
2019-11-18 13:52:11 -05:00 · 2019-11-18 13:52:11 -05:00 · 11c04b5e39
parent 61b0ebaf7b
commit 11c04b5e39
3 changed files with 78 additions and 54 deletions
--- 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'
--- 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>
--- 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 */