- Issue created by @mandclu
- 🇩🇪Germany jurgenhaas Gottmadingen
I've reviewed the code, and it looks good to me. The only suggestions for the UpdateController is about the
if ($new_data['allDay'] || ($empty_end && $timezone && !$this->convertTzs)) {
statement that we have twice. Maybe that could be delegated into aneedsTzRemap
method? That would help for maintenance so that we never forget to update all instances, if we ever need to update or enhance that block.Do you think I should also test this on a Drupal site?
- 🇨🇦Canada mandclu
TBH I struggled with this question of the not-so-DRY repetition of checks myself. It seemed like a helper method would need to passed all those same values only to return a boolean based on a pretty simple combination, so I ultimately decided it wasn't worth the effort, and instead kept the relevant four lines of code identical, so updating both at once is trivial, at least.
Theoretically, earlier in the code I could also combine
$empty_end
and!$this->convertTzs
into a new boolean value, so at least the repeated code is a little cleaner.If you have the ability test this on a Drupal site, a fresh set of eyes would definitely be likely to catch edge cases I missed.
The drag-and-drop changes I tested were:
- Drag an in-day event to a different start time
- Resize an in-day event to a different duration
- Drag an in-day event to the all-day header to make it all day
- Drag an all-day event into the day grid to make it in-day
- Drag an all-day event between days
- Resize an all-day event to make it span more (or fewer) days
I also tried using events that were the same TZ as the user, events that were the site default (with the user TZ different), event in different timezones, etc.
- 🇩🇪Germany jurgenhaas Gottmadingen
Sure, that helper function is not a blocker. I only had a hard time to verify in my brain if those conditions with ANDs and ORs are really doing the same thing. We can certainly keep that as is.
Now, I started testing, and the calendar shows a false result. Here is the setup:
- The site's TZ is Berlin, UTC+2
- The first event is at 4pm in Berlin's TZ
- The second event is at 4pm in London's TZ, UTC+1
The calendar shows the first event at 6pm and the second at 5pm. So, the second is correct but the first is wrong, it should be at 4pm.
I need to go to a meeting now but thought I leave that simple test here. Maybe you have a chance to look into it.
- 🇩🇪Germany jurgenhaas Gottmadingen
Sorry for the noise, I fooled myself completely. I had created the first event with the default timezone, and only later realized that the default was UTC. I then changed the default TZ to Berlin and that shifted the first event back to the new time, which was entirely correct.
So, I've tested this all again with proper settings and I can confirm that all the use cases are working correctly. Very nice work indeed. What an impressive solution that is.
Automatically closed - issue fixed for 2 weeks with no activity.