Skip to content

[WOOMOB-1797] Add Issue Refund button to booking details#15496

Merged
hichamboushaba merged 7 commits intotrunkfrom
issue/WOOMOB-1797-issue-refund-btn
Mar 10, 2026
Merged

[WOOMOB-1797] Add Issue Refund button to booking details#15496
hichamboushaba merged 7 commits intotrunkfrom
issue/WOOMOB-1797-issue-refund-btn

Conversation

@AdamGrzybkowski
Copy link
Copy Markdown
Contributor

@AdamGrzybkowski AdamGrzybkowski commented Mar 6, 2026

Description

Fixes WOOMOB-1797

Adds an "Issue Refund" button to the booking details payment section. The button appears for bookings with a Paid or Partially Refunded payment status and an associated order. Tapping it launches the existing refund flow. After a successful refund, both the booking and order data are refreshed so the UI reflects the updated payment state.

The Partially Refunded status is included to align with the behavior in the Order Details screen, where the refund option remains available until the order is fully refunded. It feels like a Booking shouldn't have a Partially Refunded status, because you can only have one product per booking, but that's how it work not, and without this, the refunding wouldn't work for orders with multiple bookings.

To support navigation back from RefundSummaryFragment to BookingDetailsFragment, a callerDestinationId argument was added to nav_graph_refunds. When non-zero, it overrides the default exit destination (orderDetailFragment), so callers outside the orders flow can reuse the refund graph without crashes.

Test Steps

Refund from Booking

  1. Open a booking with Paid payment status and an associated order
  2. Verify the "Issue Refund" button appears above "View Order"
  3. Tap "Issue Refund" → refund flow opens
  4. Complete the refund → returns to booking details, button disappears (or stays if partially refunded)
  5. Open a booking with Unpaid or Failed status → no refund button

Refund from Order

  1. Open a Paid Order
  2. Verify the refund works

Order with multiple bookings (to test partially refunded scenario)

  1. Create an order with multiple bookings in the cart
  2. Mark it as paid
  3. Open one of those bookings and issue a refund
  4. Go back to the list and refresh
  5. Confirm you see a booking with Partially Refunded badge
  6. Open it and issue a refund

Images/gif

Screen_recording_20260306_143313.mp4
  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

RefundSummaryFragment previously hardcoded R.id.orderDetailFragment
as the back navigation destination. Add a callerDestinationId argument
to nav_graph_refunds so callers can specify where to return after
completing a refund. Falls back to orderDetailFragment when unset.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an “Issue Refund” entry point to Booking Details so the existing refunds flow can be launched from bookings (not only orders), and refreshes booking/order data after a refund so the booking payment UI updates correctly.

Changes:

  • Adds an optional “Issue Refund” button in the booking payment section, exposed via BookingUiState.onIssueRefundClicked.
  • Reuses the existing refunds navigation graph from bookings by adding a callerDestinationId graph argument and using it on exit to pop back to the right screen.
  • Adds ViewModel/test logic to show/hide the refund action based on payment status + presence of an associated order, and triggers refresh after refund completion.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
WooCommerce/src/test/kotlin/com/woocommerce/android/ui/bookings/details/BookingDetailsViewModelTest.kt Adds coverage for refund CTA visibility and navigation event.
WooCommerce/src/main/res/navigation/nav_graph_refunds.xml Adds callerDestinationId argument at refunds graph level for safe reuse outside orders flow.
WooCommerce/src/main/res/navigation/nav_graph_bookings_details.xml Adds navigation action from booking details into refunds graph and includes refunds graph.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/payments/refunds/RefundSummaryFragment.kt Uses callerDestinationId (when set) to decide the exit destination on refund completion.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/details/BookingDetailsViewState.kt Adds nullable refund click callback to booking UI state.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/details/BookingDetailsViewModel.kt Determines refund CTA availability, emits navigation event, and refreshes booking/order data after refund completion.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/details/BookingDetailsScreen.kt Wires the refund callback through to the payment section UI.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/details/BookingDetailsFragment.kt Navigates to refunds flow and listens for refund completion notice to trigger refresh.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/compose/BookingPaymentSection.kt Renders the “Issue Refund” button when a callback is provided.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

</action>
</fragment>

<include app:graph="@navigation/nav_graph_refunds" />
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nav_graph_bookings_details now includes nav_graph_refunds, but the refunds graph has an action to @id/nav_graph_payment_flow (card reader flow). In tablet two-pane mode the detail pane NavHost is created with nav_graph_bookings_details only, so nav_graph_payment_flow won’t exist in that NavController and tapping a card-reader refund path from RefundSummaryFragment will crash with an unknown destination. Include nav_graph_payment_flow in this details graph as well, or route the card reader navigation through the main NavHost instead of the detail-pane controller.

Suggested change
<include app:graph="@navigation/nav_graph_refunds" />
<include app:graph="@navigation/nav_graph_refunds" />
<include app:graph="@navigation/nav_graph_payment_flow" />

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@AdamGrzybkowski AdamGrzybkowski Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any tips on how to verify that flow without the card reader? 🤔

It seems like this flow is triggered with some specific card transaction 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use a simulated card reader from the developer options of the app. But this specific flow requirs interac payments, and I'm not very familiar with them.
What I did to test this is hardcoding true for this condition, then I tested the logic, and adding the navgraph as suggested by Copilot works:

Screen_recording_20260306_181057.webm

But I wonder if adding the nav_graph to bookin_details navgaph is better, or if it's better to add it to the nav_graph_refunds as it's the one that requires it in all cases, WDYT? (I didn't test the change)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good on paper, but I'm a bit worried about opening a can of worms there. nav_grap_orders directly references destinations from nav_graph_payment_flow, same for the nav_graph_main. So they need to include it separately, and we can't simply put it inside nav_graph_refunds 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, why do we need to touch nav_grap_orders and nav_graph_main, I'm suggesting to add the graph to nav_graph_refunds without touching the parent navgraphs.

Do you think this could have an issue? I think it's similar to what we already have: the navgraph is duplicated in a parent and a nested navgraph (main and orders), and the PR also adds another level of duplication in booking_details.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I've done some tests, and it does appear to work just fine. Thanks!

@AdamGrzybkowski AdamGrzybkowski force-pushed the issue/WOOMOB-1797-issue-refund-btn branch from 4140442 to d5397b7 Compare March 6, 2026 14:00
Show an "Issue refund" button in the payment section for paid or
partially refunded bookings with an associated order. Tapping it
navigates to the existing refund flow. After completing a refund,
the booking and order data are re-fetched so the local state is
up to date for subsequent refund attempts.
Test that the refund callback is present for paid and partially
refunded bookings, null for unpaid bookings or bookings without an
order, and that clicking it triggers the NavigateToIssueRefund event
with the correct orderId.
@AdamGrzybkowski AdamGrzybkowski force-pushed the issue/WOOMOB-1797-issue-refund-btn branch from d5397b7 to aecbaa0 Compare March 6, 2026 14:03
@wpmobilebot
Copy link
Copy Markdown
Collaborator

wpmobilebot commented Mar 6, 2026

App Icon📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App NameWooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Build Number731
Version24.2.1
Application IDcom.woocommerce.android.prealpha
Commitbb4d23a
Installation URL3c6d4dqnk9080
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@hichamboushaba hichamboushaba self-assigned this Mar 6, 2026
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 6, 2026

Codecov Report

❌ Patch coverage is 27.27273% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.52%. Comparing base (e370be1) to head (bb4d23a).
⚠️ Report is 75 commits behind head on trunk.

Files with missing lines Patch % Lines
.../com/woocommerce/android/extensions/FragmentExt.kt 0.00% 13 Missing ⚠️
...droid/ui/bookings/compose/BookingPaymentSection.kt 0.00% 12 Missing ⚠️
...oid/ui/bookings/details/BookingDetailsViewModel.kt 61.11% 7 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk   #15496      +/-   ##
============================================
- Coverage     39.52%   39.52%   -0.01%     
- Complexity    11203    11208       +5     
============================================
  Files          2250     2250              
  Lines        129313   129347      +34     
  Branches      18091    18098       +7     
============================================
+ Hits          51115    51125      +10     
- Misses        72987    73011      +24     
  Partials       5211     5211              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@hichamboushaba hichamboushaba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well, I left few comments for discussing, but nothing blocker, so I'm pre-approving. Good work.

Comment on lines +125 to +131
is Exit -> {
val callerDestId = findNavController()
.getBackStackEntry(R.id.nav_graph_refunds)
.arguments?.getInt("callerDestinationId", 0) ?: 0
val destId = if (callerDestId != 0) callerDestId else R.id.orderDetailFragment
navigateBackWithNotice(REFUND_ORDER_NOTICE_KEY, destId)
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO This feels a bit hacky, it doesn't scale well, anyone who might need to integrate the flow they'll need to understand that they need to provide callerDestinationId.

WDYT about something like this patch:

Details Patch
Subject: [PATCH] Hide push notifications benefit when Woo PN system is enabled
---
Index: WooCommerce/src/main/kotlin/com/woocommerce/android/extensions/FragmentExt.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/extensions/FragmentExt.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/extensions/FragmentExt.kt
--- a/WooCommerce/src/main/kotlin/com/woocommerce/android/extensions/FragmentExt.kt	(revision 65aba04804013a4b91ccc6a5ee339329d80b56c2)
+++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/extensions/FragmentExt.kt	(date 1772818318697)
@@ -31,12 +31,16 @@
  * @param [key] A unique string that is the same as the one used in [handleResult]
  * @param [result] A result value to be returned
  * @param [destinationId] an optional destinationId, that can be used to navigate up to a specified destination
+ * @param [popDestination] whether to pop the destinationId from the back stack when navigating back, used only
+ *        when [destinationId] is specified
+ * @param [navHostId] an optional navHostId, that can be used to navigate up to a specified navHostId
  *
  */
 fun <T> Fragment.navigateBackWithResult(
     key: String,
     result: T,
     @IdRes destinationId: Int? = null,
+    popDestination: Boolean = false,
     @IdRes navHostId: Int? = null,
 ) {
     val navController = if (navHostId != null) findNavController(navHostId) else findNavController()
@@ -49,7 +53,7 @@
     entry?.savedStateHandle?.set(key, result)
 
     if (destinationId != null) {
-        findNavController().popBackStack(destinationId, false)
+        findNavController().popBackStack(destinationId, popDestination)
     } else {
         findNavController().navigateUp()
     }
@@ -63,7 +67,6 @@
  * @param [key] A unique string that is the same as the one used in [handleResult]
  * @param [result] A result value to be returned
  * @param [childId] an destinationId, that used to navigate up from the specified destination
- * @param [navHostId] An optional ID of the NavHostFragment, it's useful when the fragment is used in two-pane layouts
  */
 fun <T> Fragment.navigateToParentWithResult(key: String, result: T, @IdRes childId: Int) {
     if (findNavController().currentDestination?.id != childId) {
@@ -79,14 +82,17 @@
  *
  * @param [key] A unique string that is the same as the one used in [handleNotice]
  * @param [destinationId] an optional destinationId, that can be used to navigating up to a specified destination
+ * @param [popDestination] whether to pop the destinationId from the back stack when navigating back, used only
+ *        when [destinationId] is specified
  * @param [navHostId] An optional ID of the NavHostFragment, it's useful when the fragment is used in two-pane layouts
  */
 fun Fragment.navigateBackWithNotice(
     key: String,
     @IdRes destinationId: Int? = null,
+    popDestination: Boolean = false,
     @IdRes navHostId: Int? = null,
 ) {
-    navigateBackWithResult(key, key, destinationId, navHostId)
+    navigateBackWithResult(key, key, destinationId, popDestination, navHostId)
 }
 
 /**
Index: WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/details/BookingDetailsFragment.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/details/BookingDetailsFragment.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/details/BookingDetailsFragment.kt
--- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/details/BookingDetailsFragment.kt	(revision 65aba04804013a4b91ccc6a5ee339329d80b56c2)
+++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/details/BookingDetailsFragment.kt	(date 1772818982648)
@@ -78,8 +78,7 @@
                     findNavController().navigateSafely(
                         BookingDetailsFragmentDirections
                             .actionBookingDetailsFragmentToIssueRefund(
-                                orderId = event.orderId,
-                                callerDestinationId = R.id.bookingDetailsFragment
+                                orderId = event.orderId
                             )
                     )
                 }
Index: WooCommerce/src/main/kotlin/com/woocommerce/android/ui/payments/refunds/RefundSummaryFragment.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/payments/refunds/RefundSummaryFragment.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/payments/refunds/RefundSummaryFragment.kt
--- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/payments/refunds/RefundSummaryFragment.kt	(revision 65aba04804013a4b91ccc6a5ee339329d80b56c2)
+++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/payments/refunds/RefundSummaryFragment.kt	(date 1772819025651)
@@ -123,11 +123,11 @@
             when (event) {
                 is ShowSnackbar -> uiMessageResolver.getSnack(event.message, *event.args).show()
                 is Exit -> {
-                    val callerDestId = findNavController()
-                        .getBackStackEntry(R.id.nav_graph_refunds)
-                        .arguments?.getInt("callerDestinationId", 0) ?: 0
-                    val destId = if (callerDestId != 0) callerDestId else R.id.orderDetailFragment
-                    navigateBackWithNotice(REFUND_ORDER_NOTICE_KEY, destId)
+                    navigateBackWithNotice(
+                        key = REFUND_ORDER_NOTICE_KEY,
+                        destinationId = R.id.issueRefundFragment,
+                        popDestination = true
+                    )
                 }
                 is ShowRefundConfirmation -> {
                     val action =

The patch exposes the inclusive argument of popBackStack that we use internally, and this way the refund screen doesn't need to know where it was called, so it works for all scenarios.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea. Applied in b219181

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hichamboushaba I found a bug with this approach. The handleNotice wouldn't work correctly and the Booking wouldn't be properly refreshed after finishing the refund flow. This was fixed in bb4d23a

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 👍

</action>
</fragment>

<include app:graph="@navigation/nav_graph_refunds" />
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use a simulated card reader from the developer options of the app. But this specific flow requirs interac payments, and I'm not very familiar with them.
What I did to test this is hardcoding true for this condition, then I tested the logic, and adding the navgraph as suggested by Copilot works:

Screen_recording_20260306_181057.webm

But I wonder if adding the nav_graph to bookin_details navgaph is better, or if it's better to add it to the nav_graph_refunds as it's the one that requires it in all cases, WDYT? (I didn't test the change)

Instead of passing a caller destination ID through the refund nav graph,
use popBackStack's inclusive parameter to pop the issueRefundFragment,
which removes the entire refund graph from the back stack regardless of
the caller. This makes the refund flow caller-agnostic and scales to any
future integrations.
@AdamGrzybkowski AdamGrzybkowski force-pushed the issue/WOOMOB-1797-issue-refund-btn branch from 359e517 to b219181 Compare March 9, 2026 08:09
The refund flow always needs the payment flow graph (for Interac card
reader refunds), so include it directly in nav_graph_refunds rather
than requiring every parent graph to remember to include it separately.
When popDestination is true, the notice was set on the destination
entry's savedStateHandle before that entry was popped (destroyed).
The receiving fragment observes currentBackStackEntry which is a
different entry, so the notice was never delivered.

Fix by reversing the order: pop first, then set the result on the
new currentBackStackEntry which is the actual receiver.

Also use the navController variable consistently instead of calling
findNavController() again on the pop line.
@AdamGrzybkowski
Copy link
Copy Markdown
Contributor Author

@hichamboushaba I've applied your suggestion. It worked fine on my end, but I would appriciate some extra testing from you.

Additionally, the fix for the bug mentioned in #15496 (comment) is also something that requires another look from you.

Copy link
Copy Markdown
Member

@hichamboushaba hichamboushaba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @AdamGrzybkowski, thanks for addressing my comments.

@hichamboushaba hichamboushaba merged commit 2c2b71e into trunk Mar 10, 2026
16 checks passed
@hichamboushaba hichamboushaba deleted the issue/WOOMOB-1797-issue-refund-btn branch March 10, 2026 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants