-
Notifications
You must be signed in to change notification settings - Fork 135
[Bookings] Update type filter logic #14889
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
[Bookings] Update type filter logic #14889
Conversation
Converts the `BookingType` sealed interface into a data class containing a `Type` enum.
Moved the `BookingType.titleRes` extension property out of the `BookingTypeFilterUiState` companion object to the top level of the file.
Replaced the `value` string in `BookingFilterListItem` with a `BookingFilterListItemSubtitle` data class. This change allows the subtitle to be represented by either a direct string or a string resource ID, providing more flexibility in the UI.
Adds a subtitle to the booking filter list to show the currently selected booking type.
| data class BookingFilterListItem( | ||
| @StringRes val title: Int, | ||
| val value: String? = null, | ||
| val subtitle: BookingFilterListItemSubtitle? = null, |
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.
I renamed value to subtitle since BookingFilterListItem is a UI model, and subtitle better reflects its purpose.
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.
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.
Pull Request Overview
This PR refactors the BookingType filter option from a sealed interface with object implementations to a data class with an enum value. This change simplifies the architecture and makes the filtering logic more maintainable by removing the need for special case handling when updating filters.
Key Changes:
- Converted
BookingTypefrom a sealed interface with singleton objects to a data class containing an enum value - Standardized the filter update logic by removing special case handling for
BookingType - Refactored the UI state management to use a
selectedproperty in list items instead of comparing string values
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| BookingsFilterOption.kt | Refactored BookingType from sealed interface to data class with enum |
| BookingTypeFilterUiState.kt | Updated to use data class instances and moved extension properties to file level |
| BookingTypeFilterPage.kt | Simplified by removing redundant selectedValue parameter |
| SingleChoiceFilterPage.kt | Changed to use boolean selected property instead of string comparison |
| BookingFilterRootPage.kt | Updated subtitle handling to support both string and resource ID values |
| BookingFilterListViewModel.kt | Removed special case handling for BookingType filter updates |
| BookingFilterListUiState.kt | Added BookingType filter value resolution and subtitle data structure |
| BookingFilterListItem.kt | Added subtitle data class and selected boolean property |
Comments suppressed due to low confidence (1)
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/filter/BookingFilterListUiState.kt:72
- BookingFilterPage.BookingType is listed twice in this when expression - once at line 61 where it returns a subtitle value, and again at line 72 where it returns null. This creates unreachable code as the first match will always be used. Remove the duplicate entry at line 72.
BookingFilterPage.BookingType,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val onClick: () -> Unit = {} | ||
| ) | ||
| ) { | ||
| data class BookingFilterListItemSubtitle(val valueString: String? = null, @StringRes val valueRes: Int? = null) |
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.
The subtitle sometimes comes from a remote source, such as the customer name, and other times from a string resource, such as “Service.” That’s why I created the BookingFilterListItemSubtitle model.
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.
💡 Would it make sense to either:
- Make this a sealed class with one non-nullable value, or...
- Keep it like that, but use
UiStringinstead.
The above allows us to call BookingFilterListItemSubtitle(). That is not a correct "state". Using the above options would prevent that.
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.
I applied @AdamGrzybkowski's suggestion to BookingType.
📲 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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #14889 +/- ##
=========================================
Coverage 38.27% 38.27%
- Complexity 10101 10106 +5
=========================================
Files 2137 2137
Lines 120998 121002 +4
Branches 16579 16580 +1
=========================================
+ Hits 46309 46315 +6
+ Misses 69980 69975 -5
- Partials 4709 4712 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
I see that this PR closes the Linear issue, but to fully implement it, you need to implement the storage for the BookingType filter here.
Are you planning on doing that separately?
| val onClick: () -> Unit = {} | ||
| ) | ||
| ) { | ||
| data class BookingFilterListItemSubtitle(val valueString: String? = null, @StringRes val valueRes: Int? = null) |
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.
💡 Would it make sense to either:
- Make this a sealed class with one non-nullable value, or...
- Keep it like that, but use
UiStringinstead.
The above allows us to call BookingFilterListItemSubtitle(). That is not a correct "state". Using the above options would prevent that.
| } | ||
|
|
||
| val BookingType.filterValue: String? | ||
| val BookingType.remoteValue: String? |
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.
If this is strictly a value we would use to send via REST API, I would create this somewhere else, not in the ...UiState.
BTW. Is this property used?
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.
Actually, type filter hasn't been implemented on the backend yet. Besides, we don't even know if it's handled on the client side. So for now, I removed remoteValue from BookingTypeFilterUiState.kt. de8fc3e
Replaced the custom `BookingFilterListItemSubtitle` class with the generic `UiString` model to handle subtitle text in the bookings filter list.
Good idea! I've implemented it: fbb7d93 |
This change introduces the functionality to save and retrieve the "Booking Type" filter to and from the DataStore.
Do not increment the active filter count when the booking type filter is set to 'Any'.
I thought caching filters was a missing feature and wanted to add it in a separate PR, but it appears to be completed. So, I just added storing the booking type here: 5179aab. |
AdamGrzybkowski
left a comment
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.
Looks good!


Closes WOOMOB-1462
Description
This PR implements the logic in
BookingFilterListViewModelto display the selected type and includes some refactoring.Test Steps
Images/gif
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.