Fixes #1690. Added status call on api base url returning 404, but sti…

theodi / open-data-certificate

The mark of quality and trust for open data

https://certificates.theodi.org/

MIT License

46 stars 39 forks source link

Fixes #1690. Added status call on api base url returning 404, but sti… #1691

Closed ghost closed 6 years ago

ghost commented 6 years ago

…ll with a valid api as is the vanilla ckan behaviour. Reformatted. Unit test added.

This PR fixes #1690

Changes proposed in this pull request:

Add status call which exists in vanilla ckan api, containing attribute 'ckan_version'
Enables successful validation of valid api which does not return response on call to base api
Added unit test(s)

ghost commented 6 years ago

Hi @olivierthereaux Please review when you can.

olivierthereaux commented 6 years ago

Apologies for the delay Matt - I was away. Will have a look.

olivierthereaux commented 6 years ago

Looks good as far as I can tell. Just one question @mattRedBox - is there any good documentation (or UI) on how the expected URI will be e.g. https://demo.ckan.org/api/ (rather than https://demo.ckan.org/)? Otherwise it may be good to add heuristics to "add" the missing /api WKL.

ghost commented 6 years ago

Hi @olivierthereaux I may have misunderstood your comment, but as far as I can see, no there's no doco about it really. Before I had dived into the code, the only clue as to guessing was on the page itself:

However the code itself doesn't really care about the URL as such, only that the response is application/json with the key: "version". So a person's attempt to enter a URL, won't stop the app attempting to fetch that response, if that makes sense?

I guess there's nothing to stop a person trying with or without '/api'

olivierthereaux commented 6 years ago

Thanks Matt - that bit of UI was what I was trying to remember/find.

olivierthereaux commented 6 years ago

Noticed that auto-deploy to https://staging.certificates.theodi.org/ wasn't on. Now fixed.