Fix/standardise dataset times #61
Open
+308
−48
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bellisk
force-pushed
the
fix/standardise-dataset-times
branch
from
79be690
to
8b3cacb
Compare
bellisk
force-pushed
the
fix/standardise-dataset-times
branch
4 times, most recently
from
8478675
to
7137785
Compare
The CKAN templates need UTC datetimes with no time zone info. The API wants datetimes in the Europe/Zurich time zone, with time zone info. For other cases when package_show is called, we don't want to reformat the datetimes at all: it is called a lot internally when updating resources, etc.
Also remove debug statements.
Handle cases where the dataset/resource dict doesn't have a particular field.
swiss_date was set as an output validator for the 'modified' and 'issued' fields on datasets and resources. Since it was called on output, not on creating/updating datasets, the errors it added were never logged anywhere and didn't actually change anything. Furthermore, it was validating that values were dates, when in fact we save 'modified' and 'issued' values as isoformat datetime strings.
bellisk
force-pushed
the
fix/standardise-dataset-times
branch
from
483c5a4
to
39d976e
Compare
This is to make it easier to tell apart which datetimes in the test are custom fields and which are CKAN default fields.
stefina
reviewed
May 29, 2024
| if dt is False: | ||
| continue | ||
|
|
||
| dt_zh = dt.replace(tzinfo=ZURICH) |
There was a problem hiding this comment.
What if the tzinfo is already Zurich? Do you think it is worthwhile to check first, if the data is set so we do not have to do it each time or is it likely, that we will get those dates often, even after this code has been in place for a while?
| continue | ||
|
|
||
| dt_utc = dt.replace(tzinfo=UTC) | ||
| dt_zh = dt_utc.astimezone(ZURICH) |
There was a problem hiding this comment.
Same as above. I don't think it should hurt us a lot, but as it is more than one parameter we are changing every time, this might be worth checking?
| if path.startswith("/api") and not path.startswith("/api/action"): | ||
| # The API client for CKAN's JS modules uses a path starting | ||
| # /api/action, i.e. without a version number. All other API calls | ||
| # should include a version number. |
There was a problem hiding this comment.
TIL, I assumed that since a few version it is allowed to leave the version out of an standard API call as well, but just tested it with one of our projects and indeed you have to have the version in there, still.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.