Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(feature): bulk upload for Targets #56

Merged
merged 7 commits into from
Sep 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions app/admin/targets.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
ActiveAdmin.register Target do
config.batch_actions = false

menu parent: 'Laws', priority: 3

decorate_with TargetDecorator
Expand All @@ -24,7 +22,8 @@
data_export_sidebar 'Targets'

index do
id_column
selectable_column
column(:description) { |target| link_to target.description, admin_target_path(target) }
column :geography
column :sector
column :target_scope
Expand Down Expand Up @@ -84,4 +83,19 @@

f.actions
end

csv do
column :id
column(:target_type) { |t| t.model.target_type }
column :description
column :ghg_target
column :year
column :base_year_period
column :single_year
column(:geography) { |t| t.geography.name }
column(:geography_iso) { |t| t.geography.iso }
column(:sector) { |t| t.sector.name }
column(:target_scope) { |t| t.target_scope.name }
column :visibility_status
end
end
5 changes: 5 additions & 0 deletions app/assets/stylesheets/admin/base.scss
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ fieldset.actions .cancel a {

// Status
.status_tag {
// Yes / No
&.no { background: $flag-false-color; }
&.yes { background: $flag-true-color; }

// visibility status / publishing flow
&.draft { background: $draft-color; }
&.pending { background: $pending-color; }
&.published { background: $published-color; }
Expand Down
8 changes: 6 additions & 2 deletions app/assets/stylesheets/admin/settings.scss
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,17 @@ $apple: #4ba532;
$silver: #c4c0c0;
$dusty-gray: #979494;

//Status label colors
// Status label colors
$draft-color: #adc9d0;
$pending-color: $yellow-orange;
$published-color: $apple;
$published-color: #6d9f71;;
$archived-color: #d0a088;
$unset-color: $dusty-gray;

// Yes / No
$flag-true-color: #8fbb93;
$flag-false-color: #b7b6b6;

// Active Material Theme Colors Change
$am-theme-primary: $allports;
$am-theme-accent: $malibu;
Expand Down
1 change: 1 addition & 0 deletions app/models/data_upload.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class DataUpload < ApplicationRecord
Legislations
Litigations
Companies
Targets
].freeze

belongs_to :uploaded_by, class_name: 'AdminUser', optional: true
Expand Down
20 changes: 14 additions & 6 deletions app/services/csv_import/base_importer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,10 @@ def import_each_csv_row(csv)

def with_logging(row_index)
yield
rescue ActiveRecord::RecordInvalid,
ActiveRecord::RecordNotFound,
ArgumentError => e
msg = "[BaseImporter] Error on row #{row_index}: '#{e.message}' for data: #{e.record.attributes}"
warn msg
errors.add(:base, :invalid_row, message: msg, row: row_index)
rescue ActiveRecord::RecordInvalid => e
handle_row_error(row_index, e, "for data: #{e.record.attributes}")
rescue ActiveRecord::RecordNotFound, ArgumentError => e
handle_row_error(row_index, e)
end

def update_import_results(was_new_record, any_changes)
Expand All @@ -96,5 +94,15 @@ def update_import_results(was_new_record, any_changes)
import_results[:not_changed_records] += 1
end
end

def handle_row_error(row_index, exception, context_message = nil)
readable_error_message = "Error on row #{row_index}: #{exception.message}."

# log error with more details
warn "[#{self.class.name}] #{readable_error_message} #{context_message}"

# add import error
errors.add(:base, :invalid_row, message: readable_error_message, row: row_index)
end
end
end
63 changes: 63 additions & 0 deletions app/services/csv_import/targets.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
module CSVImport
class Targets < BaseImporter
include UploaderHelpers

def import
import_each_csv_row(csv) do |row|
target = prepare_target(row)

target.assign_attributes(target_attributes(row))

was_new_record = target.new_record?
any_changes = target.changed?

target.save!

update_import_results(was_new_record, any_changes)
end
end

private

def resource_klass
Target
end

def prepare_target(row)
find_record_by(:id, row) || resource_klass.new
end

# Builds resource attributes from a CSV row.
#
# If not present in DB, will create related:
# - sector
#
def target_attributes(row)
{
target_type: row[:target_type],
description: row[:description],
ghg_target: row[:ghg_target],
year: row[:year],
base_year_period: row[:base_year_period],
single_year: row[:single_year],
geography: geographies[row[:geography_iso]],
sector: find_or_create_sector(row[:sector]),
target_scope: find_target_scope(row[:target_scope]),
visibility_status: row[:visibility_status]
}
end

def find_or_create_sector(sector_name)
return unless sector_name.present?

Sector.where('lower(name) = ?', sector_name.downcase).first ||
Sector.new(name: sector_name)
end

def find_target_scope(target_scope_name)
return unless target_scope_name.present?

TargetScope.where('lower(name) = ?', target_scope_name.downcase).first
end
end
end
82 changes: 45 additions & 37 deletions spec/commands/csv_data_upload_spec.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
require 'rails_helper'

describe 'CsvDataUpload (integration)' do
let(:legislations_simple_csv) { fixture_file('legislations_simple.csv') }
let(:litigations_simple_csv) { fixture_file('litigations_simple.csv') }
let(:companies_simple_csv) { fixture_file('companies_simple.csv') }
let(:legislations_csv) { fixture_file('legislations.csv') }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as @tsubik suggested some time ago, I removed _simple suffix as it doesn't bring much value.

let(:litigations_csv) { fixture_file('litigations.csv') }
let(:companies_csv) { fixture_file('companies.csv') }
let(:targets_csv) { fixture_file('targets.csv') }

let!(:countries) do
[
Expand All @@ -14,9 +15,16 @@
]
end

let!(:target_scopes) do
[
create(:target_scope, name: 'Default Scope'),
create(:target_scope, name: 'High')
]
end

describe 'errors handling' do
it 'sets error for unknown uploader class' do
command = Command::CsvDataUpload.new(uploader: 'FooUploader', file: legislations_simple_csv)
command = Command::CsvDataUpload.new(uploader: 'FooUploader', file: legislations_csv)

expect(command.call).to eq(false)
expect(command.errors.to_a).to include('Uploader is not included in the list')
Expand All @@ -30,46 +38,46 @@
end
end

it 'imports CSV files with legislations data' do
command = Command::CsvDataUpload.new(uploader: 'Legislations', file: legislations_simple_csv)

# first import - 2 new records should be created
expect(command.call).to eq(true)
expect(command.details.symbolize_keys)
.to eq(new_records: 2, not_changed_records: 0, rows: 2, updated_records: 0)

# 2nd subsequent import - nothing should change
expect(command.call).to eq(true)
expect(command.details.symbolize_keys)
.to eq(new_records: 0, not_changed_records: 2, rows: 2, updated_records: 0)
it 'imports CSV files with Legislation data' do
expect_data_upload_results(
Legislation,
legislations_csv,
new_records: 2, not_changed_records: 0, rows: 2, updated_records: 0
)
end

it 'imports CSV files with litigations data' do
command = Command::CsvDataUpload.new(uploader: 'Litigations', file: litigations_simple_csv)

# first import - 2 new records should be created
expect(command.call).to eq(true)
expect(command.details.symbolize_keys)
.to eq(new_records: 4, not_changed_records: 0, rows: 4, updated_records: 0)
it 'imports CSV files with Litigation data' do
expect_data_upload_results(
Litigation,
litigations_csv,
new_records: 4, not_changed_records: 0, rows: 4, updated_records: 0
)
end

# 2nd subsequent import - nothing should change
expect(command.call).to eq(true)
expect(command.details.symbolize_keys)
.to eq(new_records: 0, not_changed_records: 4, rows: 4, updated_records: 0)
it 'imports CSV files with Company data' do
expect_data_upload_results(
Company,
companies_csv,
new_records: 7, not_changed_records: 0, rows: 7, updated_records: 0
)
end

it 'imports CSV files with companies data' do
command = Command::CsvDataUpload.new(uploader: 'Companies', file: companies_simple_csv)
it 'imports CSV files with Target data' do
expect_data_upload_results(
Target,
targets_csv,
new_records: 3, not_changed_records: 0, rows: 3, updated_records: 0
)
end

# first import - 2 new records should be created
expect(command.call).to eq(true)
expect(command.details.symbolize_keys)
.to eq(new_records: 7, not_changed_records: 0, rows: 7, updated_records: 0)
def expect_data_upload_results(uploaded_resource_klass, csv, expected_details)
uploader_name = uploaded_resource_klass.name.pluralize
command = Command::CsvDataUpload.new(uploader: uploader_name, file: csv)

# 2nd subsequent import - nothing should change
expect(command.call).to eq(true)
expect(command.details.symbolize_keys)
.to eq(new_records: 0, not_changed_records: 7, rows: 7, updated_records: 0)
expect do
expect(command.call).to eq(true)
expect(command.details.symbolize_keys).to eq(expected_details)
end.to change(uploaded_resource_klass, :count).by(expected_details[:new_records])
end

def fixture_file(filename)
Expand Down
25 changes: 24 additions & 1 deletion spec/controllers/admin/targets_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

RSpec.describe Admin::TargetsController, type: :controller do
let(:admin) { create(:admin_user) }
let!(:target) { create(:target) }
let!(:target) { create(:target, year: 2030) }
let!(:target2) { create(:target, year: 2040) }
let!(:target3) { create(:target, year: 2050) }
let(:sector) { create(:sector) }
let(:geography) { create(:geography) }
let(:target_scope) { create(:target_scope) }
Expand All @@ -26,6 +28,23 @@
it { is_expected.to be_successful }
end

describe 'GET index with .csv format' do
before :each do
get :index, format: 'csv'
end

it('returns CSV file') do
expect(response.header['Content-Type']).to include('text/csv')
end

it('returns all targets') do
csv = response_as_csv

expect(csv.by_col[0].sort).to eq([target.id, target2.id, target3.id].map(&:to_s))
expect(csv.by_col[4].sort).to eq(%w[2030 2040 2050])
end
end

describe 'GET show' do
subject { get :show, params: {id: target.id} }

Expand Down Expand Up @@ -75,4 +94,8 @@
end
end
end

def response_as_csv
CSV.parse(response.body, headers: true)
end
end
2 changes: 1 addition & 1 deletion spec/factories/targets.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,6 @@
ghg_target { false }
single_year { true }
target_type { 'base_year_target' }
visibility_status { Litigation::VISIBILITY.sample }
visibility_status { Litigation::VISIBILITY.first }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's remove this randomness from specs ;)

end
end
4 changes: 4 additions & 0 deletions spec/support/fixtures/files/targets.csv
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
"Id","Target type","Description","Ghg target","Year","Base year period","Single year","Geography","Geography iso","Sector","Target scope","Visibility status"
"201","no_document_submitted","description","true","1995","1998","false","Japan","JPN","Airlines","High","draft"
"202","base_year_target","description","true","1994","1994","false","Poland","POL","Cement","Default Scope","published"
"203","intensity_target_and_trajectory_target","description","false","2003","2001","true","Poland","POL","Electricity Utilities","Default Scope","pending"