-
Notifications
You must be signed in to change notification settings - Fork 136
[POS - Historical Orders] Refresh orders after the receipt was sent. #14805
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
[POS - Historical Orders] Refresh orders after the receipt was sent. #14805
Conversation
| onSendReceiptClicked = { viewModel.onUIEvent(WooPosEmailReceiptUIEvent.SendEmailClicked) }, | ||
| onBackClicked = { onNavigationEvent(WooPosNavigationEvent.GoBack) }, | ||
| onEmailSent = { onNavigationEvent(WooPosNavigationEvent.GoBack) } | ||
| onEmailSent = { onNavigationEvent(WooPosNavigationEvent.GoBackWithResult(key = EMAIL_RECEIPT_SENT, true)) } |
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 was wondering if this logic needs to be in the view model, but given that we already have the onEmailSent callback and we don't add more model knowledge to the screen, I left it there.
| popBackStack() | ||
| } | ||
|
|
||
| is WooPosNavigationEvent.GoBackWithResult -> { |
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 considered adding a specific event like GoBackAfterReceiptSent, but I’d rather keep this generic so it’s reusable across flows. Using GoBackWithResult keeps the navigation API abstract and avoids event proliferation (“one event per case”).
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
| if (shouldRefresh) { | ||
| viewModel.onRefresh() | ||
| // clear so it doesn't retrigger on recomposition / process death restore | ||
| entry.savedStateHandle[EMAIL_RECEIPT_REFRESH_ORDERS] = false |
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.
Probably we should remove this from the bundel, not set to false?
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.
Yeah that's better, done in 02c24c8
| import com.woocommerce.android.ui.woopos.common.composeui.designsystem.WooPosTypography | ||
| import com.woocommerce.android.ui.woopos.root.navigation.WooPosNavigationEvent | ||
|
|
||
| const val EMAIL_RECEIPT_SENT = "email_receipt_sent" |
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 would consider placing this in the navigation because I believe it’s semantically related to 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.
That's true, done in ddf806d
| val viewModel: WooPosOrdersViewModel = hiltViewModel() | ||
| val state by viewModel.state.collectAsState() | ||
|
|
||
| val entry = remember(navController) { navController.currentBackStackEntry } |
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.
Have you considered not passing navController inside the screen but passing the value to the screen from navigation composable? Both, I think, are fine, but separating the navigation part from the actual screen may be a bit cleaner.
Example:
@Composable
fun MyNavHost() {
val navController = rememberNavController()
NavHost(navController = navController, startDestination = "screenA") {
composable("screenA") { backStackEntry ->
val result = backStackEntry.savedStateHandle
.getStateFlow<String?>("selected_item", null)
.collectAsState()
ScreenA(
selectedItem = result.value,
onNavigateToScreenB = {
navController.navigate("screenB")
}
)
}
composable("screenB") {
ScreenB(
onItemSelected = { item ->
navController.previousBackStackEntry
?.savedStateHandle
?.set("selected_item", item)
navController.popBackStack()
}
)
}
}
}
@Composable
fun ScreenA(selectedItem: String?, onNavigateToScreenB: () -> Unit) {
Column {
Text("Selected: ${selectedItem ?: "None"}")
Button(onClick = onNavigateToScreenB) {
Text("Go to Screen B")
}
}
}
@Composable
fun ScreenB(onItemSelected: (String) -> Unit) {
Column {
Button(onClick = { onItemSelected("Item 1") }) {
Text("Select Item 1")
}
Button(onClick = { onItemSelected("Item 2") }) {
Text("Select Item 2")
}
}
}
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.
Yeah, it's cleaner that way, done in 68e10c3
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
| import com.woocommerce.android.ui.woopos.root.navigation.WooPosNavigationEvent | ||
| import com.woocommerce.android.ui.woopos.root.navigation.navigateOnce | ||
|
|
||
| const val EMAIL_RECEIPT_SENT = "email_receipt_sent" |
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.
np: I'd recommend naming it EMAIL_RECEIPT_SENT_KEY - for clarity
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #14805 +/- ##
============================================
+ Coverage 38.25% 38.26% +0.01%
- Complexity 10080 10088 +8
============================================
Files 2132 2132
Lines 120683 120735 +52
Branches 16543 16556 +13
============================================
+ Hits 46170 46202 +32
- Misses 69831 69843 +12
- Partials 4682 4690 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kidinov
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.
The approach LGTM! But:
- Notice that it doesn't work and the email is not actually appeared neither on the list or in the order details
- The selection of the order is lost
- I doubt that we need to show the PTR indicator. Check the iOS UX for reference.
Also, there is an issue not related to the changes in the PR inWooPosEmailReceiptRepository::sendReceiptByEmail there is an assumption that the order is stored in the database, which may not be the case when you open the POS after a clean install; therefore, sending fails. So:
- We either need to take the order from the in-memory cache, but correct me if I am wrong, it contains just 1 page of orders at max
- We need to fetch the order from the remote if it's not in the database.
|
Thanks for the comment @kidinov!
After merging from trunk, the order details are now displayed correctly. On iOS, the selected order is reloaded directly from the Email Receipt screen, but in our case, I’ve kept that logic within the POS Orders screen. This approach has the added benefit of making the email receipt action faster. In this PR: Let me know what you think.
Good catch! Let's tackle that in a separate PR 👍 |
Done in #14817 |
…fresh-after # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersScreen.kt
| suspend fun loadMore(searchQuery: String? = null): Result<List<Order>> = | ||
| withContext(Dispatchers.IO) { loadNextPage(searchQuery) } | ||
|
|
||
| suspend fun refreshOrderById(orderId: Long): Result<Order> { |
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.
You don't update the cache, so if I understand correctly, after returning back to this screen, the order will be displayed without the updated email as cached data firstly 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.
Good point, fixed it in 9052df4
| } | ||
| } | ||
|
|
||
| fun onBackFromSuccesfullySendingEmailReceipt() { |
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 is a typo in succesfully
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.
Good catch! Done in 7f696e1
| Spacer(modifier = Modifier.height(WooPosSpacing.Small.value)) | ||
|
|
||
| AnimatedVisibility(visible = state.isRefreshingSelectedDetails) { | ||
| LinearProgressIndicator( |
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.
We don't have a Woo POS-specific linear progress component, and the default one uses colors that do not match the POS colors or style. I'd suggest not having any loading element at all. It'll simplify the code and won't bother merchants with not really clear what and why reloading indicator
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.
Nice, let's remove it then. Done in 7ca6800
| } | ||
| } | ||
|
|
||
| private suspend fun applyOrderUpdate(updated: Order) { |
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.
Probably can be simplified to
private suspend fun applyOrderUpdate(updated: Order) {
val current = _state.value as? WooPosOrdersState.Content ?: return
val loaded = current.items as? WooPosOrdersState.Content.Items.Loaded ?: return
val selectedId = loaded.items.keys.firstOrNull { it.isSelected }?.id
val newItem = mapOrderItem(updated, selectedId)
val newDetails = mapOrderDetails(updated)
val newMap = loaded.items.entries.associate { (item, details) ->
if (item.id == updated.id) newItem to newDetails else item to details
}
_state.value = current.copy(
items = WooPosOrdersState.Content.Items.Loaded(newMap),
selectedDetails = if (selectedId == updated.id) newDetails else current.selectedDetails
)
}
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.
Thanks, changed in e829d88
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.
It looks good to me; I'd just really consider removing the loading indicator, as this is more of a side effect of the temporary architecture we are currently using, and I think it's fine to not show it at the moment. If you think the loading indicator is needed, I think we need to consider using something else, not the one currently used, as I think it's not good for the current POS design language.
Also, there are a few minor suggestions - please take a look!
…fresh-after # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/root/navigation/WooPosNavigationEventHandler.kt
Description
With this PR we refresh the selected POS order after the user sends the email recept and they are navigated back. We do so by passing a parameter to the back navigation event that is then read by the POS orders navigation logic.
Test Steps
Verify also that when the email is not sent, i.e. by tapping on the back button, the orders aren't refreshed.
Images/gif
Screen_Recording_20251024_112801_Woo.Dev.mp4
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.