(feature) added date_passed & document_types to Legislations #10
Conversation
app/services/import_helpers.rb
Outdated
| puts "Couldn't find Location with ISO: #{country_iso}" | ||
| end | ||
|
|
||
| def try_to_parse_date(date, expected_date_formats) |
There was a problem hiding this comment.
maybe rename it to find_date_with_format?
There was a problem hiding this comment.
You can change it, for me both are good.
There was a problem hiding this comment.
I will leave it as it is.
| @@ -1,3 +1,25 @@ | |||
| # To tag your model with let's say 'keywords', you need to | |||
app/models/document_type.rb
Outdated
| # | ||
|
|
||
| class DocumentType < Tag | ||
| DOCUMENT_TYPES = [ |
There was a problem hiding this comment.
Hmm, not sure if we want to restrict that list
There was a problem hiding this comment.
Also, that list is funny now, I wouldn't pay too much attention to what is in the data currently. See there is document type All, they should clean this up, what they wanted to just tag Law with different types. Maybe we will have to extract some of those tags to be something else in the future (for example if application logic will depend on it, I suspect "Out of date" would be the first candidate)
There was a problem hiding this comment.
yeah I agree, this list will definitely change. For now I can remove the validation, but eventually, I think we will need some constraint on "document type". I guess this is a more precise term than, let's say "category".
There was a problem hiding this comment.
Not sure, I think right now those are like categories, and definitely, that list needs cleanup from their side. I would just remove validation for now.
app/services/import_helpers.rb
Outdated
| @@ -0,0 +1,17 @@ | |||
| module ImportHelpers | |||
app/services/import_helpers.rb
Outdated
| puts "Couldn't find Location with ISO: #{country_iso}" | ||
| end | ||
|
|
||
| def try_to_parse_date(date, expected_date_formats) |
There was a problem hiding this comment.
You can change it, for me both are good.
| let!(:legislation) { create(:legislation) } | ||
| let(:valid_attributes) { | ||
| attributes_for(:legislation).merge( |
There was a problem hiding this comment.
@tsubik I couldn't find a way to include location by default in what's returned by attributes_for :/ then I found your approach from other models and used it here as well.
There was a problem hiding this comment.
Yes, attributes_for are not working with associations, probably they have a good reason for that.
|
@tsubik more updates:
|
kowal
changed the title
app/models/document_type.rb
Outdated
| # | ||
|
|
||
| class DocumentType < Tag | ||
| DOCUMENT_TYPES = [ |
There was a problem hiding this comment.
Not sure, I think right now those are like categories, and definitely, that list needs cleanup from their side. I would just remove validation for now.
app/services/import_helpers.rb
Outdated
| @@ -0,0 +1,17 @@ | |||
| module ImportHelpers | |||
There was a problem hiding this comment.
We can move it to app/services/concerns. I'm not sure if that would require to add new concern directory to autoload_paths in config/application.rb, like config.autoload_paths += Dir[Rails.root.join("app", "services", "concerns")]
There was a problem hiding this comment.
I think concerns are meant to encapsulate a piece of domain logic, here we have 2 simple helper methods related to different things.
Maybe I add 2 different modules, which could be used like this (I also renamed methods)
def normalize_date
Import::DateUtils.safe_parse(date)
end
def attributes
{ location: Import::LocationUtils.find_by_iso(..) }
endThere was a problem hiding this comment.
Also, I'd move all import classes to services/import as well, so we'll end up with Import::Companies.new.call and so on.
There was a problem hiding this comment.
done, please re-review
| let!(:legislation) { create(:legislation) } | ||
| let(:valid_attributes) { | ||
| attributes_for(:legislation).merge( |
There was a problem hiding this comment.
Yes, attributes_for are not working with associations, probably they have a good reason for that.
date_passedanddocument_types(as tags) toLegislations.you can re-import records via
bundle exec rake import:legislation