3619: Use correct timezone for poi timeslots #3731
Conversation
FreshtaM
requested review from
LeandraH,
bahaaTuffaha,
f1sh1918,
lunars97 and
steffenkleinle
as code owners
steffenkleinle
changed the title
steffenkleinle
changed the title
There was a problem hiding this comment.
Thanks for the PR, this looks really good already and works as expected (tested on firefox). However, please make sure not to do unrelated changes if there is no specific reason for it. Also, there are currently still some issues with prettier, linting and ts checks. Please try to resolve them :)
Another note: It is usually a good idea to stick to the PR template and not copy most of the issue description here.
shared/api/models/PoiModel.ts
Outdated
| get isCurrentlyOpen(): boolean { | ||
| if (!this.openingHours) return false | ||
|
|
||
| const now = DateTime.now() | ||
| const weekdayIndex = now.weekday - 1 | ||
| const today = this.openingHours[weekdayIndex] | ||
|
|
||
| if (!today) return false | ||
| if (today.allDay) return true | ||
|
|
||
| return today.timeSlots.some(timeslot => { | ||
| const zone = timeslot.timezone ?? 'UTC' | ||
| const start = DateTime.fromFormat(timeslot.start, 'HH:mm', { zone }) | ||
| const end = DateTime.fromFormat(timeslot.end, 'HH:mm', { zone }) | ||
|
|
||
| return Interval.fromDateTimes(start, end).contains(now.setZone(zone)) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🔧 Please revert unrelated changes, except they were done on purpose. Why did you choose to rename some of the variables here?
There was a problem hiding this comment.
Hello thank you for your review
the unrelated code has been reverted .
shared/api/models/PoiModel.ts
Outdated
| const start = DateTime.fromFormat(timeslot.start, 'HH:mm', { zone }) | ||
| const end = DateTime.fromFormat(timeslot.end, 'HH:mm', { zone }) | ||
|
|
||
| return Interval.fromDateTimes(start, end).contains(now.setZone(zone)) |
There was a problem hiding this comment.
❓ Is it really necessary to convert now to the same time zone?
| timeSlots: hours.timeSlots.map(timeslot => ({ | ||
| start: timeslot.start, | ||
| end: timeslot.end, | ||
| timezone: (timeslot as { timezone: string }).timezone ? (timeslot as { timezone: string }).timezone : undefined, |
There was a problem hiding this comment.
This looks redundant.
| openAllDay: boolean | ||
| closedAllDay: boolean | ||
| timeSlots: { start: string; end: string }[] | ||
| timeSlots: { start: string; end: string }[] | { start: string; end: string; timezone: string }[] |
There was a problem hiding this comment.
I think it should be optional instead of or operator?
timeSlots: { start: string; end: string; timezone?: string }[]| timeSlots: hours.timeSlots.map(timeslot => ({ | ||
| start: timeslot.start, | ||
| end: timeslot.end, | ||
| timezone: timeslot.timezone ?? undefined, |
There was a problem hiding this comment.
why add the nullish thing if it's already string | undefined
| timezone: timeslot.timezone ?? undefined, | |
| timezone: timeslot.timezone, |
| this._organization = organization | ||
| this._barrierFree = barrierFree | ||
| } | ||
|
|
There was a problem hiding this comment.
🔧 Please revert these unnecessary blank line deletion/assertion.
Short Description
Added correct_time_zone for POI and related to this issue. Changed 2 files:
DatabaseConnector.tsandPoiModel.ts.PR labels:
nativeandshared.Proposed Changes
PoiModel.tsto use timezone forisCurrentlyOpen.DatabaseConnector.tsto map timezone from CMS.Side Effects
None expected. Only POI opening hours behavior is affected.
Testing
isCurrentlyOpennow correctly reflects POI status in different timezones.nullor missing timezone.Resolved Issues
Fixes: #3619