popolo-project / popolo-engine

A reference implementation of Popolo as a Rails engine
MIT License
11 stars 7 forks source link

Add remaining models from spec into engine #19

Closed digitalWestie closed 9 years ago

digitalWestie commented 9 years ago

Hello, I've been working on this to use with an app I'm building. I'm not very well versed with the spec or mongoid so any help / feedback would be appreciated.

coveralls commented 9 years ago

Coverage Status

Changes Unknown when pulling 218e20a01ad64c3311a3ebc109df04c8f14d1942 on digitalWestie:master into \ on opennorth:master**.

jpmckinney commented 9 years ago

Had a quick look - It's looking good! I see there are some TODO in the code. Is this ready for review, or should I wait for those TODO's to be done?

digitalWestie commented 9 years ago

Oh, forgot about some of those. It would be good if you could review it. That way I can see if I'm on the right track regarding the spec.

coveralls commented 9 years ago

Coverage Status

Changes Unknown when pulling ba8051b26fe78fb86a65322a79207553f5d2a8eb on digitalWestie:master into \ on opennorth:master**.

coveralls commented 9 years ago

Coverage Status

Changes Unknown when pulling 9f3e0238c42d38fdb166912cb151f934a1271326 on digitalWestie:master into \ on opennorth:master**.

jpmckinney commented 9 years ago

Can you confirm that the autoload_paths change is necessary?

I'll squash the commits into one. I'm reverting the date format stuff because the date format should respect ISO 8601, which the new regex does not. You should just reformat your data to match the regex. Popolo allows for truncated dates like 2004, 2004-01.

Nothing uses DateTimeString yet, so I won't merge that. I'll push my commits soon.

jpmckinney commented 9 years ago

I've pushed my commits. Thanks for moving this forward! Please switch to HEAD for additional commits.

digitalWestie commented 9 years ago

Good stuff @jpmckinney - on the topic of date validation I see I've got it wrong on respecting ISO 8601. However, the reason I went messing with the dates was due to errors cropping up during validation. It tries to parse the date and then fails during validation, that's why I came up with a custom validation method.

Stick this test in organization_spec.rb and you'll see what I mean:

it "can validate without causing errors" do
    subject = FactoryGirl.create(:organization)
    subject.founding_date = ''
    subject.valid?.should == false
end

Which results in:

ArgumentError: invalid date
    from (irb):11:in `parse'
    from (irb):11

I believe the cause is in date_string.rb

def demongoize(object)
  object.nil? ? nil : Date.parse(object)
end

Date.parse() doesn't handle '2004' or 2004-11:

2.1.2 :014 > Date.parse('2004-11')
ArgumentError: invalid date
    from (irb):14:in `parse'
    from (irb):14

Thoughts?

jpmckinney commented 9 years ago

I've pushed commits (and specs) to handle these cases. Thanks!