-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use ISO datetime format #1642
Use ISO datetime format #1642
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, congratulations to your first PR and thanks a lot! 🎉 🚀
First of all: I'm sorry I labeled this issue as "good first issue" - it was definitely not 😉
I am all the more impressed that you have fought your way through it. 💪
In general, everything seems to work as expected 👍
I see two major points that should be fixed before merging this PR:
- We need to migrate our existing event data, otherwise all start and end dates will be reset to the point in time when we install this update on our server.
- We should also adapt the event list templates to use the same new fields instead of setting custom attributes.
If you have any troubles implementing these changes, don't hesitate to ask!
@digitalfabrik/app-team FYI We are currently implementing your required ISO format :) |
Code Climate has analyzed commit 4465f01 and detected 1 issue on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 75.6% (50% is the threshold). This pull request will bring the total coverage in the repository to 74.3%. View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing, thanks! 🙏
I have to admit that I misunderstood how Django handles timezones 😅
Even if dates are given in timezone aware local times, Django converts them to UTC and does not keep track of the original timezone when writing them to the database. So every time we retrieve a date from the database, we have to convert it to the local time in the region's timezone before we either display it in the CMS or deliver it via the API.
This makes everything a bit more complicated, I hope I did not mess up my suggestions.
I think that we also have to update the expected test results as well to match the timezone. I think that part is still missing, or? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks a lot! 💪
Now there is not too much left to do I think. After you fixed the last issues and adapted the tests, could you clean up the git history and rebase the branch? After that, I will test everything extensively again 😁
Timezones overall seem to be huge pain in the neck
Yes, they give me quite a headache as well 😅
I think that we also have to update the expected test results as well to match the timezone. I think that part is still missing, or?
Yes, I didn't make suggestions for this part because it's probably easier to just retrieve the endpoints and copy + paste the result into the files instead of replacing the dates manually...
Yes sure. But I´m not sure when I will get to it since I am going to Munich today. Maybe I will find time and space in the train. In any other case, I should have time for it sometime next week |
- Added the datetime field start and end to the event model, removed start/end time and date fields - Adjusted all event forms and templates to the new event model fields - Made sure that the displayed datetimes are given in the timezone of the event - Added start and end fields to API events endpoint - Added last_updated field in every API endpoint that contains a last_modified or timestamp field - Adjusted the test data and expected results to match the new API output and required model input
d4b9d43
to
4465f01
Compare
I completed the rebase and everything seems to work fine. I squashed the commits into 1 and added one slightly longer commit description. @timoludwig if you find something while re-checking everything, let me know. I added the change in the unreleased block inside the changelog, I hope this is fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thank you so much! 🙏
Works flawlessly 😍
Short description
Adjusted API datetime field to match an ISO standard
Proposed changes
Resolved issues
Fixes: #1011