From 398ca11a8c03e7d76f20bf9def1538d8815cb616 Mon Sep 17 00:00:00 2001 From: Michael Brewer-Davis Date: Mon, 29 Jul 2019 23:15:16 -0400 Subject: [PATCH] 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 Reviewed-by: Frank Murphy III QA-Review: Brian Watson Product-Review: Michael Brewer-Davis --- app/controllers/outcome_results_controller.rb | 2 +- .../lib/account_reports/outcome_reports.rb | 3 +- .../lib/account_reports/report_helper.rb | 10 +- .../spec_canvas/report_helper_spec.rb | 26 +++++ lib/api/v1/outcome_results.rb | 8 +- lib/{csv_i18n_helpers.rb => csv_with_i18n.rb} | 32 +++++-- lib/gradebook_exporter.rb | 7 +- spec/apis/v1/outcome_results_api_spec.rb | 34 +++++-- spec/lib/csv_i18n_helpers_spec.rb | 87 ----------------- spec/lib/csv_with_i18n_spec.rb | 95 +++++++++++++++++++ 10 files changed, 187 insertions(+), 117 deletions(-) rename lib/{csv_i18n_helpers.rb => csv_with_i18n.rb} (68%) delete mode 100644 spec/lib/csv_i18n_helpers_spec.rb create mode 100644 spec/lib/csv_with_i18n_spec.rb diff --git a/app/controllers/outcome_results_controller.rb b/app/controllers/outcome_results_controller.rb index 43791919246..038eda74a10 100644 --- a/app/controllers/outcome_results_controller.rb +++ b/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" diff --git a/gems/plugins/account_reports/lib/account_reports/outcome_reports.rb b/gems/plugins/account_reports/lib/account_reports/outcome_reports.rb index e86601444d8..02a69723c07 100644 --- a/gems/plugins/account_reports/lib/account_reports/outcome_reports.rb +++ b/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| diff --git a/gems/plugins/account_reports/lib/account_reports/report_helper.rb b/gems/plugins/account_reports/lib/account_reports/report_helper.rb index 414f06ddb06..15b7cdd8aa4 100644 --- a/gems/plugins/account_reports/lib/account_reports/report_helper.rb +++ b/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 diff --git a/gems/plugins/account_reports/spec_canvas/report_helper_spec.rb b/gems/plugins/account_reports/spec_canvas/report_helper_spec.rb index e24ea2b0fbf..8ba4cbd6a93 100644 --- a/gems/plugins/account_reports/spec_canvas/report_helper_spec.rb +++ b/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 diff --git a/lib/api/v1/outcome_results.rb b/lib/api/v1/outcome_results.rb index 1da3cd700ed..1755abc1dff 100644 --- a/lib/api/v1/outcome_results.rb +++ b/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') diff --git a/lib/csv_i18n_helpers.rb b/lib/csv_with_i18n.rb similarity index 68% rename from lib/csv_i18n_helpers.rb rename to lib/csv_with_i18n.rb index 8a44eda94ef..e4fd889262f 100644 --- a/lib/csv_i18n_helpers.rb +++ b/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 . -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 diff --git a/lib/gradebook_exporter.rb b/lib/gradebook_exporter.rb index a87f975c865..feff6947728 100644 --- a/lib/gradebook_exporter.rb +++ b/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 diff --git a/spec/apis/v1/outcome_results_api_spec.rb b/spec/apis/v1/outcome_results_api_spec.rb index 1f77b1f239f..3c8d0be52ae 100644 --- a/spec/apis/v1/outcome_results_api_spec.rb +++ b/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 diff --git a/spec/lib/csv_i18n_helpers_spec.rb b/spec/lib/csv_i18n_helpers_spec.rb deleted file mode 100644 index c567182c612..00000000000 --- a/spec/lib/csv_i18n_helpers_spec.rb +++ /dev/null @@ -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 . -# - -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 diff --git a/spec/lib/csv_with_i18n_spec.rb b/spec/lib/csv_with_i18n_spec.rb new file mode 100644 index 00000000000..23780066ca7 --- /dev/null +++ b/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 . +# + +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