search

ckan / ckanext-dcat

CKAN ♥ DCAT
163 stars 142 forks source link

Map existing resources based on their URI #94

Closed metaodi closed 6 years ago

metaodi commented 6 years ago

Fixes #91

metaodi commented 6 years ago

This PR also fixes the controller tests, which now require a test_request_context (see https://github.com/ckan/ckan/pull/3228). This is unrelated, but the build fails otherwise.

metaodi commented 6 years ago

@amercader When writing tests I noticed something strange: if the URI of a resource is empty, it is saved with the string "None" is CKAN. I guess it's because it gets explicitly assigned the value None. I guess later on this is being serialized using str() or something similar, which then creates the string "None".

In this PR, I now changed the value from None to an empty string. I think this is actually a very good solution, as you end up with an empty string in CKAN and all the conditionals (if value etc.) still work as the empty string is falsy in Python. WDYT?

The other solution would be to change to serialization, and make sure None values are saved as null or an empty string.

It probably would make sense to implement this consequently, as we have now empty strings and None values.

metaodi commented 6 years ago

See https://github.com/ckan/ckan/issues/3780 for backwards-compatible test_request_context

metaodi commented 6 years ago

ping @amercader

metaodi commented 6 years ago

So I just removed my changed for the test_request_context since https://github.com/ckan/ckan/pull/3782 has been merged.

metaodi commented 6 years ago

@amercader do you have time to review this PR?

amercader commented 6 years ago

sorry @metaodi I've been a bit swamped recently. This looks great. What's the reason behind 31042e69?

metaodi commented 6 years ago

@amercader see my comment above, let me know if you need more information.

amercader commented 6 years ago

Sorry @metaodi, should have read first. Your approach sounds good, we should definitely consolidate the behaviour in the serialization code but that can be done at another time.

Shall we change this and this as well for consistency?

metaodi commented 6 years ago

@amercader okay I updated the other URIs as well, and to be consistent I updated _object_value to return an empty string if the value is not found, e.g. _contact_details() should return an empty string for all attributes not only uri, do you agree?

amercader commented 6 years ago

Sounds good @metaodi, many thanks for this!

camfindlay commented 6 years ago

Just checking - does this apply to the DCAT JSON Harvester or will that need to implemented still?

amercader commented 6 years ago

@camfindlay the DCAT JSON harvester hasn't been updated in a long time. Current efforts are focused on the RDF based functionality.

camfindlay commented 6 years ago

thanks @amercader - we're making use of DCAT JSON so take it if we'd like this same improvement we'll need to raise a PR and add a similar check to avoid harvesting when resources haven't changed. ^ CC/ @dythya