ThreeSixtyGiving / standard

The 360Giving data standard for UK philanthropic giving
http://www.threesixtygiving.org
Other
10 stars 15 forks source link

Recipient Org: County should be in CSV template #89

Closed timgdavies closed 8 years ago

timgdavies commented 8 years ago

At the moment 'Recipient Org: County' is not included in the relevant rollUp properties for the CSV template.

This is causing some people to add this column, but to get titles wrong, which is then causing problems.

caprenter commented 8 years ago

To be clear, this is not a field in the standard as such. (is it?)

Are you proposing it gets added to the standard, or just that the flatten-tool alters it's behaviour?

Is there a bigger problem with the flatten tool in that it should be allowing other/any random column titles to also be handled?

timgdavies commented 8 years ago

'Recipient Org: County' is a field in the standard in so far as:

{ "recipientOrganisation": {
  "addressRegion":"---"
}

is valid JSON in the standard, and any field should be allowed in flattened form (Parent field title: Child field title).

However, currently:

(a) Recipient Org: County is not included in the CSV template (the template is just a possible serialisation of the standard, not the standard itself) (b) I believe the flatten tool only maps field titles for rolled-up properties, and so incorrectly maps Recipient Org: County to a non-valid JSON field. There is an open issue for this at https://github.com/OpenDataServices/flatten-tool/issues/50 which would be good to get some focus on.

Short version:

caprenter commented 8 years ago

ok -thanks.

I'm just logging this because I was slightly confused here by a couple of things (this is not meant to be pedantic - but I can see how you might think it is!)

The json example above is not in 360Giving standard, those fields should be recipientOrganization and addressRegion

I also didn't realise that the Spreadsheet format of the 360Giving Standard was not a complete mapping - so looking at the documentation under http://www.threesixtygiving.org/standard/reference/#toc-grants-sheet I did not see Recipient Org:County

timgdavies commented 8 years ago

Good point on the json examples - I've updated.

On the mapping - happy to look at better documenting this.

I believe http://www.threesixtygiving.org/standard/reference/#toc-grants-sheet is generated from the same process that generates the CSV template - so if roll ups are changed, that will get changed too.

County is in the Organisation table in all cases: http://www.threesixtygiving.org/standard/reference/#toc-organization

caprenter commented 8 years ago

Thanks - that's helpful.

Bjwebb commented 8 years ago

Looked at this briefly today, and I think solving this particular issue for county in the schema is a matter of adding addressRegion to https://github.com/ThreeSixtyGiving/standard/blob/master/schema/360-giving-schema.json#L513

I'll try and submit a pull request over that over the next few days, but probably not today, as I want to take some time to review whether the current rollUp is correct (I don't think country is for example), and whether we could be better alerted of such errors.

timgdavies commented 8 years ago

What goes in rollUp is ultimately a design choice, rather than a matter of a technical error.

(In this case, the design choice to excluded County and Country was wrong.... but the rollUp it working as it should I believe).

Unlike OCDS which flattens everything it can out into the master table, in 360Giving we were aiming to allow some selection of what should end up in the default CSV template, to minimise the number of columns that contains.

I think the bigger issue is with the flatten tool functionality in issue 50 which would be good to scope out a fix for.

Happy to talk this through more if useful.

Bjwebb commented 8 years ago

The error is that country is in the rollUp list, but there is no field called country (only addressCountry I think).

timgdavies commented 8 years ago

Ah - got you. Yes - that does sound like an error.

Bjwebb commented 8 years ago

Pull request submitted for this: https://github.com/ThreeSixtyGiving/standard/pull/96 Flatten tool should now emit a warning if something in rollUp isn't in the schema: https://github.com/OpenDataServices/flatten-tool/commit/363995da3ad63ac04c6dfc2e0f6bddcc764ec261 (using this, it looks like country was the only instance).

Bjwebb commented 8 years ago

The rollup titles bug is now fixed in flattentool, see the commits against https://github.com/OpenDataServices/flatten-tool/issues/50 . This is now available for testing at http://flatten-tool-updates.dev.cove.opendataservices.coop/360/