add i18n to outcome reports, lmgb export

closes OUT-3197

Test plan:
With permutations of the following feature flags
 - Account:
   Enable i18n features in outcomes exports
 - User:
   Include BOM in compatible exports
   Use semicolons in compatible exports
   Autodetect separators in compatible exports

Test that the following reports obey the
requested settings:
 - Account reports
   - Outcome Results
   - Student Competency
 - Learning Mastery Gradebook
   - Export report
   
See g/199468 for details on how reports should respond
to each setting

Change-Id: Iea942bdbc6d1efc560b0c4fc422c7c14a8fd90c4
Reviewed-on: https://gerrit.instructure.com/203101
Tested-by: Jenkins
Reviewed-by: Nathan Prabhu <nprabhu@instructure.com>
Reviewed-by: Frank Murphy III <fmurphy@instructure.com>
QA-Review: Brian Watson <bwatson@instructure.com>
Product-Review: Michael Brewer-Davis <mbd@instructure.com>

app/controllers/outcome_results_controller.rb

@@ -324,7 +324,7 @@ class OutcomeResultsController < ApplicationController
       format.csv do
         build_outcome_paths
         send_data(
-          outcome_results_rollups_csv(user_rollups, @outcomes, @outcome_paths),
+          outcome_results_rollups_csv(@current_user, @context, user_rollups, @outcomes, @outcome_paths),
           :type => "text/csv",
           :filename => t('outcomes_filename', "Outcomes").gsub(/ /, "_") + "-" + @context.name.to_s.gsub(/ /, "_") + ".csv",
           :disposition => "attachment"

gems/plugins/account_reports/lib/account_reports/outcome_reports.rb

@@ -296,8 +296,9 @@ module AccountReports
       header_keys = headers.keys
       header_names = headers.values
       host = root_account.domain
+      enable_i18n_features = @account_report.account.feature_enabled?(:enable_i18n_features_in_outcomes_exports)
 
-      write_report header_names do |csv|
+      write_report header_names, enable_i18n_features do |csv|
         total = scope.length
         Shackles.activate(:master) { AccountReport.where(id: @account_report.id).update_all(total_lines: total) }
         scope.each do |row|

gems/plugins/account_reports/lib/account_reports/report_helper.rb

@@ -20,7 +20,6 @@ require 'csv'
 
 module AccountReports::ReportHelper
   include ::Api
-  include CsvI18nHelpers
 
   def parse_utc_string(datetime)
     if datetime.is_a? String
@@ -269,12 +268,11 @@ module AccountReports::ReportHelper
 
   def generate_and_run_report(headers = nil, extension = 'csv', enable_i18n_features = false)
     file = AccountReports.generate_file(@account_report, extension)
-    options, bom = {}, ''
+    options = {}
     if enable_i18n_features
-      options, bom = csv_i18n_settings(@account_report.user)
+      options = CsvWithI18n.csv_i18n_settings(@account_report.user)
     end
-    ExtendedCSV.open(file, "w", options.slice(:col_sep, :encoding)) do |csv|
-      csv.to_io.write(bom)
+    ExtendedCSV.open(file, "w", options) do |csv|
       csv.instance_variable_set(:@account_report, @account_report)
       csv << headers unless headers.nil?
       Shackles.activate(:slave) { yield csv } if block_given?
@@ -433,7 +431,7 @@ module AccountReports::ReportHelper
     end
   end
 
-  class ExtendedCSV < CSV
+  class ExtendedCSV < CsvWithI18n
     def <<(row)
       if lineno % 1_000 == 0
         Shackles.activate(:master) do

gems/plugins/account_reports/spec_canvas/report_helper_spec.rb

@@ -144,6 +144,32 @@ describe "report helper" do
     end
   end
 
+  describe "#generate_and_run_report" do
+    context 'i18n' do
+      before do
+        account_report.save!
+        user.enable_feature!(:include_byte_order_mark_in_gradebook_exports)
+        user.enable_feature!(:use_semi_colon_field_separators_in_gradebook_exports)
+      end
+
+      it "Should not have byte order mark and semicolons when i18n compatibility disabled" do
+        file = report.generate_and_run_report(['h1', 'h2']) do |csv|
+          csv << ['val1', 'val2']
+        end
+        contents = File.read(file)
+        expect(contents).to eq "h1,h2\nval1,val2\n"
+      end
+
+      it "Should use i18n compatibility when enabled" do
+        file = report.generate_and_run_report(['h1', 'h2'], 'csv', true) do |csv|
+          csv << ['val1', 'val2']
+        end
+        contents = File.read(file)
+        expect(contents).to eq "\xEF\xBB\xBFh1;h2\nval1;val2\n"
+      end
+    end
+  end
+
   context 'Scopes' do
 
     before(:once) do

lib/api/v1/outcome_results.rb

@@ -212,8 +212,12 @@ module Api::V1::OutcomeResults
     }
   end
 
-  def outcome_results_rollups_csv(rollups, outcomes, outcome_paths)
-    CSV.generate do |csv|
+  def outcome_results_rollups_csv(current_user, context, rollups, outcomes, outcome_paths)
+    options = {}
+    if context.root_account.feature_enabled?(:enable_i18n_features_in_outcomes_exports)
+      options = CsvWithI18n.csv_i18n_settings(current_user)
+    end
+    CsvWithI18n.generate(options) do |csv|
       row = []
       row << I18n.t(:student_name, 'Student name')
       row << I18n.t(:student_id, 'Student ID')

lib/csv_with_i18n.rb

@@ -14,9 +14,25 @@
 #
 # You should have received a copy of the GNU Affero General Public License along
 # with this program. If not, see <http://www.gnu.org/licenses/>.
-module CsvI18nHelpers
+class CsvWithI18n < CSV
 
-  def csv_i18n_settings(user, options = {})
+  BYTE_ORDER_MARK = "\xEF\xBB\xBF".freeze
+
+  def initialize(data, **options)
+    @include_bom = options.delete(:include_bom)
+    super(data, options)
+    raise 'include_bom and write_headers cannot both be true' if self.write_headers? && @include_bom
+  end
+
+  def <<(row)
+    if @include_bom && !@bom_written
+      @io.write(BYTE_ORDER_MARK)
+      @bom_written = true
+    end
+    super(row)
+  end
+
+  def self.csv_i18n_settings(user, options = {})
     options[:col_sep] ||= determine_column_separator(user)
     options[:encoding] ||= I18n.t('csv.encoding', 'UTF-8')
 
@@ -24,20 +40,20 @@ module CsvI18nHelpers
     # Notepad treat the BOM as a required magic number rather than use heuristics. These tools add a BOM when saving
     # text as UTF-8, and cannot interpret UTF-8 unless the BOM is present or the file contains only ASCII.
     # https://en.wikipedia.org/wiki/Byte_order_mark#UTF-8
-    bom = include_bom?(user, options[:encoding]) ? "\xEF\xBB\xBF" : ''
+    options[:include_bom] = include_bom?(user, options[:encoding])
 
-    [options, bom]
+    options
   end
 
-  private
-
-  def include_bom?(user, encoding)
+  def self.include_bom?(user, encoding)
     encoding == 'UTF-8' && user.feature_enabled?(:include_byte_order_mark_in_gradebook_exports)
   end
+  private_class_method :include_bom?
 
-  def determine_column_separator(user)
+  def self.determine_column_separator(user)
     return ';' if user.feature_enabled?(:use_semi_colon_field_separators_in_gradebook_exports)
     return ',' unless user.feature_enabled?(:autodetect_field_separators_for_gradebook_exports)
     I18n.t('number.format.separator', '.') == ',' ? ';' : ','
   end
+  private_class_method :determine_column_separator
 end

lib/gradebook_exporter.rb

@@ -19,7 +19,6 @@
 class GradebookExporter
   include GradebookSettingsHelpers
   include LocaleSelection
-  include CsvI18nHelpers
 
   # You may see a pattern in this file of things that look like `<< nil << nil`
   # to create 'buffer' cells for columns. Let's try to stop using that pattern
@@ -45,8 +44,8 @@ class GradebookExporter
       root_account: @course.root_account
     )
 
-    @options, bom = csv_i18n_settings(@user, @options)
-    csv_data.prepend(bom)
+    @options = CsvWithI18n.csv_i18n_settings(@user, @options)
+    csv_data
   end
 
   private
@@ -104,7 +103,7 @@ class GradebookExporter
     should_show_totals = show_totals?
     include_sis_id = @options[:include_sis_id]
 
-    CSV.generate(@options.slice(:encoding, :col_sep)) do |csv|
+    CsvWithI18n.generate(@options.slice(:encoding, :col_sep, :include_bom)) do |csv|
       # First row
       header = ["Student", "ID"]
       header << "SIS User ID" if include_sis_id

spec/apis/v1/outcome_results_api_spec.rb

@@ -20,7 +20,6 @@ require File.expand_path(File.dirname(__FILE__) + '/../api_spec_helper')
 require File.expand_path(File.dirname(__FILE__) + '/../../sharding_spec_helper')
 
 describe "Outcome Results API", type: :request do
-
   let_once(:outcome_course) do
     course_factory(active_all: true)
     @course
@@ -257,13 +256,32 @@ describe "Outcome Results API", type: :request do
         end
       end
 
-      it "returns a csv file" do
-        outcome_result
-        user_session @user
-        get "/courses/#{@course.id}/outcome_rollups.csv"
-        expect(response).to be_successful
-        expect(response.body).to eq "Student name,Student ID,new outcome result,new outcome mastery points\n"+
-          "User,#{outcome_student.id},3.0,3.0\n"
+      context "csv" do
+        it "returns a csv file" do
+          outcome_result
+          user_session @user
+          get "/courses/#{@course.id}/outcome_rollups.csv"
+          expect(response).to be_successful
+          expect(response.body).to eq <<~END
+            Student name,Student ID,new outcome result,new outcome mastery points
+            User,#{outcome_student.id},3.0,3.0
+          END
+        end
+
+        it "obeys csv i18n flags" do
+          outcome_course.root_account.enable_feature! :enable_i18n_features_in_outcomes_exports
+          @user.enable_feature! :include_byte_order_mark_in_gradebook_exports
+          @user.enable_feature! :use_semi_colon_field_separators_in_gradebook_exports
+
+          outcome_result
+          user_session @user
+          get "/courses/#{@course.id}/outcome_rollups.csv"
+          expect(response).to be_successful
+          expect(response.body).to eq <<~END
+            \xEF\xBB\xBFStudent name;Student ID;new outcome result;new outcome mastery points
+            User;#{outcome_student.id};3.0;3.0
+          END
+        end
       end
 
       describe "user_ids parameter" do

spec/lib/csv_i18n_helpers_spec.rb

@@ -1,87 +0,0 @@
-
-#
-# Copyright (C) 2013 - present Instructure, Inc.
-#
-# This file is part of Canvas.
-#
-# Canvas is free software: you can redistribute it and/or modify it under
-# the terms of the GNU Affero General Public License as published by the Free
-# Software Foundation, version 3 of the License.
-#
-# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
-# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
-# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
-# details.
-#
-# You should have received a copy of the GNU Affero General Public License along
-# with this program. If not, see <http://www.gnu.org/licenses/>.
-#
-
-require_relative '../../gems/plugins/account_reports/spec_canvas/report_spec_helper'
-require 'csv'
-
-# These tests and a subset of tests in gradebook_exporter_spec collectively cover csv_i18n_settings
-describe CsvI18nHelpers do
-  include ReportSpecHelper
-
-  before(:once) do
-    @account = account_model
-    @account.enable_feature!(:enable_i18n_features_in_outcomes_exports)
-    @admin = account_admin_user(account: @account)
-  end
-
-  let(:report_options) {
-    {
-      parse_header: true,
-      account: @account,
-      order: 'skip',
-      header: true
-    }
-  }
-
-  let(:report) do
-    run_report(
-      'outcome_export_csv',
-      report_options
-    )
-  end
-
-  describe "byte_order mark" do
-    it "is included when the user has it enabled" do
-      @admin.enable_feature!(:include_byte_order_mark_in_gradebook_exports)
-      actual_headers = parse_report(report, report_options)[0].headers
-      expect(actual_headers[0].bytes).to eq("\xEF\xBB\xBFvendor_guid".bytes)
-    end
-
-    it "is excluded when the user has it disabled" do
-      @admin.disable_feature!(:include_byte_order_mark_in_gradebook_exports)
-      actual_headers = parse_report(report, report_options)[0].headers
-      expect(actual_headers[0].bytes).to eq("vendor_guid".bytes)
-    end
-  end
-
-  describe "column_separator" do
-    it "respects the semicolon feature flag" do
-      @admin.enable_feature!(:use_semi_colon_field_separators_in_gradebook_exports)
-      actual_headers = parse_report(report, report_options.merge('col_sep': ';'))[0].headers
-      expected_headers = ['vendor_guid', 'object_type', 'title']
-      expect(actual_headers[0..2]).to eq(expected_headers)
-    end
-
-    it "can automatically determine the column separator to use when asked to autodetect" do
-      @admin.enable_feature!(:autodetect_field_separators_for_gradebook_exports)
-      I18n.locale = :is
-      actual_headers = parse_report(report, report_options.merge('col_sep': ';'))[0].headers
-      expected_headers = ['vendor_guid', 'object_type', 'title']
-      expect(actual_headers[0..2]).to eq(expected_headers)
-    end
-
-    it "uses comma as the column separator when not asked to autodetect" do
-      @admin.disable_feature!(:autodetect_field_separators_for_gradebook_exports)
-      I18n.locale = :is
-      actual_headers = parse_report(report, report_options.merge('col_sep': ','))[0].headers
-      expected_headers = ['vendor_guid', 'object_type', 'title']
-      expect(actual_headers[0..2]).to eq(expected_headers)
-    end
-  end
-end

spec/lib/csv_with_i18n_spec.rb

@@ -0,0 +1,95 @@
+
+#
+# Copyright (C) 2013 - present Instructure, Inc.
+#
+# This file is part of Canvas.
+#
+# Canvas is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License as published by the Free
+# Software Foundation, version 3 of the License.
+#
+# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along
+# with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+require_relative '../../gems/plugins/account_reports/spec_canvas/report_spec_helper'
+require 'csv'
+
+# These tests and a subset of tests in gradebook_exporter_spec collectively cover csv_i18n_settings
+describe CsvWithI18n do
+  before(:once) do
+    @account = account_model
+    @admin = account_admin_user(account: @account)
+  end
+
+  describe "csv_i18n_settings" do
+    describe "byte_order mark" do
+      it "is included when the user has it enabled" do
+        @admin.enable_feature!(:include_byte_order_mark_in_gradebook_exports)
+        expect(CsvWithI18n.csv_i18n_settings(@admin)).to include(include_bom: true)
+      end
+
+      it "is excluded when the user has it disabled" do
+        @admin.disable_feature!(:include_byte_order_mark_in_gradebook_exports)
+        expect(CsvWithI18n.csv_i18n_settings(@admin)).to include(include_bom: false)
+      end
+    end
+
+    describe "column_separator" do
+      it "respects the semicolon feature flag" do
+        @admin.enable_feature!(:use_semi_colon_field_separators_in_gradebook_exports)
+        expect(CsvWithI18n.csv_i18n_settings(@admin)).to include(col_sep: ';')
+      end
+
+      it "can automatically determine the column separator to use when asked to autodetect" do
+        @admin.enable_feature!(:autodetect_field_separators_for_gradebook_exports)
+        I18n.locale = :is
+        expect(CsvWithI18n.csv_i18n_settings(@admin)).to include(col_sep: ';')
+      end
+
+      it "uses comma as the column separator when not asked to autodetect" do
+        @admin.disable_feature!(:autodetect_field_separators_for_gradebook_exports)
+        I18n.locale = :is
+       expect(CsvWithI18n.csv_i18n_settings(@admin)).to include(col_sep: ',')
+      end
+    end
+
+    it 'passes through other options' do
+      expect(CsvWithI18n.csv_i18n_settings(@admin, foo: 'bar')).to include(foo: 'bar')
+    end
+
+    it 'works alongside CsvWithI18n' do
+      @admin.enable_feature!(:use_semi_colon_field_separators_in_gradebook_exports)
+      @admin.enable_feature!(:include_byte_order_mark_in_gradebook_exports)
+      options = CsvWithI18n.csv_i18n_settings(@admin)
+      output = CsvWithI18n.generate(options) do |csv|
+        csv << [1, 2]
+        csv << [3, 4]
+      end
+      expect(output.bytes).to eq "\xEF\xBB\xBF1;2\n3;4\n".bytes
+    end
+  end
+
+  describe 'CsvWithI18n' do
+    it 'does not add a bom if not set as an option' do
+      output = CsvWithI18n.generate do |csv|
+        csv << [1, 2]
+        csv << [3, 4]
+      end
+      expect(output).to eq "1,2\n3,4\n"
+    end
+
+    it 'does add a bom if set as an option' do
+      output = CsvWithI18n.generate(include_bom: true) do |csv|
+        csv << [1, 2]
+        csv << [3, 4]
+      end
+      expect(output.bytes).to eq "\xEF\xBB\xBF1,2\n3,4\n".bytes
+    end
+  end
+end