Include student first/last name in gradebook export

Gradebook csv export should work for both student name or
separate first and last names

closes EVAL-1989
flag=gradebook_show_first_last_names

Test plan:
With the siteadmin FF ON, and the account admin setting to allow
showing student first/last names enabled, as a teacher:

1. Select "Split Student Names" option from the view options menu
2. Verify the gradebook grid shows separate columns for student
last and first  names
3. From the gradebook actions menu, select export
4. Verify the exported csv has values for student last and first
names (in this order, comma separated) and rest of the values are
correct.
5. Unselect the "Split Student Names" option
6. Verify the grid has 1 column for student name
7. Export the csv, and verify the csv has 1 column for student
name and rest of the values are correct

Change-Id: Ia38e9d47333414e3fdac67eddc820547126ea345
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/276073
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
QA-Review: Kai Bjorkman <kbjorkman@instructure.com>
Product-Review: Syed Hussain <shussain@instructure.com>
Reviewed-by: Adrian Packel <apackel@instructure.com>
Reviewed-by: Aaron Shafovaloff <ashafovaloff@instructure.com>

app/controllers/gradebook_csvs_controller.rb

@@ -37,6 +37,12 @@ class GradebookCsvsController < ApplicationController
         csv_options[:assignment_order] = params[:assignment_order].map(&:to_i)
       end
 
+      if @context.root_account.allow_gradebook_show_first_last_names? &&
+         Account.site_admin.feature_enabled?(:gradebook_show_first_last_names) &&
+         params[:show_student_first_last_name]
+        csv_options[:show_student_first_last_name] = Canvas::Plugin.value_to_boolean(params[:show_student_first_last_name])
+      end
+
       attachment_progress = @context.gradebook_to_csv_in_background(filename, @current_user, csv_options)
       render json: attachment_progress, status: :ok
     end

lib/gradebook_exporter.rb

@@ -121,7 +121,8 @@ class GradebookExporter
 
     CSVWithI18n.generate(**@options.slice(:encoding, :col_sep, :include_bom)) do |csv|
       # First row
-      header = ["Student", "ID"]
+      header = @options[:show_student_first_last_name] ? ["LastName", "FirstName"] : ["Student"]
+      header << "ID"
       header << "SIS User ID" if include_sis_id
       header << "SIS Login ID"
       header << "Integration ID" if include_sis_id && show_integration_id?
@@ -156,6 +157,7 @@ class GradebookExporter
       # Possible "hidden" (muted or manual posting) row
       if assignments.any? { |assignment| show_as_hidden?(assignment) }
         row = [nil, nil, nil, nil]
+        row << nil if @options[:show_student_first_last_name]
         if include_sis_id
           row << nil
           row << nil if show_integration_id?
@@ -190,6 +192,7 @@ class GradebookExporter
 
       # Second Row
       row = ["    Points Possible", nil, nil, nil]
+      row << nil if @options[:show_student_first_last_name]
       if include_sis_id
         row << nil
         row << nil if show_integration_id?
@@ -260,7 +263,12 @@ class GradebookExporter
               "N/A"
             end
           end
-          row = [student_name(student), student.id]
+          row = if @options[:show_student_first_last_name]
+                  [student_name(student.last_name), student_name(student.first_name)]
+                else
+                  [student_name(student.sortable_name)]
+                end
+          row << student.id
           pseudonym = SisPseudonym.for(student, student_enrollment, type: :implicit, require_sis: false, root_account: @course.root_account)
           row << pseudonym&.sis_user_id if include_sis_id
           row << pseudonym&.unique_id
@@ -393,9 +401,8 @@ class GradebookExporter
   # Returns the student name to use for the export.  If the name
   # starts with =, quote it so anyone pulling the data into Excel
   # doesn't have a formula execute.
-  def student_name(student)
-    name = student.sortable_name
-    name = "=\"#{name}\"" if name =~ STARTS_WITH_EQUAL
+  def student_name(name)
+    name = "=\"#{name}\"" if name.match?(STARTS_WITH_EQUAL)
     name
   end
 

spec/javascripts/jsx/gradebook/shared/GradebookExportManagerSpec.js

@@ -88,17 +88,17 @@ QUnit.module('GradebookExportManager - monitoringUrl', {
   }
 })
 
-test('returns an appropriate url if all relevant pieces are present', function() {
+test('returns an appropriate url if all relevant pieces are present', function () {
   equal(this.subject.monitoringUrl(), `${monitoringBase}/progressId`)
 })
 
-test('returns undefined if export is missing', function() {
+test('returns undefined if export is missing', function () {
   this.subject.export = undefined
 
   equal(this.subject.monitoringUrl(), undefined)
 })
 
-test('returns undefined if progressId is missing', function() {
+test('returns undefined if progressId is missing', function () {
   this.subject.export.progressId = undefined
 
   equal(this.subject.monitoringUrl(), undefined)
@@ -118,17 +118,17 @@ QUnit.module('GradebookExportManager - attachmentUrl', {
   }
 })
 
-test('returns an appropriate url if all relevant pieces are present', function() {
+test('returns an appropriate url if all relevant pieces are present', function () {
   equal(this.subject.attachmentUrl(), `${attachmentBase}/attachmentId`)
 })
 
-test('returns undefined if export is missing', function() {
+test('returns undefined if export is missing', function () {
   this.subject.export = undefined
 
   equal(this.subject.attachmentUrl(), undefined)
 })
 
-test('returns undefined if attachmentId is missing', function() {
+test('returns undefined if attachmentId is missing', function () {
   this.subject.export.attachmentId = undefined
 
   equal(this.subject.attachmentUrl(), undefined)
@@ -158,6 +158,32 @@ QUnit.module('GradebookExportManager - startExport', {
   }
 })
 
+test('sets show_student_first_last_name setting if requested', function () {
+  this.subject = new GradebookExportManager(exportingUrl, currentUserId)
+  this.subject.monitorExport = (resolve, _reject) => {
+    resolve('success')
+  }
+
+  const getAssignmentOrder = () => []
+  return this.subject.startExport(undefined, getAssignmentOrder, true).then(() => {
+    const postData = JSON.parse(moxios.requests.mostRecent().config.data)
+    propEqual(postData.show_student_first_last_name, true)
+  })
+})
+
+test('does not set show_student_first_last_name setting by default', function () {
+  this.subject = new GradebookExportManager(exportingUrl, currentUserId)
+  this.subject.monitorExport = (resolve, _reject) => {
+    resolve('success')
+  }
+
+  const getAssignmentOrder = () => []
+  return this.subject.startExport(undefined, getAssignmentOrder).then(() => {
+    const postData = JSON.parse(moxios.requests.mostRecent().config.data)
+    propEqual(postData.show_student_first_last_name, false)
+  })
+})
+
 test('includes assignment_order if getAssignmentOrder returns some assignments', function () {
   this.subject = new GradebookExportManager(exportingUrl, currentUserId)
   this.subject.monitorExport = (resolve, _reject) => {
@@ -184,7 +210,7 @@ test('does not include assignment_order if getAssignmentOrder returns no assignm
   })
 })
 
-test('returns a rejected promise if the manager has no exportingUrl set', function() {
+test('returns a rejected promise if the manager has no exportingUrl set', function () {
   this.subject = new GradebookExportManager(exportingUrl, currentUserId)
   this.subject.exportingUrl = undefined
 
@@ -195,7 +221,7 @@ test('returns a rejected promise if the manager has no exportingUrl set', functi
     })
 })
 
-test('returns a rejected promise if the manager already has an export going', function() {
+test('returns a rejected promise if the manager already has an export going', function () {
   this.subject = new GradebookExportManager(exportingUrl, currentUserId, workingExport)
 
   return this.subject
@@ -205,7 +231,7 @@ test('returns a rejected promise if the manager already has an export going', fu
     })
 })
 
-test('sets a new existing export and returns a fulfilled promise', function() {
+test('sets a new existing export and returns a fulfilled promise', function () {
   const expectedExport = {
     progressId: 'newProgressId',
     attachmentId: 'newAttachmentId'
@@ -223,7 +249,7 @@ test('sets a new existing export and returns a fulfilled promise', function() {
     })
 })
 
-test('clears any new export and returns a rejected promise if no monitoring is possible', function() {
+test('clears any new export and returns a rejected promise if no monitoring is possible', function () {
   sandbox.stub(GradebookExportManager.prototype, 'monitoringUrl').returns(undefined)
   this.subject = new GradebookExportManager(exportingUrl, currentUserId)
 
@@ -235,7 +261,7 @@ test('clears any new export and returns a rejected promise if no monitoring is p
     })
 })
 
-test('starts polling for progress and returns a rejected promise on progress failure', function() {
+test('starts polling for progress and returns a rejected promise on progress failure', function () {
   const expectedMonitoringUrl = `${monitoringBase}/newProgressId`
 
   this.subject = new GradebookExportManager(exportingUrl, currentUserId, null, 1)
@@ -255,7 +281,7 @@ test('starts polling for progress and returns a rejected promise on progress fai
     })
 })
 
-test('starts polling for progress and returns a rejected promise on unknown progress status', function() {
+test('starts polling for progress and returns a rejected promise on unknown progress status', function () {
   const expectedMonitoringUrl = `${monitoringBase}/newProgressId`
 
   this.subject = new GradebookExportManager(exportingUrl, currentUserId, null, 1)
@@ -275,7 +301,7 @@ test('starts polling for progress and returns a rejected promise on unknown prog
     })
 })
 
-test('starts polling for progress and returns a fulfilled promise on progress completion', function() {
+test('starts polling for progress and returns a fulfilled promise on progress completion', function () {
   const expectedMonitoringUrl = `${monitoringBase}/newProgressId`
   const expectedAttachmentUrl = `${attachmentBase}/newAttachmentId`
 

spec/lib/gradebook_exporter_spec.rb

@@ -173,6 +173,41 @@ describe GradebookExporter do
       end
     end
 
+    describe "separate columns for student last and first names" do
+      subject(:csv) { exporter(@exporter_options).to_csv }
+
+      before(:once) do
+        @exporter_options = {}
+        @current_assignment = @course.assignments.create! due_at: 1.week.from_now,
+                                                          title: "current",
+                                                          points_possible: 10
+        student_in_course active_all: true
+        @current_assignment.grade_student @student, grade: 3, grader: @teacher
+        @rows = CSV.parse(csv)
+      end
+
+      it "is a csv with three rows" do
+        expect(@rows.count).to be 3
+      end
+
+      it "is a csv with rows of equal length" do
+        expect(@rows.first.length).to eq @rows.second.length
+      end
+
+      it "shows student first and last names in headers" do
+        @exporter_options[:show_student_first_last_name] = true
+        expect(CSV.parse(csv, headers: true).headers).to include("LastName", "FirstName")
+        expect(CSV.parse(csv, headers: true).headers).not_to include("Student")
+      end
+
+      it "shows student first and last name in rows" do
+        @exporter_options[:show_student_first_last_name] = true
+        rows = CSV.parse(csv)
+        expect(rows[2][0]).to eq(@student.last_name)
+        expect(rows[2][1]).to eq(@student.first_name)
+      end
+    end
+
     describe "default output with blank course" do
       before(:once) do
         @course.custom_gradebook_columns.create! title: "Custom Column 1"

ui/features/gradebook/react/default_gradebook/Gradebook.js

@@ -1977,6 +1977,7 @@ class Gradebook extends React.Component {
       gradebookIsEditable: this.options.gradebook_is_editable,
       contextAllowsGradebookUploads: this.options.context_allows_gradebook_uploads,
       gradebookImportUrl: this.options.gradebook_import_url,
+      showStudentFirstLastName: this.gridDisplaySettings.showSeparateFirstLastNames,
       currentUserId: this.options.currentUserId,
       gradebookExportUrl: this.options.export_gradebook_csv_url,
       postGradesLtis: this.postGradesLtis,
@@ -3676,7 +3677,7 @@ class Gradebook extends React.Component {
     const toggleableAction = () => {
       this.gridDisplaySettings.showSeparateFirstLastNames =
         !this.gridDisplaySettings.showSeparateFirstLastNames
-      return this.updateColumnsAndRenderViewOptionsMenu()
+      return this.updateColumnsAndRenderViewOptionsMenu() && this.renderActionMenu()
     }
     toggleableAction()
     return this.saveSettings(

ui/features/gradebook/react/default_gradebook/components/ActionMenu.js

@@ -47,6 +47,7 @@ class ActionMenu extends React.Component {
     contextAllowsGradebookUploads: bool.isRequired,
     getAssignmentOrder: func.isRequired,
     gradebookImportUrl: string.isRequired,
+    showStudentFirstLastName: bool.isRequired,
 
     currentUserId: string.isRequired,
     gradebookExportUrl: string.isRequired,
@@ -133,7 +134,11 @@ class ActionMenu extends React.Component {
     $.flashMessage(I18n.t('Gradebook export started'))
 
     return this.exportManager
-      .startExport(this.props.gradingPeriodId, this.props.getAssignmentOrder)
+      .startExport(
+        this.props.gradingPeriodId,
+        this.props.getAssignmentOrder,
+        this.props.showStudentFirstLastName
+      )
       .then(resolution => {
         this.setExportInProgress(false)
 

ui/features/gradebook/react/shared/GradebookExportManager.js

@@ -117,7 +117,7 @@ class GradebookExportManager {
     }, this.pollingInterval)
   }
 
-  startExport(gradingPeriodId, getAssignmentOrder) {
+  startExport(gradingPeriodId, getAssignmentOrder, showStudentFirstLastName = false) {
     if (!this.exportingUrl) {
       return Promise.reject(I18n.t('No way to export gradebooks provided!'))
     }
@@ -136,6 +136,8 @@ class GradebookExportManager {
       params.assignment_order = assignmentOrder
     }
 
+    params.show_student_first_last_name = showStudentFirstLastName
+
     return axios.post(this.exportingUrl, params).then(response => {
       this.export = {
         progressId: response.data.progress_id,