[WOOMOB-1515] - Format booking duration following web's logic #14760
Conversation
There was a problem hiding this comment.
Pull Request Overview
Updates booking duration formatting in the mobile app to match the web application's logic, implementing duration normalization and human-readable formatting.
- Replaced simple "X min" formatting with comprehensive duration display (days, hours, minutes, seconds)
- Added duration normalization to handle precision issues by rounding up durations within one minute of common boundaries
- Introduced localized string resources for proper pluralization of time units
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| BookingMapper.kt | Implements new duration formatting logic with normalization and human-readable output |
| strings.xml | Adds localized string resources for time unit pluralization |
| BookingMapperTest.kt | Adds comprehensive test coverage for duration formatting scenarios |
| BookingListViewModelTest.kt | Updates test setup to inject ResourceProvider dependency |
| BookingDetailsViewModelTest.kt | Updates test setup and adds duration string stubbing for exact days |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @Suppress("LongMethod") | ||
| private fun Duration.toHumanReadableFormat(): String { |
There was a problem hiding this comment.
[nitpick] The toHumanReadableFormat() method is quite long and handles multiple formatting scenarios. Consider breaking it down into smaller, more focused methods for each time unit range (formatDaysHoursMinutes, formatHoursMinutes, formatMinutes, formatSeconds) to improve readability and maintainability.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I think AI has a point here, the method is quite long, and the multiple branches complicate the code a bit.
I'm suggesting a simpler version below, please check it out:
private fun Duration.toHumanReadableFormat(): String {
if (this < Duration.ofMinutes(1)) {
return resourceProvider.getQuantityString(
quantity = seconds.toInt(),
default = R.string.booking_duration_seconds,
one = R.string.booking_duration_second
)
}
val days = toDays()
val hours = minusDays(days).toHours()
val minutes = minusDays(days).minusHours(hours).toMinutes()
return buildString {
if (days > 0) {
append(
resourceProvider.getQuantityString(
quantity = days.toInt(),
default = R.string.booking_duration_days,
one = R.string.booking_duration_day
)
)
}
if (hours > 0) {
append(" ")
append(
resourceProvider.getQuantityString(
quantity = hours.toInt(),
default = R.string.booking_duration_hours,
one = R.string.booking_duration_hour
)
)
}
if (minutes > 0) {
append(" ")
append(
resourceProvider.getQuantityString(
quantity = minutes.toInt(),
default = R.string.booking_duration_minutes,
one = R.string.booking_duration_minute
)
)
}
}.trim()
}This gives the same results, and the tests are green.
But please feel free to ignore the suggestion if you prefer the other version 🙂
| val remainder = durationInSeconds % boundary | ||
| val difference = if (remainder == 0L) 0L else boundary - remainder | ||
| if (difference > 0 && difference <= minuteInSeconds) { | ||
| durationInSeconds += difference | ||
| } | ||
| } | ||
| return Duration.ofSeconds(durationInSeconds) | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] The normalization logic could be simplified by extracting the boundary checking into a separate function. This would make the algorithm clearer and more testable.
| val remainder = durationInSeconds % boundary | |
| val difference = if (remainder == 0L) 0L else boundary - remainder | |
| if (difference > 0 && difference <= minuteInSeconds) { | |
| durationInSeconds += difference | |
| } | |
| } | |
| return Duration.ofSeconds(durationInSeconds) | |
| } | |
| durationInSeconds = adjustToBoundary(durationInSeconds, boundary, minuteInSeconds) | |
| } | |
| return Duration.ofSeconds(durationInSeconds) | |
| } | |
| private fun adjustToBoundary(durationInSeconds: Long, boundary: Long, minuteInSeconds: Long): Long { | |
| val remainder = durationInSeconds % boundary | |
| val difference = if (remainder == 0L) 0L else boundary - remainder | |
| return if (difference > 0 && difference <= minuteInSeconds) { | |
| durationInSeconds + difference | |
| } else { | |
| durationInSeconds | |
| } | |
| } |
Copilot uses AI. Check for mistakes.
| val hoursPart = resourceProvider.getQuantityString( | ||
| quantity = hours, | ||
| default = R.string.booking_duration_hours, | ||
| one = R.string.booking_duration_hour | ||
| ) |
There was a problem hiding this comment.
I'm not sure this is enough for other languages that we support, but from what I've checked, this is how we do plurals here.
For example, that wouldn't work for Polish, but we don't support Polish, so maybe that's fine.
There was a problem hiding this comment.
Yes, our plurals support is quite limited, a limitation of Glotpress, this won't work well for Arabic too, but I don't think we have much to do here.
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## trunk #14760 +/- ##
============================================
+ Coverage 38.00% 38.03% +0.03%
- Complexity 9960 9972 +12
============================================
Files 2118 2118
Lines 119121 119188 +67
Branches 16224 16233 +9
============================================
+ Hits 45267 45334 +67
Misses 69287 69287
Partials 4567 4567 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AdamGrzybkowski
force-pushed
the
issue/WOOMOB-1515_booking_duration
branch
from
f8ef79f to
8645155
Compare
|
Version |
There was a problem hiding this comment.
Really nice work @AdamGrzybkowski.
I left some comments but nothing is blocking.
And just a question: how do you feel about moving the formatting logic to a separate class? I feel like it's something that can be used elsewhere if we ever use Duration.
| val hoursPart = resourceProvider.getQuantityString( | ||
| quantity = hours, | ||
| default = R.string.booking_duration_hours, | ||
| one = R.string.booking_duration_hour | ||
| ) |
There was a problem hiding this comment.
Yes, our plurals support is quite limited, a limitation of Glotpress, this won't work well for Arabic too, but I don't think we have much to do here.
| val hours = ((totalSeconds % dayInSeconds) / hourInSeconds).toInt() | ||
| val minutes = ((totalSeconds % hourInSeconds) / minuteInSeconds).toInt() | ||
|
|
||
| val parts = mutableListOf<String>() |
There was a problem hiding this comment.
np, we can probably use buildString here instead of a mutableListOf<String>, for example for this first formatting logic:
buildString {
append(
resourceProvider.getQuantityString(
quantity = days,
default = R.string.booking_duration_days,
one = R.string.booking_duration_day
)
)
if (hours > 0) {
append(" ")
append(
resourceProvider.getQuantityString(
quantity = hours,
default = R.string.booking_duration_hours,
one = R.string.booking_duration_hour
)
)
}
if (minutes > 0) {
append(" ")
append(
resourceProvider.getQuantityString(
quantity = minutes,
default = R.string.booking_duration_minutes,
one = R.string.booking_duration_minute
)
)
}
}| @Suppress("LongMethod") | ||
| private fun Duration.toHumanReadableFormat(): String { |
There was a problem hiding this comment.
I think AI has a point here, the method is quite long, and the multiple branches complicate the code a bit.
I'm suggesting a simpler version below, please check it out:
private fun Duration.toHumanReadableFormat(): String {
if (this < Duration.ofMinutes(1)) {
return resourceProvider.getQuantityString(
quantity = seconds.toInt(),
default = R.string.booking_duration_seconds,
one = R.string.booking_duration_second
)
}
val days = toDays()
val hours = minusDays(days).toHours()
val minutes = minusDays(days).minusHours(hours).toMinutes()
return buildString {
if (days > 0) {
append(
resourceProvider.getQuantityString(
quantity = days.toInt(),
default = R.string.booking_duration_days,
one = R.string.booking_duration_day
)
)
}
if (hours > 0) {
append(" ")
append(
resourceProvider.getQuantityString(
quantity = hours.toInt(),
default = R.string.booking_duration_hours,
one = R.string.booking_duration_hour
)
)
}
if (minutes > 0) {
append(" ")
append(
resourceProvider.getQuantityString(
quantity = minutes.toInt(),
default = R.string.booking_duration_minutes,
one = R.string.booking_duration_minute
)
)
}
}.trim()
}This gives the same results, and the tests are green.
But please feel free to ignore the suggestion if you prefer the other version 🙂
4 participants
WOOMOB-1515
Description
This PR updates the booking duration formatting to follow the same logic as the web.
Steps to reproduce
Testing information
The tests that have been performed
The above.
Images/gif
Without the normalization, this would show
1 day 23 hours 59 minutesRELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.