-
Notifications
You must be signed in to change notification settings - Fork 136
[WOOMOB-1378] "Trash Product" option invisible after creating the product #14657
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
[WOOMOB-1378] "Trash Product" option invisible after creating the product #14657
Conversation
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 fixes a bug where the "Trash Product" option was not visible after creating a new product from the product list screen. The issue occurred because the trash option was controlled by a destination argument that defaulted to false.
- Replaced the destination argument-based approach with a backstack-based check to determine when trash should be enabled
- Removed the
isTrashEnablednavigation argument from all product detail navigation calls - Added comprehensive tests to verify the trash option visibility logic
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ProductDetailFragment.kt | Implements backstack checking logic to determine trash option visibility |
| ProductDetailViewModel.kt | Replaces destination argument with StateFlow-based trash action control |
| ProductDetailViewModelTest.kt | Adds comprehensive tests for trash option visibility scenarios |
| nav_graph_main.xml | Removes isTrashEnabled navigation argument definitions |
| nav_graph_products.xml | Removes isTrashEnabled navigation argument definitions |
| ProductListFragment.kt | Removes isTrashEnabled argument from navigation calls |
| MainNavigationRouter.kt | Updates interface to remove enableTrash parameters |
| MainActivity.kt | Updates implementation to remove enableTrash parameters |
| DashboardTopPerformersWidgetCard.kt | Removes isTrashEnabled argument from navigation call |
| DashboardProductStockCard.kt | Removes isTrashEnabled argument from navigation call |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...ommerce/src/main/kotlin/com/woocommerce/android/ui/products/details/ProductDetailFragment.kt
Outdated
Show resolved
Hide resolved
...ommerce/src/main/kotlin/com/woocommerce/android/ui/products/details/ProductDetailFragment.kt
Show resolved
Hide resolved
📲 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 #14657 +/- ##
=========================================
Coverage 38.43% 38.44%
Complexity 9798 9798
=========================================
Files 2081 2081
Lines 116124 116123 -1
Branches 15498 15500 +2
=========================================
+ Hits 44638 44644 +6
+ Misses 67339 67332 -7
Partials 4147 4147 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hichamboushaba
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.
Nice work @AdamGrzybkowski 💯, pre-approving, I left two minor comments, but nothing blocking.
...ommerce/src/main/kotlin/com/woocommerce/android/ui/products/details/ProductDetailFragment.kt
Outdated
Show resolved
Hide resolved
| get() = viewModel.productDetailViewStateData.liveData.value?.productDraft | ||
|
|
||
| @Test | ||
| fun `given add new product flow, when trash action becomes possible, then trashOption remains hidden`() = |
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.
not a blocker, but I think we can make this test name clearer, the trash button can be available in the product creation flow, the important bit that controls whether the trash button is hidden not, is whether the product was created on the server or not yet. The current name makes it look like the trash button will always be hidden in the creation flow.
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.
This test doesn't verify that flow, but rather simply checks based on the initial conditions. In this case, for the AddNewProduct flow, the trash should be invisible.
That said, I think it's good to test the flow mentioned when the product is stored so I've added it here - f848f65
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.
This test doesn't verify that flow, but rather simply checks based on the initial conditions. In this case, for the AddNewProduct flow, the trash should be invisible.
Yes I'm aware, I suggested just modifying the test name, not the test itself 🙂
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.
Got it, what about: given add new product flow when product not yet created on server then trashOption is hidden?
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 like it, it's clear. Thanks @AdamGrzybkowski
878d7fd to
5c49211
Compare
5c49211 to
a262969
Compare
Closes WOOMOB-1378
Description
From the issue description:
This is cause becase the
trashEnableddestination argument defaulted to false when the fragment was opened from the bottom sheet. We can't simply change it totruebecause the same bottom sheet is used in other places, and we can only enable that option when the product details screen is opened from the product list screen.Adding the same value as a bottom sheet destination argument would be an option, but that would create a long chain of passing arguments, because the above-mentioned bottom sheet is opened from yet another bottom sheet. It would be a pain to write and maintain.
As proposed by @hichamboushaba this PR removed the destination argument completely, and instead checked whether the product list is in the backstack.
Steps to reproduce
Add manually...menu iconTrash productoption visibilityTesting information
The above steps are good to reproduce the bug and verify it's fixed.
Additionally, it's good to test other entry points for the Product details screen. One example is shown below, where the product is created from the onboarding checklist.
The tests that have been performed
Images/gif
Bug
Regression test (should be the same):
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.