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/enable multiple isins #31

Merged
merged 4 commits into from Aug 12, 2019
Merged

Conversation

simaob
Copy link
Contributor

@simaob simaob commented Aug 12, 2019

This small PR allows the input of multiple isin codes for the same company.

  • render as strings;
  • using a tags input to manage them;
  • updating the importer to allow saving multiple isins, matching the import on the company name.

@simaob simaob requested review from kowal and tsubik August 12, 2019 13:32
company = Company.find_or_initialize_by(isin: row[:isin])
company = Company.find_or_initialize_by(name: row[:company_name])

company.isin = if company.isin.present?
Copy link
Contributor

Choose a reason for hiding this comment

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

couldn't this be moved to company_attributes method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably so. I kept it here, but it doesn't make so much sense now. I'll move it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm, actually, I would need to pass the current isin to that method if I were to manage this there. Currently the company_attributes is not aware of the record being updated. So not sure if that's worth it.

@@ -48,4 +48,8 @@ def latest_assessment
def to_s
name
end

def isin_string
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this belongs to CompanyDecorator which needs to be created :)

Copy link
Contributor

Choose a reason for hiding this comment

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

also, this name is a bit confusing - this returns an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right! I've moved the method to a new decorator, and I've renamed it to isin_as_tags. 🙇

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@simaob simaob force-pushed the feature/enable-multiple-isins branch from a611ff3 to e308deb Compare August 12, 2019 14:33
@simaob simaob merged commit fb795e7 into develop Aug 12, 2019
@simaob simaob deleted the feature/enable-multiple-isins branch August 12, 2019 14:46
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