From c1b13f9d6c59841c6f42115d6166332a09b76925 Mon Sep 17 00:00:00 2001 From: Harsohail Brar Date: Sun, 17 Apr 2022 12:14:52 -0600 Subject: [PATCH 01/23] add sms opt in modal to volunteer import page --- app/views/imports/_sms_opt_in.html.erb | 26 ++++++++++++++++++++++++++ app/views/imports/_volunteers.html.erb | 14 ++++++++++---- config/locales/views.en.yml | 5 +++++ 3 files changed, 41 insertions(+), 4 deletions(-) create mode 100644 app/views/imports/_sms_opt_in.html.erb diff --git a/app/views/imports/_sms_opt_in.html.erb b/app/views/imports/_sms_opt_in.html.erb new file mode 100644 index 0000000000..55c0970a9f --- /dev/null +++ b/app/views/imports/_sms_opt_in.html.erb @@ -0,0 +1,26 @@ + \ No newline at end of file diff --git a/app/views/imports/_volunteers.html.erb b/app/views/imports/_volunteers.html.erb index 5ecaf9973a..306836cfc7 100644 --- a/app/views/imports/_volunteers.html.erb +++ b/app/views/imports/_volunteers.html.erb @@ -22,10 +22,16 @@ <%= f.file_field :file, id: 'volunteer-file', accept: 'text/csv', class: 'form-control-file', style: "margin: auto;" %> - <%= button_tag id: "volunteer-import-button", class: "btn btn-md btn-success pull-right", - disabled: true, data: {disable_with: " #{t(".disable_with")}"} do %> - <%= t(".button.import") %> - <% end %> + <%# <%= button_tag id: "volunteer-import-button", class: "btn btn-md btn-success pull-right", %> + <%# disabled: true, data: {disable_with: " #{t(".disable_with")}"} do %> %> + <%# <%= t(".button.import") %> %> + <%# <% end %> %> + + <%= render 'sms_opt_in', form: f %> + \ No newline at end of file diff --git a/app/views/imports/_volunteers.html.erb b/app/views/imports/_volunteers.html.erb index 306836cfc7..ef8e453ae1 100644 --- a/app/views/imports/_volunteers.html.erb +++ b/app/views/imports/_volunteers.html.erb @@ -22,10 +22,6 @@ <%= f.file_field :file, id: 'volunteer-file', accept: 'text/csv', class: 'form-control-file', style: "margin: auto;" %> - <%# <%= button_tag id: "volunteer-import-button", class: "btn btn-md btn-success pull-right", %> - <%# disabled: true, data: {disable_with: " #{t(".disable_with")}"} do %> %> - <%# <%= t(".button.import") %> %> - <%# <% end %> %> <%= render 'sms_opt_in', form: f %> + <%= form.submit t(".continue"), { id: "sms-opt-in-continue-button", class: "btn btn-primary", disabled: true } %> diff --git a/app/views/imports/_volunteers.html.erb b/app/views/imports/_volunteers.html.erb index ef8e453ae1..df8208ded1 100644 --- a/app/views/imports/_volunteers.html.erb +++ b/app/views/imports/_volunteers.html.erb @@ -24,10 +24,10 @@ <%= render 'sms_opt_in', form: f %> - + <%= button_tag type: "button", id: "volunteer-import-button", class: "btn btn-md btn-success pull-right", + disabled: true, data: { toggle: "modal", target: "#smsOptIn", disable_with: " #{t(".disable_with")}"} do %> + <%= t(".button.import") %> + <% end %> - \ No newline at end of file + diff --git a/app/views/imports/_supervisors.html.erb b/app/views/imports/_supervisors.html.erb index 40ebb3fc34..646d64d2a8 100644 --- a/app/views/imports/_supervisors.html.erb +++ b/app/views/imports/_supervisors.html.erb @@ -33,7 +33,6 @@ <%= t(".button.import") %> <% end %> - <% end %> diff --git a/app/views/imports/_volunteers.html.erb b/app/views/imports/_volunteers.html.erb index 1bed4ab62a..fb5139e416 100644 --- a/app/views/imports/_volunteers.html.erb +++ b/app/views/imports/_volunteers.html.erb @@ -13,7 +13,7 @@ 2. <%= t(".upload_title") %> - <%= form_with(url: imports_path, local: :true) do |f| %> + <%= form_with(url: imports_path, local: :true, id: "volunteer-import-form") do |f| %> <%= f.hidden_field :import_type, value: "volunteer" %> <%= f.hidden_field :sms_opt_in, value: false %> @@ -30,10 +30,4 @@ disabled: true, data: { disable_with: " #{t(".disable_with")}"} do %> <%= t(".button.import") %> <% end %> - - <% end %> From d8c2da5adaba6eb48f1770e0f25c277d6afeede1 Mon Sep 17 00:00:00 2001 From: Harsohail Brar Date: Fri, 22 Apr 2022 00:13:41 -0600 Subject: [PATCH 09/23] delete local storage temp csv file upon successful import --- app/controllers/imports_controller.rb | 4 +++- app/javascript/src/import.js | 11 +++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/app/controllers/imports_controller.rb b/app/controllers/imports_controller.rb index e68d9cbf53..ff1a589349 100644 --- a/app/controllers/imports_controller.rb +++ b/app/controllers/imports_controller.rb @@ -115,6 +115,8 @@ def validate_file(file, import_type) end def requires_sms_opt_in(file, import_type, sms_opt_in) + puts "IMPORT CONTAINS" + puts import_contains_phone_numbers(file) if (import_type == "volunteer" || import_type == "supervisor") && import_contains_phone_numbers(file) return sms_opt_in != "1" end @@ -124,7 +126,7 @@ def requires_sms_opt_in(file, import_type, sms_opt_in) def import_contains_phone_numbers(file) File.readlines(file).each_with_index do |line, index| - if index == 0 + if index == 0 || line.split(",").length <= 2 next end diff --git a/app/javascript/src/import.js b/app/javascript/src/import.js index 70d89638f0..b1bc3ec290 100644 --- a/app/javascript/src/import.js +++ b/app/javascript/src/import.js @@ -38,8 +38,7 @@ function fetchCSVFile(key) { function deleteCSVFile(key, submitEvent) { const formData = new FormData(submitEvent.target); const formProps = Object.fromEntries(formData); - console.log(formProps.sms_opt_in); - if (formProps.sms_opt_in) { + if (formProps.sms_opt_in === "1") { delete localStorage[key]; } } @@ -64,9 +63,17 @@ $("document").ready(() => { storeCSVFile(file, "volunteer-file"); }); + document.getElementById("volunteer-import-form").addEventListener("submit", function (event) { + deleteCSVFile("volunteer-file", event); + }); + document.getElementById("supervisor-file").addEventListener("change", function (event) { document.getElementById("supervisor-import-button").disabled = event.target.value == ""; const file = document.getElementById("supervisor-file").files[0]; storeCSVFile(file, "supervisor-file"); }); + + document.getElementById("supervisor-import-form").addEventListener("submit", function (event) { + deleteCSVFile("supervisor-file", event); + }); }); From a7d5a909353cb237b78cbec1c8d789b82ac296b7 Mon Sep 17 00:00:00 2001 From: Harsohail Brar Date: Fri, 22 Apr 2022 12:23:38 -0600 Subject: [PATCH 10/23] improve checking of phone number existence in import csv --- app/controllers/imports_controller.rb | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/app/controllers/imports_controller.rb b/app/controllers/imports_controller.rb index ff1a589349..288998eff8 100644 --- a/app/controllers/imports_controller.rb +++ b/app/controllers/imports_controller.rb @@ -1,4 +1,6 @@ class ImportsController < ApplicationController + require "csv" + include ActionView::Helpers::UrlHelper after_action :verify_authorized @@ -115,8 +117,6 @@ def validate_file(file, import_type) end def requires_sms_opt_in(file, import_type, sms_opt_in) - puts "IMPORT CONTAINS" - puts import_contains_phone_numbers(file) if (import_type == "volunteer" || import_type == "supervisor") && import_contains_phone_numbers(file) return sms_opt_in != "1" end @@ -125,17 +125,13 @@ def requires_sms_opt_in(file, import_type, sms_opt_in) end def import_contains_phone_numbers(file) - File.readlines(file).each_with_index do |line, index| - if index == 0 || line.split(",").length <= 2 - next - end - - phone_number = line.split(",").last + CSV.foreach(file, headers: true, header_converters: :symbol) do |row| + phone_number = row[:phone_number] if !phone_number.nil? && !phone_number.strip.empty? return true end end - + false end end From f8bf146e1cdebc1c54ef03517924e5e4aec32633 Mon Sep 17 00:00:00 2001 From: Harsohail Brar Date: Fri, 22 Apr 2022 12:35:17 -0600 Subject: [PATCH 11/23] delete local storage temp file when opt in message not present --- app/javascript/src/import.js | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/app/javascript/src/import.js b/app/javascript/src/import.js index b1bc3ec290..d86d101dd2 100644 --- a/app/javascript/src/import.js +++ b/app/javascript/src/import.js @@ -35,14 +35,6 @@ function fetchCSVFile(key) { return file; } -function deleteCSVFile(key, submitEvent) { - const formData = new FormData(submitEvent.target); - const formProps = Object.fromEntries(formData); - if (formProps.sms_opt_in === "1") { - delete localStorage[key]; - } -} - function populateFileInput(inputId) { const csvInput = document.getElementById(inputId); if (csvInput.files.length === 0 && localStorage[inputId]) { @@ -54,26 +46,23 @@ function populateFileInput(inputId) { } $("document").ready(() => { - populateFileInput("volunteer-file"); - populateFileInput("supervisor-file"); - document.getElementById("volunteer-file").addEventListener("change", function (event) { document.getElementById("volunteer-import-button").disabled = event.target.value == ""; const file = document.getElementById("volunteer-file").files[0]; storeCSVFile(file, "volunteer-file"); }); - document.getElementById("volunteer-import-form").addEventListener("submit", function (event) { - deleteCSVFile("volunteer-file", event); - }); - document.getElementById("supervisor-file").addEventListener("change", function (event) { document.getElementById("supervisor-import-button").disabled = event.target.value == ""; const file = document.getElementById("supervisor-file").files[0]; storeCSVFile(file, "supervisor-file"); }); - document.getElementById("supervisor-import-form").addEventListener("submit", function (event) { - deleteCSVFile("supervisor-file", event); - }); + if (document.getElementById("smsOptIn") === null) { + delete localStorage["volunteer-file"]; + delete localStorage["supervisor-file"]; + } else { + populateFileInput("volunteer-file"); + populateFileInput("supervisor-file"); + } }); From b1d4f8e21058de559819431709bf35b0fc162bae Mon Sep 17 00:00:00 2001 From: Harsohail Brar Date: Fri, 22 Apr 2022 14:23:29 -0600 Subject: [PATCH 12/23] fix continue import button for supervisor import --- app/controllers/imports_controller.rb | 6 +++--- app/views/imports/_supervisors.html.erb | 2 +- app/views/imports/_volunteers.html.erb | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/controllers/imports_controller.rb b/app/controllers/imports_controller.rb index 288998eff8..704ffd422e 100644 --- a/app/controllers/imports_controller.rb +++ b/app/controllers/imports_controller.rb @@ -28,7 +28,7 @@ def create if import[:type] == :error session[:import_error] = message elsif import[:type] == :sms_opt_in_error - session[:sms_opt_in_error] = true + session[:sms_opt_in_error] = import[:import_type] # Only use flash for success messages. Otherwise may cause CookieOverflow else flash[:success] = message @@ -72,7 +72,7 @@ def import_from_csv(import_type, sms_opt_in, file, org_id) return validated_file unless validated_file.nil? if requires_sms_opt_in(file, import_type, sms_opt_in) - return {type: :sms_opt_in_error} + return {type: :sms_opt_in_error, import_type: import_type} end case import_type @@ -131,7 +131,7 @@ def import_contains_phone_numbers(file) return true end end - + false end end diff --git a/app/views/imports/_supervisors.html.erb b/app/views/imports/_supervisors.html.erb index 3e4b4c0b3b..143198b35c 100644 --- a/app/views/imports/_supervisors.html.erb +++ b/app/views/imports/_supervisors.html.erb @@ -27,7 +27,7 @@ style: "margin: auto;" %> - <%= render "sms_opt_in_modal", { form: f } if @sms_opt_in_error %> + <%= render "sms_opt_in_modal", { form: f } if @sms_opt_in_error == "supervisor" %> <%= button_tag id: "supervisor-import-button", class: "btn btn-md btn-success pull-right", disabled: true, data: { disable_with: " #{t(".disable_with")}"} do %> <%= t(".button.import") %> diff --git a/app/views/imports/_volunteers.html.erb b/app/views/imports/_volunteers.html.erb index fb5139e416..1ff92bd8ec 100644 --- a/app/views/imports/_volunteers.html.erb +++ b/app/views/imports/_volunteers.html.erb @@ -25,7 +25,7 @@ <%= f.file_field :file, id: 'volunteer-file', accept: 'text/csv', class: 'form-control-file', style: "margin: auto;" %> - <%= render "sms_opt_in_modal", { form: f } if @sms_opt_in_error %> + <%= render "sms_opt_in_modal", { form: f } if @sms_opt_in_error == "volunteer" %> <%= button_tag id: "volunteer-import-button", class: "btn btn-md btn-success pull-right", disabled: true, data: { disable_with: " #{t(".disable_with")}"} do %> <%= t(".button.import") %> From 8b1a54ce2dfccb18c136abe840ea5056f425323a Mon Sep 17 00:00:00 2001 From: Harsohail Brar Date: Fri, 22 Apr 2022 14:33:33 -0600 Subject: [PATCH 13/23] toggle sms notifications when opt in checked --- app/lib/importers/supervisor_importer.rb | 1 + app/lib/importers/volunteer_importer.rb | 1 + 2 files changed, 2 insertions(+) diff --git a/app/lib/importers/supervisor_importer.rb b/app/lib/importers/supervisor_importer.rb index 9d269e84e0..8b9b7ef4fd 100644 --- a/app/lib/importers/supervisor_importer.rb +++ b/app/lib/importers/supervisor_importer.rb @@ -18,6 +18,7 @@ def import_supervisors end supervisor_params[:phone_number] = supervisor_params.key?(:phone_number) ? "+#{supervisor_params[:phone_number]}" : "" + supervisor_params[:receive_sms_notifications] = !supervisor_params[:phone_number].empty? supervisor = Supervisor.find_by(email: supervisor_params[:email]) volunteer_assignment_list = email_addresses_to_users(Volunteer, String(row[:supervisor_volunteers])) diff --git a/app/lib/importers/volunteer_importer.rb b/app/lib/importers/volunteer_importer.rb index 4485c3730c..c805498137 100644 --- a/app/lib/importers/volunteer_importer.rb +++ b/app/lib/importers/volunteer_importer.rb @@ -18,6 +18,7 @@ def import_volunteers end volunteer_params[:phone_number] = volunteer_params.key?(:phone_number) ? "+#{volunteer_params[:phone_number]}" : "" + volunteer_params[:receive_sms_notifications] = !volunteer_params[:phone_number].empty? volunteer = Volunteer.find_by(email: volunteer_params[:email]) From ff2a8041d129a9b8a1d8af9b50b663952347f482 Mon Sep 17 00:00:00 2001 From: Harsohail Brar Date: Fri, 22 Apr 2022 14:38:52 -0600 Subject: [PATCH 14/23] fix linting issues --- app/javascript/src/import.js | 88 ++++++++++++++++++------------------ 1 file changed, 44 insertions(+), 44 deletions(-) diff --git a/app/javascript/src/import.js b/app/javascript/src/import.js index d86d101dd2..10980ce0e0 100644 --- a/app/javascript/src/import.js +++ b/app/javascript/src/import.js @@ -1,68 +1,68 @@ -function dataURItoBlob(dataURI) { +function dataURItoBlob (dataURI) { // convert base64 to raw binary data held in a string - var byteString = atob(dataURI.split(",")[1]); + const byteString = atob(dataURI.split(',')[1]) // separate out the mime component - var mimeString = dataURI.split(",")[0].split(":")[1].split(";")[0]; + const mimeString = dataURI.split(',')[0].split(':')[1].split(';')[0] // write the bytes of the string to an ArrayBuffer - var arrayBuffer = new ArrayBuffer(byteString.length); - var _ia = new Uint8Array(arrayBuffer); - for (var i = 0; i < byteString.length; i++) { - _ia[i] = byteString.charCodeAt(i); + const arrayBuffer = new ArrayBuffer(byteString.length) + const _ia = new Uint8Array(arrayBuffer) + for (let i = 0; i < byteString.length; i++) { + _ia[i] = byteString.charCodeAt(i) } - var dataView = new DataView(arrayBuffer); - var blob = new Blob([dataView], { type: mimeString }); - return blob; + const dataView = new DataView(arrayBuffer) + const blob = new Blob([dataView], { type: mimeString }) + return blob } -function storeCSVFile(file, key) { - var reader = new FileReader(); +function storeCSVFile (file, key) { + const reader = new FileReader() reader.onload = (fileEvent) => { localStorage[key] = JSON.stringify({ name: file.name, - data: fileEvent.target.result, - }); - }; - reader.readAsDataURL(file); + data: fileEvent.target.result + }) + } + reader.readAsDataURL(file) } -function fetchCSVFile(key) { - const storedFileData = JSON.parse(localStorage[key]); - var fileContent = dataURItoBlob(storedFileData.data); - var file = new File([fileContent], storedFileData.name, { type: "text/csv" }); - return file; +function fetchCSVFile (key) { + const storedFileData = JSON.parse(localStorage[key]) + const fileContent = dataURItoBlob(storedFileData.data) + const file = new File([fileContent], storedFileData.name, { type: 'text/csv' }) + return file } -function populateFileInput(inputId) { - const csvInput = document.getElementById(inputId); +function populateFileInput (inputId) { + const csvInput = document.getElementById(inputId) if (csvInput.files.length === 0 && localStorage[inputId]) { - const file = fetchCSVFile(inputId); - const container = new DataTransfer(); - container.items.add(file); - csvInput.files = container.files; + const file = fetchCSVFile(inputId) + const container = new DataTransfer() + container.items.add(file) + csvInput.files = container.files } } -$("document").ready(() => { - document.getElementById("volunteer-file").addEventListener("change", function (event) { - document.getElementById("volunteer-import-button").disabled = event.target.value == ""; - const file = document.getElementById("volunteer-file").files[0]; - storeCSVFile(file, "volunteer-file"); - }); +$('document').ready(() => { + document.getElementById('volunteer-file').addEventListener('change', function (event) { + document.getElementById('volunteer-import-button').disabled = event.target.value == '' + const file = document.getElementById('volunteer-file').files[0] + storeCSVFile(file, 'volunteer-file') + }) - document.getElementById("supervisor-file").addEventListener("change", function (event) { - document.getElementById("supervisor-import-button").disabled = event.target.value == ""; - const file = document.getElementById("supervisor-file").files[0]; - storeCSVFile(file, "supervisor-file"); - }); + document.getElementById('supervisor-file').addEventListener('change', function (event) { + document.getElementById('supervisor-import-button').disabled = event.target.value == '' + const file = document.getElementById('supervisor-file').files[0] + storeCSVFile(file, 'supervisor-file') + }) - if (document.getElementById("smsOptIn") === null) { - delete localStorage["volunteer-file"]; - delete localStorage["supervisor-file"]; + if (document.getElementById('smsOptIn') === null) { + delete localStorage['volunteer-file'] + delete localStorage['supervisor-file'] } else { - populateFileInput("volunteer-file"); - populateFileInput("supervisor-file"); + populateFileInput('volunteer-file') + populateFileInput('supervisor-file') } -}); +}) From ec6c974045ebe9df801a3a14de03b39a1304184d Mon Sep 17 00:00:00 2001 From: Harsohail Brar Date: Fri, 22 Apr 2022 14:45:52 -0600 Subject: [PATCH 15/23] add undefined variables to js global scope --- app/javascript/src/import.js | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/app/javascript/src/import.js b/app/javascript/src/import.js index 10980ce0e0..23236dd779 100644 --- a/app/javascript/src/import.js +++ b/app/javascript/src/import.js @@ -1,3 +1,10 @@ +/* global atob */ +/* global Blob */ +/* global FileReader */ +/* global localStorage */ +/* global File */ +/* global DataTransfer */ + function dataURItoBlob (dataURI) { // convert base64 to raw binary data held in a string const byteString = atob(dataURI.split(',')[1]) @@ -47,13 +54,13 @@ function populateFileInput (inputId) { $('document').ready(() => { document.getElementById('volunteer-file').addEventListener('change', function (event) { - document.getElementById('volunteer-import-button').disabled = event.target.value == '' + document.getElementById('volunteer-import-button').disabled = event.target.value === '' const file = document.getElementById('volunteer-file').files[0] storeCSVFile(file, 'volunteer-file') }) document.getElementById('supervisor-file').addEventListener('change', function (event) { - document.getElementById('supervisor-import-button').disabled = event.target.value == '' + document.getElementById('supervisor-import-button').disabled = event.target.value === '' const file = document.getElementById('supervisor-file').files[0] storeCSVFile(file, 'supervisor-file') }) From 24a9f117fc330143e731df253ec6e46daca44885 Mon Sep 17 00:00:00 2001 From: Harsohail Brar Date: Fri, 22 Apr 2022 15:34:35 -0600 Subject: [PATCH 16/23] update import request and importer tests --- .../lib/importers/supervisor_importer_spec.rb | 8 ++++--- spec/lib/importers/volunteer_importer_spec.rb | 8 ++++--- spec/requests/imports_spec.rb | 21 ++++++++++++------- 3 files changed, 24 insertions(+), 13 deletions(-) diff --git a/spec/lib/importers/supervisor_importer_spec.rb b/spec/lib/importers/supervisor_importer_spec.rb index d4d46d25f0..891741aec4 100644 --- a/spec/lib/importers/supervisor_importer_spec.rb +++ b/spec/lib/importers/supervisor_importer_spec.rb @@ -78,11 +78,12 @@ }.to change(existing_supervisor, :display_name).to("Supervisor Two") end - it "updates phone number to valid number" do + it "updates phone number to valid number and turns on sms notifications" do expect { supervisor_importer.import_supervisors existing_supervisor.reload }.to change(existing_supervisor, :phone_number).to("+11111111111") + .and change(existing_supervisor, :receive_sms_notifications).to(true) end end @@ -101,13 +102,14 @@ context "when row doesn't have phone number" do let(:supervisor_import_data_path) { Rails.root.join("spec", "fixtures", "supervisors_without_phone_numbers.csv") } - let!(:existing_supervisor_with_number) { create(:supervisor, display_name: "#", email: "supervisor1@example.net", phone_number: "+11111111111") } + let!(:existing_supervisor_with_number) { create(:supervisor, display_name: "#", email: "supervisor1@example.net", phone_number: "+11111111111", receive_sms_notifications: true) } - it "updates phone number to be deleted" do + it "updates phone number to be deleted and turns off sms notifications" do expect { supervisor_importer.import_supervisors existing_supervisor_with_number.reload }.to change(existing_supervisor_with_number, :phone_number).to("") + .and change(existing_supervisor_with_number, :receive_sms_notifications).to(false) end end diff --git a/spec/lib/importers/volunteer_importer_spec.rb b/spec/lib/importers/volunteer_importer_spec.rb index 8f50c1523e..02e3d28fc0 100644 --- a/spec/lib/importers/volunteer_importer_spec.rb +++ b/spec/lib/importers/volunteer_importer_spec.rb @@ -50,11 +50,12 @@ }.to change(existing_volunteer, :display_name).to("Volunteer One") end - it "updates phone number to valid number" do + it "updates phone number to valid number and turns sms notifications on" do expect { volunteer_importer.call existing_volunteer.reload }.to change(existing_volunteer, :phone_number).to("+11234567890") + .and change(existing_volunteer, :receive_sms_notifications).to(true) end end @@ -73,13 +74,14 @@ context "when row doesn't have phone number" do let(:import_file_path) { Rails.root.join("spec", "fixtures", "volunteers_without_phone_numbers.csv") } - let!(:existing_volunteer_with_number) { create(:volunteer, display_name: "#", email: "volunteer2@example.net", phone_number: "+11111111111") } + let!(:existing_volunteer_with_number) { create(:volunteer, display_name: "#", email: "volunteer2@example.net", phone_number: "+11111111111", receive_sms_notifications: true) } - it "updates phone number to be deleted" do + it "updates phone number to be deleted and turns sms notifications off" do expect { volunteer_importer.call existing_volunteer_with_number.reload }.to change(existing_volunteer_with_number, :phone_number).to("") + .and change(existing_volunteer_with_number, :receive_sms_notifications).to(false) end end diff --git a/spec/requests/imports_spec.rb b/spec/requests/imports_spec.rb index f1f61949a4..413c9eb235 100644 --- a/spec/requests/imports_spec.rb +++ b/spec/requests/imports_spec.rb @@ -30,7 +30,8 @@ post imports_url, params: { import_type: "volunteer", - file: upload_file(supervisor_file) + file: upload_file(supervisor_file), + sms_opt_in: "1" } expect(request.session[:import_error]).to include("Expected", VolunteerImporter::IMPORT_HEADER.join(", ")) @@ -42,7 +43,8 @@ post imports_url, params: { import_type: "supervisor", - file: upload_file(volunteer_file) + file: upload_file(volunteer_file), + sms_opt_in: "1" } expect(request.session[:import_error]).to include("Expected", SupervisorImporter::IMPORT_HEADER.join(", ")) @@ -54,7 +56,8 @@ post imports_url, params: { import_type: "casa_case", - file: upload_file(supervisor_file) + file: upload_file(supervisor_file), + sms_opt_in: "1" } expect(request.session[:import_error]).to include("Expected", CaseImporter::IMPORT_HEADER.join(", ")) @@ -70,7 +73,8 @@ post imports_url, params: { import_type: "volunteer", - file: upload_file(volunteer_file) + file: upload_file(volunteer_file), + sms_opt_in: "1" } }.to change(Volunteer, :count).by(3) @@ -89,7 +93,8 @@ post imports_url, params: { import_type: "supervisor", - file: upload_file(supervisor_file) + file: upload_file(supervisor_file), + sms_opt_in: "1" } }.to change(Supervisor, :count).by(3) @@ -112,7 +117,8 @@ post imports_url, params: { import_type: "supervisor", - file: upload_file(supervisor_volunteers_file) + file: upload_file(supervisor_volunteers_file), + sms_opt_in: "1" } }.to change(Supervisor, :count).by(2) @@ -131,7 +137,8 @@ post imports_url, params: { import_type: "casa_case", - file: upload_file(case_file) + file: upload_file(case_file), + sms_opt_in: "1" } }.to change(CasaCase, :count).by(3) From 042b44391add8effca3c06bdbf36f1a12aacd6ec Mon Sep 17 00:00:00 2001 From: Harsohail Brar Date: Fri, 22 Apr 2022 18:49:08 -0600 Subject: [PATCH 17/23] add system tests for sms opt in modal --- .../volunteers_without_phone_numbers.csv | 2 +- .../lib/importers/supervisor_importer_spec.rb | 4 +- spec/lib/importers/volunteer_importer_spec.rb | 4 +- spec/system/imports/index_spec.rb | 83 ++++++++++++++++++- 4 files changed, 87 insertions(+), 6 deletions(-) diff --git a/spec/fixtures/volunteers_without_phone_numbers.csv b/spec/fixtures/volunteers_without_phone_numbers.csv index 495abe83cc..31e3958257 100644 --- a/spec/fixtures/volunteers_without_phone_numbers.csv +++ b/spec/fixtures/volunteers_without_phone_numbers.csv @@ -1,3 +1,3 @@ display_name,email,phone_number -Volunteer One,volunteer1@example.net,11111111111 +Volunteer One,volunteer1@example.net, Volunteer Two,volunteer2@example.net, \ No newline at end of file diff --git a/spec/lib/importers/supervisor_importer_spec.rb b/spec/lib/importers/supervisor_importer_spec.rb index 891741aec4..8458c893df 100644 --- a/spec/lib/importers/supervisor_importer_spec.rb +++ b/spec/lib/importers/supervisor_importer_spec.rb @@ -83,7 +83,7 @@ supervisor_importer.import_supervisors existing_supervisor.reload }.to change(existing_supervisor, :phone_number).to("+11111111111") - .and change(existing_supervisor, :receive_sms_notifications).to(true) + .and change(existing_supervisor, :receive_sms_notifications).to(true) end end @@ -109,7 +109,7 @@ supervisor_importer.import_supervisors existing_supervisor_with_number.reload }.to change(existing_supervisor_with_number, :phone_number).to("") - .and change(existing_supervisor_with_number, :receive_sms_notifications).to(false) + .and change(existing_supervisor_with_number, :receive_sms_notifications).to(false) end end diff --git a/spec/lib/importers/volunteer_importer_spec.rb b/spec/lib/importers/volunteer_importer_spec.rb index 02e3d28fc0..0bcbaf3660 100644 --- a/spec/lib/importers/volunteer_importer_spec.rb +++ b/spec/lib/importers/volunteer_importer_spec.rb @@ -55,7 +55,7 @@ volunteer_importer.call existing_volunteer.reload }.to change(existing_volunteer, :phone_number).to("+11234567890") - .and change(existing_volunteer, :receive_sms_notifications).to(true) + .and change(existing_volunteer, :receive_sms_notifications).to(true) end end @@ -81,7 +81,7 @@ volunteer_importer.call existing_volunteer_with_number.reload }.to change(existing_volunteer_with_number, :phone_number).to("") - .and change(existing_volunteer_with_number, :receive_sms_notifications).to(false) + .and change(existing_volunteer_with_number, :receive_sms_notifications).to(false) end end diff --git a/spec/system/imports/index_spec.rb b/spec/system/imports/index_spec.rb index 366a31558c..b340d219d3 100644 --- a/spec/system/imports/index_spec.rb +++ b/spec/system/imports/index_spec.rb @@ -1,7 +1,8 @@ require "rails_helper" RSpec.describe "imports/index", type: :system do - let(:volunteer) { build(:volunteer) } + let(:volunteer) { create(:volunteer) } + let(:admin) { create(:casa_admin) } context "as a volunteer" do before { sign_in volunteer } @@ -12,4 +13,84 @@ expect(page).to have_selector(".alert", text: "Sorry, you are not authorized to perform this action.") end end + + context "import volunteer csv with phone numbers", js: true do + let(:import_file_path) { Rails.root.join("spec", "fixtures", "volunteers.csv") } + + it "shows sms opt in modal" do + sign_in admin + visit imports_path(:volunteer) + + expect(page).to have_content("Import Volunteers") + expect(page).to have_button("volunteer-import-button", disabled: true) + + attach_file "volunteer-file", import_file_path + click_button "volunteer-import-button" + + expect(page).to have_text("SMS Opt In") + expect(page).to have_button("sms-opt-in-continue-button", disabled: true) + + check "sms-opt-in-checkbox" + click_button "sms-opt-in-continue-button" + + expect(page).to have_text("You successfully imported") + end + end + + context "import volunteer csv without phone numbers", js: true do + let(:import_file_path) { Rails.root.join("spec", "fixtures", "volunteers_without_phone_numbers.csv") } + + it "shows successful import" do + sign_in admin + visit imports_path(:volunteer) + + expect(page).to have_content("Import Volunteers") + + attach_file "volunteer-file", import_file_path + click_button "volunteer-import-button" + + expect(page).to have_text("You successfully imported") + end + end + + context "import supervisors csv with phone numbers", js: true do + let(:import_file_path) { Rails.root.join("spec", "fixtures", "supervisors.csv") } + + it "shows sms opt in modal" do + sign_in admin + visit imports_path + click_on "supervisor-tab" + + expect(page).to have_content("Import Supervisors") + expect(page).to have_button("supervisor-import-button", disabled: true) + + attach_file "supervisor-file", import_file_path + click_button "supervisor-import-button" + + expect(page).to have_text("SMS Opt In") + expect(page).to have_button("sms-opt-in-continue-button", disabled: true) + + check "sms-opt-in-checkbox" + click_button "sms-opt-in-continue-button" + + expect(page).to have_text("You successfully imported") + end + end + + context "import supervisors csv without phone numbers", js: true do + let(:import_file_path) { Rails.root.join("spec", "fixtures", "supervisors_without_phone_numbers.csv") } + + it "shows successful import" do + sign_in admin + visit imports_path + click_link "supervisor-tab" + + expect(page).to have_content("Import Supervisors") + + attach_file "supervisor-file", import_file_path + click_button "supervisor-import-button" + + expect(page).to have_text("You successfully imported") + end + end end From f2c66f8151651d77699ef1f6aa82b953966e79be Mon Sep 17 00:00:00 2001 From: Harsohail Brar Date: Fri, 22 Apr 2022 19:52:24 -0600 Subject: [PATCH 18/23] reduce code complexity and duplication --- app/javascript/src/import.js | 30 ++++++++++-------------- app/lib/importers/supervisor_importer.rb | 1 - 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/app/javascript/src/import.js b/app/javascript/src/import.js index 23236dd779..15dd38700f 100644 --- a/app/javascript/src/import.js +++ b/app/javascript/src/import.js @@ -53,23 +53,19 @@ function populateFileInput (inputId) { } $('document').ready(() => { - document.getElementById('volunteer-file').addEventListener('change', function (event) { - document.getElementById('volunteer-import-button').disabled = event.target.value === '' - const file = document.getElementById('volunteer-file').files[0] - storeCSVFile(file, 'volunteer-file') - }) + ['volunteer', 'supervisor'].forEach((importType) => { + const inputFileElementId = `${importType}-file` - document.getElementById('supervisor-file').addEventListener('change', function (event) { - document.getElementById('supervisor-import-button').disabled = event.target.value === '' - const file = document.getElementById('supervisor-file').files[0] - storeCSVFile(file, 'supervisor-file') - }) + document.getElementById(inputFileElementId).addEventListener('change', function (event) { + document.getElementById(`${importType}-import-button`).disabled = event.target.value === '' + const file = document.getElementById(inputFileElementId).files[0] + storeCSVFile(file, inputFileElementId) + }) - if (document.getElementById('smsOptIn') === null) { - delete localStorage['volunteer-file'] - delete localStorage['supervisor-file'] - } else { - populateFileInput('volunteer-file') - populateFileInput('supervisor-file') - } + if (document.getElementById('smsOptIn') == null) { + delete localStorage[inputFileElementId] + } else { + populateFileInput(inputFileElementId) + } + }) }) diff --git a/app/lib/importers/supervisor_importer.rb b/app/lib/importers/supervisor_importer.rb index 8b9b7ef4fd..656203736e 100644 --- a/app/lib/importers/supervisor_importer.rb +++ b/app/lib/importers/supervisor_importer.rb @@ -36,7 +36,6 @@ def import_supervisors volunteer_assignment_list.each do |volunteer| if volunteer.supervisor next if volunteer.supervisor == supervisor - raise "Volunteer #{volunteer.email} already has a supervisor" else supervisor.volunteers << volunteer From 93668223b1a52f026aa53841a729d64ec6219a77 Mon Sep 17 00:00:00 2001 From: Harsohail Brar Date: Sat, 23 Apr 2022 10:10:54 -0600 Subject: [PATCH 19/23] reduce supervisor import function to be the accepted 25 lines --- app/lib/importers/supervisor_importer.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/lib/importers/supervisor_importer.rb b/app/lib/importers/supervisor_importer.rb index 656203736e..94f512e227 100644 --- a/app/lib/importers/supervisor_importer.rb +++ b/app/lib/importers/supervisor_importer.rb @@ -22,7 +22,6 @@ def import_supervisors supervisor = Supervisor.find_by(email: supervisor_params[:email]) volunteer_assignment_list = email_addresses_to_users(Volunteer, String(row[:supervisor_volunteers])) - if volunteer_assignment_list.count != String(row[:supervisor_volunteers]).split(",").count raise "Row contains unimported volunteers." end From ec460bc239ad7c30f3f67a858cedbd8b740bdd37 Mon Sep 17 00:00:00 2001 From: Harsohail Brar Date: Sat, 23 Apr 2022 10:28:35 -0600 Subject: [PATCH 20/23] abstract out volunteer assignment in supervisor import to reduce function length --- app/lib/importers/supervisor_importer.rb | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/app/lib/importers/supervisor_importer.rb b/app/lib/importers/supervisor_importer.rb index 94f512e227..78889cb278 100644 --- a/app/lib/importers/supervisor_importer.rb +++ b/app/lib/importers/supervisor_importer.rb @@ -22,6 +22,7 @@ def import_supervisors supervisor = Supervisor.find_by(email: supervisor_params[:email]) volunteer_assignment_list = email_addresses_to_users(Volunteer, String(row[:supervisor_volunteers])) + if volunteer_assignment_list.count != String(row[:supervisor_volunteers]).split(",").count raise "Row contains unimported volunteers." end @@ -32,14 +33,7 @@ def import_supervisors supervisor = create_user_record(Supervisor, supervisor_params) end - volunteer_assignment_list.each do |volunteer| - if volunteer.supervisor - next if volunteer.supervisor == supervisor - raise "Volunteer #{volunteer.email} already has a supervisor" - else - supervisor.volunteers << volunteer - end - end + assign_volunteers(supervisor, volunteer_assignment_list) end end @@ -48,4 +42,15 @@ def update_supervisor(supervisor, supervisor_params, volunteer_assignment_list) supervisor.update(supervisor_params) end end + + def assign_volunteers(supervisor, volunteer_assignment_list) + volunteer_assignment_list.each do |volunteer| + if volunteer.supervisor + next if volunteer.supervisor == supervisor + raise "Volunteer #{volunteer.email} already has a supervisor" + else + supervisor.volunteers << volunteer + end + end + end end From c95c1ab35e6c7d45fee86757ad84677d7ebd43b9 Mon Sep 17 00:00:00 2001 From: Harsohail Brar Date: Sat, 23 Apr 2022 11:16:42 -0600 Subject: [PATCH 21/23] fix linting issues and code complexity --- app/lib/importers/supervisor_importer.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/lib/importers/supervisor_importer.rb b/app/lib/importers/supervisor_importer.rb index 78889cb278..73062ed4be 100644 --- a/app/lib/importers/supervisor_importer.rb +++ b/app/lib/importers/supervisor_importer.rb @@ -22,7 +22,7 @@ def import_supervisors supervisor = Supervisor.find_by(email: supervisor_params[:email]) volunteer_assignment_list = email_addresses_to_users(Volunteer, String(row[:supervisor_volunteers])) - + if volunteer_assignment_list.count != String(row[:supervisor_volunteers]).split(",").count raise "Row contains unimported volunteers." end @@ -45,8 +45,8 @@ def update_supervisor(supervisor, supervisor_params, volunteer_assignment_list) def assign_volunteers(supervisor, volunteer_assignment_list) volunteer_assignment_list.each do |volunteer| + next if volunteer.supervisor && volunteer.supervisor == supervisor if volunteer.supervisor - next if volunteer.supervisor == supervisor raise "Volunteer #{volunteer.email} already has a supervisor" else supervisor.volunteers << volunteer From d2c3491905180038c5c4794412b950b32d51dc67 Mon Sep 17 00:00:00 2001 From: Harsohail Brar Date: Sat, 23 Apr 2022 11:31:38 -0600 Subject: [PATCH 22/23] remove next if statement to remove code complexity --- app/lib/importers/supervisor_importer.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/lib/importers/supervisor_importer.rb b/app/lib/importers/supervisor_importer.rb index 73062ed4be..23cdefe436 100644 --- a/app/lib/importers/supervisor_importer.rb +++ b/app/lib/importers/supervisor_importer.rb @@ -44,8 +44,7 @@ def update_supervisor(supervisor, supervisor_params, volunteer_assignment_list) end def assign_volunteers(supervisor, volunteer_assignment_list) - volunteer_assignment_list.each do |volunteer| - next if volunteer.supervisor && volunteer.supervisor == supervisor + volunteer_assignment_list.select { |v| v.supervisor != supervisor }.each do |volunteer| if volunteer.supervisor raise "Volunteer #{volunteer.email} already has a supervisor" else From 52ff24e75083db72e2d9f26ee7e6c050e96ed0cd Mon Sep 17 00:00:00 2001 From: Harsohail Brar Date: Sun, 24 Apr 2022 12:37:36 -0600 Subject: [PATCH 23/23] change sms opt in error to be named as warning --- app/controllers/imports_controller.rb | 10 +++++----- app/views/imports/_supervisors.html.erb | 2 +- app/views/imports/_volunteers.html.erb | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/controllers/imports_controller.rb b/app/controllers/imports_controller.rb index 704ffd422e..62e164c872 100644 --- a/app/controllers/imports_controller.rb +++ b/app/controllers/imports_controller.rb @@ -8,9 +8,9 @@ def index authorize :import @import_type = params.fetch(:import_type, "volunteer") @import_error = session[:import_error] - @sms_opt_in_error = session[:sms_opt_in_error] + @sms_opt_in_warning = session[:sms_opt_in_warning] session[:import_error] = nil - session[:sms_opt_in_error] = nil + session[:sms_opt_in_warning] = nil end def create @@ -27,8 +27,8 @@ def create if import[:type] == :error session[:import_error] = message - elsif import[:type] == :sms_opt_in_error - session[:sms_opt_in_error] = import[:import_type] + elsif import[:type] == :sms_opt_in_warning + session[:sms_opt_in_warning] = import[:import_type] # Only use flash for success messages. Otherwise may cause CookieOverflow else flash[:success] = message @@ -72,7 +72,7 @@ def import_from_csv(import_type, sms_opt_in, file, org_id) return validated_file unless validated_file.nil? if requires_sms_opt_in(file, import_type, sms_opt_in) - return {type: :sms_opt_in_error, import_type: import_type} + return {type: :sms_opt_in_warning, import_type: import_type} end case import_type diff --git a/app/views/imports/_supervisors.html.erb b/app/views/imports/_supervisors.html.erb index 143198b35c..c7a05852aa 100644 --- a/app/views/imports/_supervisors.html.erb +++ b/app/views/imports/_supervisors.html.erb @@ -27,7 +27,7 @@ style: "margin: auto;" %> - <%= render "sms_opt_in_modal", { form: f } if @sms_opt_in_error == "supervisor" %> + <%= render "sms_opt_in_modal", { form: f } if @sms_opt_in_warning == "supervisor" %> <%= button_tag id: "supervisor-import-button", class: "btn btn-md btn-success pull-right", disabled: true, data: { disable_with: " #{t(".disable_with")}"} do %> <%= t(".button.import") %> diff --git a/app/views/imports/_volunteers.html.erb b/app/views/imports/_volunteers.html.erb index 1ff92bd8ec..e50b9cedb5 100644 --- a/app/views/imports/_volunteers.html.erb +++ b/app/views/imports/_volunteers.html.erb @@ -25,7 +25,7 @@ <%= f.file_field :file, id: 'volunteer-file', accept: 'text/csv', class: 'form-control-file', style: "margin: auto;" %> - <%= render "sms_opt_in_modal", { form: f } if @sms_opt_in_error == "volunteer" %> + <%= render "sms_opt_in_modal", { form: f } if @sms_opt_in_warning == "volunteer" %> <%= button_tag id: "volunteer-import-button", class: "btn btn-md btn-success pull-right", disabled: true, data: { disable_with: " #{t(".disable_with")}"} do %> <%= t(".button.import") %>