Add new Customer model (including import script)#27
Merged
Conversation
tekin
force-pushed
the
290-add-customers-model
branch
2 times, most recently
from
0fc6128 to
755b96d
Compare
leeky
approved these changes
Aug 7, 2018
Contributor
leeky
left a comment
leeky
left a comment
There was a problem hiding this comment.
I've added some minor comments, but otherwise LGTM! 
| t.timestamps | ||
| end | ||
|
|
||
| add_index :customers, :urn, unique: true |
| @@ -0,0 +1,6 @@ | |||
| class Customer < ApplicationRecord | |||
| enum sector: %w[central_government wider_public_sector] | |||
Contributor
There was a problem hiding this comment.
Is it worth being explicit with the mappings here?
| RSpec.describe Customer do | ||
| it { is_expected.to validate_presence_of(:name) } | ||
| it { is_expected.to validate_presence_of(:urn) } | ||
| it { is_expected.to validate_presence_of(:sector) } |
Contributor
There was a problem hiding this comment.
How about a validate_uniqueness_of(:urn) test here for completeness?
Contributor
Author
There was a problem hiding this comment.
adding for completeness, although I'll be honest I'm not a huge fan of these declarative specs. not sure what value they add as they are simply mirror the validations on the model.
| csv = CSV.table(Rails.root.join('db', 'data_migrate', '20180807095900_customers.csv')) | ||
|
|
||
| puts 'Importing customers...' | ||
| bar = ProgressBar.new(csv.count) |
Contributor
There was a problem hiding this comment.
Does this actually render properly inside the docker containers slightly-broken terminal?
Contributor
Author
There was a problem hiding this comment.
tested locally and it appears it does.
| csv.each do |customer_row| | ||
| postcode = customer_row[:postcode] == 'XXXX' ? nil : customer_row[:postcode] | ||
|
|
||
| Customer.create!( |
Contributor
There was a problem hiding this comment.
How about making this able to be run more than once? (i.e. if the VPN dies in the middle of the script running)
Also, it's probably worth calling strip on all these input strings, and a lot of the other exports have included erroneous whitespace.
This is the model that represents the part of government a supplier is selling to. This model has become necessary so that we can generate the report for finance that groups invoice totals and levys according to the customers' sectors, i.e. "central government", or "wider public sector". I'm keeping the attributes on this model to a minimum for now to avoid having to add code we don't need just yet. Right now, all we really need are the URN number and the sector. That said, I've additionally included the name and postcode, mainly because we are expecting to need these two fields very soon when we add the URN look-up tool to the supplier-facing application. I've also avoided adding too many validations or constraints on fields such as the URN number. We can define and implement those when we get to the backend administrative part of the system.
tekin
force-pushed
the
290-add-customers-model
branch
from
755b96d to
f27a330
Compare
Can be used to seed the database with all the customers we currently know about: $ rails runner db/data_migrate/20180807095900_import_customers.rb The CSV data was generated using the URNs file we are currently sharing with suppliers. Some of the customers from that file have been excluded: - Customers with "Active" set to "False" as business cannot be reported against these - A handful of "bucket code" URNS that shouldn't be reported against by suppliers (e.g. Education/991100) Note that some customers have been given a postcode of 'XXXX'. I'm assuming this was to get around some limitation in MISO that meant a postcode needs to be set. Rather than carry this over to the new system, the script will ignore these postcodes. Also note that I've updated the rubocop config to exclude the Rails/Output rule for the data migrations, as there is no reason to prevent outputting directly to STDOUT from scripts like this that are expected to be run directly from the terminal.
tekin
force-pushed
the
290-add-customers-model
branch
from
f27a330 to
9337b63
Compare
Contributor
|
👍 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds the Customer model and a script to import all the customers we currently know about so that we can use the data to generate the finance report. More details in the commit messages.