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

TPI Production Ready Seeds #131

Merged
merged 14 commits into from
Nov 24, 2019
Merged

TPI Production Ready Seeds #131

merged 14 commits into from
Nov 24, 2019

Conversation

simaob
Copy link
Contributor

@simaob simaob commented Nov 21, 2019

This PR updates the TPI scripts and data files with the most up to date information, which we can use to seed production.

Main changes are:

  • we are using the existing TPI ids, instead of creating new ones each time, making it easy to link data;
  • isin stopped being an unique value, because they had to duplicate some companies that are assessed in different sectors;
  • updated companies script to import the new fields that were added recently;

- removed index and validation on ISIN as we had to duplicate some
companies that are assessed in multiple sectors;
- adds Hong Kong to the geographies list;
- updates import script for companies, and override ids to keep the TPI
ids;
@simaob simaob requested a review from tsubik November 21, 2019 18:17
@simaob
Copy link
Contributor Author

simaob commented Nov 21, 2019

oh I'm also importing Instrument Types, Instruments and LawSectors in this PR, did it directly in the seeds.rb file.

@simaob
Copy link
Contributor Author

simaob commented Nov 21, 2019

Fixed specs and rubocop and added company_id to MQ and CP assessments download, as this is now used to import the data.

Copy link
Collaborator

@tsubik tsubik left a comment

Choose a reason for hiding this comment

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

Nice! Almost there with TPI data.

Just one thing. CP assessment date is "No date" everywhere.

And the other below could be done here, or later (but then seeds needs to be reimported, I guess they will anyway)

We may also need an extra column to MQ::Assessment, methodology_version to know right away the version, that would be helpful when building question edit feature. This one should not be editable, should come from csv files, so all records in _M1 will have methodolody_version: M1 and so on.

@@ -46,7 +46,7 @@ class Company < ApplicationRecord

validates :ca100, inclusion: {in: [true, false]}
validates_presence_of :name, :slug, :isin, :market_cap_group
validates_uniqueness_of :slug, :isin, :name
Copy link
Collaborator

Choose a reason for hiding this comment

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

right, I missed that thing before, but that wasn't right after having multiple ISINs in one string ;]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I missed that too 😅

db/seeds.rb Outdated
file = File.open(Rails.root.join('db', 'seeds', 'companies.csv'), 'r')
CSVImport::Companies.new(file).call
file = File.open(Rails.root.join('db', 'seeds', 'tpi-companies.csv'), 'r')
importer = CSVImport::Companies.new(file)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just taking a note here (not a blocker for this PR) to refactor this just a little CSVImport::Companies.new(file, override_id: true)

@simaob
Copy link
Contributor Author

simaob commented Nov 22, 2019

Thanks for the feedback, I'll check it again later today, including the override small refactor.
In regards to the MQ version, I think I can pass it in the seeds, but then maybe we increment it on importing, so that it's sequential and doesn't need to be provided by the file each time. What do you think?

@simaob
Copy link
Contributor Author

simaob commented Nov 22, 2019

Refactored to have override_id as an option, and added methodology_version, added it to the importing files. The dates are so different that was hard to extract from that...

Looking now at the CP Assessments assessment_date

@simaob
Copy link
Contributor Author

simaob commented Nov 22, 2019

the cp-assessments file had the wrong format on the assessment date... fixed now. Should be good to go, unless there's feedback on my refactor! =D

@simaob simaob requested a review from tsubik November 22, 2019 15:26
Copy link
Collaborator

@tsubik tsubik left a comment

Choose a reason for hiding this comment

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

Looks great 👍

@@ -60,6 +65,12 @@ def csv_converters

private

def reset_id_seq
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -52,7 +52,8 @@ def assessment_attributes(row)
publication_date: publication_date(row),
level: row[:level],
notes: row[:notes],
questions: get_questions(row)
questions: get_questions(row),
methodology_version: row[:methodology_version]
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@tsubik tsubik merged commit 93cba49 into develop Nov 24, 2019
@tsubik tsubik deleted the feature/production-seeds branch November 25, 2019 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants