Skip to content

[Shipping labels] Remove shipment sheet #13924

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

Merged
merged 13 commits into from
Apr 17, 2025

Conversation

irfano
Copy link
Contributor

@irfano irfano commented Apr 16, 2025

Ref WOOMOB-285

Description

This adds a remove shipment sheet and displays it when the “Remove shipment” buttons on the split shipment screen are tapped. The removal action itself is not handled in this PR.

Steps to reproduce

  1. Open the orders tab
  2. Tap on an order eligible for shipping label creation. It’s better to choose one with multiple items.
  3. Tap on Create shipping label.
  4. Tap on Split shipment.
  5. Choose some items and move them to a new shipment.

Testing information

Please also test in dark mode and landscape orientation.

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.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

@irfano irfano added the feature: shipping labels Related to creating, ordering, or printing shipping labels. label Apr 16, 2025
@irfano irfano added this to the 22.2 milestone Apr 16, 2025
@dangermattic
Copy link
Collaborator

dangermattic commented Apr 16, 2025

2 Warnings
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️ This PR is assigned to the milestone 22.2. This milestone is due in less than 2 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Apr 16, 2025

📲 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
FlavorJalapeno
Build TypeDebug
Commitc9783ae
Direct Downloadwoocommerce-wear-prototype-build-pr13924-c9783ae.apk

Comment on lines +56 to +58
var selectedShipmentKeyState by rememberSaveable {
mutableIntStateOf(removeShipmentSheet.otherShipments.keys.first())
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the selected shipment logic is simple, I decided to use the composable as state owner.


@Suppress("UnusedParameter")
fun onRemoveShipment(removingShipmentKey: Int, movingToShipmentKey: Int) {
// TODO handle removing shipment
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll handle onRemoveShipment later.

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Apr 16, 2025

📲 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
FlavorJalapeno
Build TypeDebug
Commitc9783ae
Direct Downloadwoocommerce-prototype-build-pr13924-c9783ae.apk

Base automatically changed from fix/shipping-labels-incorrect-item-quantity to trunk April 16, 2025 18:51
@irfano irfano added the type: task An internally driven task. label Apr 16, 2025
@irfano irfano changed the base branch from trunk to issue/13763-refactor-variations-data-source April 17, 2025 10:43
@irfano irfano changed the base branch from issue/13763-refactor-variations-data-source to issue/WOOMOB-282-fix-split-shipment-move-action April 17, 2025 10:43
…ix/add-shipping-labels-remove-shipment-sheet
@irfano irfano force-pushed the fix/add-shipping-labels-remove-shipment-sheet branch from e70e1fe to 1c4f5e7 Compare April 17, 2025 10:59
@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2025

Codecov Report

Attention: Patch coverage is 6.58683% with 156 lines in your changes missing coverage. Please review.

Project coverage is 38.27%. Comparing base (aec5665) to head (c9783ae).
Report is 34 commits behind head on trunk.

Files with missing lines Patch % Lines
...oshippinglabels/split/RemoveShipmentBottomSheet.kt 0.00% 142 Missing ⚠️
...glabels/split/WooShippingSplitShipmentViewModel.kt 44.00% 13 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk   #13924      +/-   ##
============================================
- Coverage     38.32%   38.27%   -0.06%     
+ Complexity     9407     9405       -2     
============================================
  Files          2098     2099       +1     
  Lines        115971   116118     +147     
  Branches      14831    14853      +22     
============================================
- Hits          44450    44446       -4     
- Misses        67477    67628     +151     
  Partials       4044     4044              

☔ 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 hichamboushaba self-assigned this Apr 17, 2025
Copy link
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 @irfano, code looks good.

I just left one nit comment.

And a question, should this PR close the ticket given that it doesn't implement the removing yet?

) {
Row(modifier = Modifier.padding(16.dp), verticalAlignment = Alignment.CenterVertically) {
Icon(
imageVector = ImageVector.vectorResource(id = R.drawable.ic_remove_shipment),
Copy link
Member

Choose a reason for hiding this comment

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

nit, the icon is just an arrow, so maybe we can name it as so to favor reusability if this icon ends up reused in other screens in the future, WDYT about ic_arrow_return (As this seems similar to this bootstrap icon)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I renamed it: c9783ae

@irfano
Copy link
Contributor Author

irfano commented Apr 17, 2025

should this PR close the ticket given that it doesn't implement the removing yet?

Good catch! I overlooked that the "Close" keyword was in the description. I've removed it.

@irfano irfano changed the base branch from issue/WOOMOB-282-fix-split-shipment-move-action to trunk April 17, 2025 13:35
<path
android:fillAlpha="0.6"
android:fillColor="#3C3C43"
android:pathData="M1.516,0.846c0.302,0 0.546,0.095 0.732,0.285 0.186,0.186 0.278,0.43 0.278,0.733 0,0.17 -0.007,0.361 -0.022,0.571 -0.014,0.21 -0.022,0.459 -0.022,0.747 0,1.02 0.137,1.848 0.41,2.483a2.688,2.688 0,0 0,1.334 1.392c0.61,0.293 1.42,0.44 2.431,0.44h6.27l2.226,0.116L12.626,5.3 10.261,2.91a1.037,1.037 0,0 1,-0.227 -0.322,0.967 0.967,0 0,1 -0.088,-0.41 0.94,0.94 0,0 1,0.285 -0.703,0.992 0.992,0 0,1 0.733,-0.286c0.268,0 0.513,0.107 0.732,0.322l6.248,6.24c0.22,0.22 0.33,0.479 0.33,0.777 0,0.298 -0.11,0.554 -0.33,0.769l-6.218,6.218c-0.123,0.122 -0.25,0.21 -0.381,0.264a0.902,0.902 0,0 1,-0.381 0.088,0.992 0.992,0 0,1 -0.733,-0.286 0.94,0.94 0,0 1,-0.285 -0.703c0,-0.147 0.027,-0.283 0.08,-0.41 0.059,-0.122 0.137,-0.23 0.235,-0.322l2.366,-2.388 2.519,-2.315 -2.22,0.11h-6.4c-1.426,0 -2.588,-0.224 -3.487,-0.674A4.255,4.255 0,0 1,1.047 6.8C0.622,5.868 0.41,4.68 0.41,3.233c0,-0.273 0.005,-0.532 0.014,-0.776 0.015,-0.249 0.04,-0.471 0.074,-0.666 0.029,-0.152 0.08,-0.298 0.153,-0.44a0.968,0.968 0,0 1,0.33 -0.359,0.92 0.92,0 0,1 0.535,-0.146Z" />

Check warning

Code scanning / Android Lint

Long vector paths Warning

Very long vector path (1036 characters), which is bad for performance. Considering reducing precision, removing minor details or rasterizing vector.
@irfano irfano enabled auto-merge April 17, 2025 13:36
@irfano irfano merged commit 98c979b into trunk Apr 17, 2025
19 checks passed
@irfano irfano deleted the fix/add-shipping-labels-remove-shipment-sheet branch April 17, 2025 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: shipping labels Related to creating, ordering, or printing shipping labels. type: task An internally driven task.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants