fix(Android): Properly round alert end dates backwards#1654
Conversation
Alert end dates with a service day end time should always be rounded backwards rather than forwards.
| */ | ||
| fun EasternTimeInstant.formattedServiceDate(): String = | ||
| this.coerceInServiceDay().formatWith(formatMonthDay) | ||
| fun EasternTimeInstant.formattedServiceDate( |
There was a problem hiding this comment.
suggestion(non-blocking): May be worth adding to the function comment what rounding forwards & backwards means conceptually here, or adding some test cases to represent it.
Walking through coerceInServiceDay -> serviceDate(rounding) -> val serviceDate`, I'm feeling kind of confused since there is a subtraction of 1 day or 24 hours in a conditional at each level... seems like we're getting the right output from it, but going down the function call paths I lose track of what the right output is
There was a problem hiding this comment.
Yeah I can do that!
val serviceDate always rounds forward, 3:00am counts as the next service day, since you can't choose forward or backward rounding, it has to be forward. It also only includes the date component, so it's no longer an EasternTimeInstant that can be formatted.
fun serviceDate(rounding) allows you to specify whether or not you want to round back or forward, which is only relevant if the time is exactly set to the service end time. If the time is between 12-3, val serviceDate will be the previous calendar date already, and only when you want to round backwards and the time is exactly 3 would we need to subtract a day from the forward rounded val serviceDate. And this is also only giving you a LocalDate object, not the timestamp.
fun coerceInServiceDay uses fun serviceDate (with the provided rounding) to check if the instant's date matches the service date. The serviceDate result is only used for this check, not to calculate the returned instant, so if the service day doesn't match the instant's date, only then do we subtract 24 hours and return an instant with the service date we want to display. And like the comment says, the time then stops making sense, but we still need an instant for formatting purposes.
There was a problem hiding this comment.
That all is so helpful, thank you! I think the thing that was trickiest for me was that the rounding only applies to exactly 3AM, but this helped clear it up for me.
9bd52e2 to
b2d2954
Compare
Summary
Ticket: Notifications QA | Android - end date should use service day, not calendar day
Alert end dates with a service day end time should always be rounded backwards rather than forwards. It seems like this has been an issue basically forever, I'm a little surprised we haven't caught it before now.
I also checked on all the equivalent calls on iOS and they seem to be properly rounding to the previous day for all end dates, but I did update one spot to use the shared
coerceInServiceDayhelper rather than subtracting 24 hours on its own.iOS
- [ ] If you added any user-facing strings on iOS, are they included in Localizable.xcstrings?- [ ] Add temporary machine translations, marked "Needs Review"android
- [ ] All user-facing strings added to strings resource in alphabetical order- [ ] Expensive calculations are run inwithContext(Dispatchers.Default)where possible (ideally in shared code)Testing
Update a test that wasn't properly setting the end of service, all other existing tests still pass, manually checked that the alert end date shows up properly