[Shipping labels] Request label refund network

[irfano]

Part of WOOMOB-370

Description

This implements network integration for the Refund Request screen.

Warning

Known issues:

• The condition for the visibility of the "Request refund" button will be handled in a separate PR.

Steps to reproduce

Successful case

1. Open the Orders screen.
2. Select an order that includes a purchased shipment.
3. Tap the "Request refund" button.
4. Tap the "Refund label" button

Testing information

• Test tapping back while the refund is in progress.
• Test the failure case. You can do this by attempting to refund a shipment that has already been refunded. Since the visibility condition for the refund button is currently broken, you can take advantage of that to test the failure case.
• Test refunding a shipment after a successful purchase.

The tests that have been performed

Steps above.

Images/gif

• 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.

[wpmobilebot]

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

	App Name	WooCommerce-Wear Android
	Platform	⌚️ Wear OS
	Flavor	Jalapeno
	Build Type	Debug
	Commit	ec27f71
	Direct Download	woocommerce-wear-prototype-build-pr14171-ec27f71.apk

[wpmobilebot]

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

	App Name	WooCommerce Android
	Platform	📱 Mobile
	Flavor	Jalapeno
	Build Type	Debug
	Commit	ec27f71
	Direct Download	woocommerce-prototype-build-pr14171-ec27f71.apk

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 40.90909% with 13 lines in your changes missing coverage. Please review.

Project coverage is 37.75%. Comparing base (d86e726) to head (ec27f71).
Report is 189 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...inglabels/networking/WooShippingLabelRestClient.kt	0.00%	7 Missing ⚠️
...nglabels/refund/WooShippingLabelRefundViewModel.kt	61.53%	3 Missing and 2 partials ⚠️
...inglabels/networking/WooShippingLabelRepository.kt	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk   #14171      +/-   ##
============================================
+ Coverage     37.74%   37.75%   +0.01%     
- Complexity     8952     8956       +4     
============================================
  Files          1955     1955              
  Lines        109520   109542      +22     
  Branches      14364    14365       +1     
============================================
+ Hits          41334    41363      +29     
+ Misses        64427    64416      -11     
- Partials       3759     3763       +4     

☔ 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.

[hichamboushaba]

Thanks @irfano, this works well, I left some comments, and I'll explain the currency issue with more details below:

I see that we currently use the order.currency for formatting of the shipping label rates and refund, which is problematic, as the order could be created using a different currency (either when using a multi-currency plugin, or just by changing Woo settings), and this issue results in the following:

I think we should use the currency from the StoreOptionsModel and for refund another option is to use the currency that we get from the endpoint /wcshipping/v1/label/purchase/<order-id> (that's what was done in the old flow), anyway, I think each approach is fine.

Given this issue is present in other places of the flow, I'm pre-approving the PR, as I think it should be addressed in a separate PR.

[hichamboushaba]

np, I'm not sure using getOrNull() in this cases is needed, IMO it just complicates the code with nullability handling. When the user reaches this step, we are sure there is a selected site, so it should be fine to use get()

[irfano]

Makes sense! Done: ec27f71

[hichamboushaba]

np, WDYT about passing just what's needed and not the whole ShipmentUIModel? this will have the advantage of avoiding nullable items as we can check for null items before triggering the navigation.

This is also related to a potential issue with the currency usage that we have in this screen and other places, I'll explain this further in the review itself.

[irfano]

In my first attempt, I used separate parameters, but then I noticed I'm already using items, purchaseDate, refundableAmount and labelId from ShipmentUIModel. So, I thought it would be cleaner to use only ShipmentUIModel.

This is also related to a potential issue with the currency usage that we have in this screen and other places

If we fix that issue, we might stop using items, but it’s likely that we’ll use the shipping rates’ currency instead, which can also be part of ShipmentUIModel.

So, I thought it would be cleaner to use a single model, ShipmentUIModel. Let me know if you still think passing separate arguments would be a better approach.

[hichamboushaba]

As I pointed, this is just a np comment, but personally I think it's still better to pass separate items, or even better just pass the labelId of the label, and retrieve the data from the DataStores (like what we did in the old flow).

When we pass a large model to a screen, it gets saved into the SavedStateHandle, and thus increasing the chances of TransactionTooLargeException crashes, I don't think the model is that large in this case, but to me it's just a good practice that I try to follow, given these crashes were hard to fix in the past for us.

[hichamboushaba]

Merging this as is and revisiting this when working on the currency fix is an option too 🙂

[irfano]

Okay, that makes sense. It would be better to pass fewer arguments. I’ll do it in another PR.

[hichamboushaba]

Another point, I see that the "Request a refund" button on the order details still opens the old flow, will this be handled in a separate PR?

[wpmobilebot]

Version 22.6 has now entered code-freeze, so the milestone of this PR has been updated to 22.7.

[irfano]

Thanks for the review, @hichamboushaba!

• I've opened an issue for the currency problem: WOOMOB-609
• I'll handle opening the new refund flow from the order screen in a separate PR.