Skip to content
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

Product list selection #7971

Merged
merged 57 commits into from
Dec 12, 2022
Merged

Product list selection #7971

merged 57 commits into from
Dec 12, 2022

Conversation

atorresveiga
Copy link
Contributor

@atorresveiga atorresveiga commented Dec 1, 2022

Closes: #7929
Closes: #7930

Why

To be able able to bulk update products, first, the merchant needs to select the products he wants to update.

Description

This task is about enabling a selectable state of the products list view. It allows product selection by using the SelectionTracker, part of androidx.recyclerview.selection, an addon library providing support for item selection.

Testing instructions

TC1

  1. Open the products list screen
  2. Tap on the multi-select menu item
  3. Check that the view enters on selection mode
  4. Tap on the products and check that the selection/deselection works as expected
  5. Check that the selection count works as expected

TC2

  1. Open the products list screen
  2. Long press a product and check that the view enters on selection mode
  3. Complete Steps 4 and 5 of TC1

TC3

  1. Open the products list screen
  2. Tap on the multi-select menu item
  3. Tap on the more menu (3 dots)
  4. Tap on Select All
  5. Check that all loaded products are selected

TC4

  1. Open the products list screen
  2. Tap on Search
  3. Type a valid search query
  4. Long press a product to enter selection mode
  5. Tap on <- to exit the selection mode
  6. Tap on <- to exit the search
  7. Check that the app doesn't crash.
  8. Check this comment

Images/gif

selection.mp4
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@atorresveiga atorresveiga added the feature: product list Related to the product list. label Dec 1, 2022
@atorresveiga atorresveiga requested a review from wzieba December 1, 2022 01:01
@atorresveiga atorresveiga changed the title Issue/7929 product list selection Product list selection Dec 1, 2022
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 1, 2022

You can test the changes on this Pull Request by downloading an installable build, or scanning this QR code:

@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2022

Codecov Report

Base: 41.79% // Head: 41.85% // Increases project coverage by +0.06% 🎉

Coverage data is based on head (7987e34) compared to base (990af07).
Patch coverage: 50.87% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##              trunk    #7971      +/-   ##
============================================
+ Coverage     41.79%   41.85%   +0.06%     
- Complexity     3477     3486       +9     
============================================
  Files           688      689       +1     
  Lines         37317    37463     +146     
  Branches       4826     4834       +8     
============================================
+ Hits          15596    15682      +86     
- Misses        20310    20367      +57     
- Partials       1411     1414       +3     
Impacted Files Coverage Δ
.../woocommerce/android/analytics/AnalyticsTracker.kt 20.76% <ø> (ø)
...tlin/com/woocommerce/android/extensions/ViewExt.kt 0.00% <0.00%> (ø)
...d/ui/products/MutableMultipleSelectionPredicate.kt 0.00% <0.00%> (ø)
...merce/android/ui/products/ProductListRepository.kt 8.63% <0.00%> (-1.13%) ⬇️
...oid/ui/products/ProductSelectionItemKeyProvider.kt 0.00% <0.00%> (ø)
...e/android/ui/products/ProductSortAndFiltersCard.kt 0.00% <0.00%> (ø)
...oid/ui/products/SelectableProductListItemLookup.kt 0.00% <0.00%> (ø)
...mmerce/android/ui/products/ProductListViewModel.kt 56.11% <74.75%> (+7.38%) ⬆️
...om/woocommerce/android/analytics/AnalyticsEvent.kt 100.00% <100.00%> (ø)
...m/woocommerce/android/viewmodel/ScopedViewModel.kt 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@atorresveiga atorresveiga added the needs: design Requires design input/work from a designer. label Dec 2, 2022
@atorresveiga
Copy link
Contributor Author

atorresveiga commented Dec 2, 2022

P2 about the changes implemented here that diverges from the task's design

@peril-woocommerce
Copy link

peril-woocommerce bot commented Dec 6, 2022

Warnings
⚠️ PR is not assigned to a milestone.
⚠️ PR has more than 300 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

…agment

Fix/list multi-select top fragment
@atorresveiga atorresveiga added the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Dec 6, 2022
@atorresveiga atorresveiga marked this pull request as ready for review December 6, 2022 14:18
Copy link
Contributor

@wzieba wzieba left a comment

Choose a reason for hiding this comment

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

Thank you, great work @atorresveiga ! Leaving some comments

Comment on lines 169 to 188
val selectionCount = tracker?.selection?.size() ?: 0
if (selectionCount > 0 && actionMode == null) {
viewModel.enterSelectionMode()
actionMode = (requireActivity() as AppCompatActivity)
.startSupportActionMode(this@ProductListFragment)
}
when (selectionCount) {
0 -> {
viewModel.exitSelectionMode()
actionMode?.finish()
}
else -> {
actionMode?.title = StringUtils.getQuantityString(
context = requireContext(),
quantity = selectionCount,
default = R.string.product_selection_count,
one = R.string.product_selection_count_single
)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚙️ Can we move this logic to ViewModel? We should only pass information about selection change and a number of selected items to VM and then orchestrate changes from ViewModel.

Suggested change
val selectionCount = tracker?.selection?.size() ?: 0
if (selectionCount > 0 && actionMode == null) {
viewModel.enterSelectionMode()
actionMode = (requireActivity() as AppCompatActivity)
.startSupportActionMode(this@ProductListFragment)
}
when (selectionCount) {
0 -> {
viewModel.exitSelectionMode()
actionMode?.finish()
}
else -> {
actionMode?.title = StringUtils.getQuantityString(
context = requireContext(),
quantity = selectionCount,
default = R.string.product_selection_count,
one = R.string.product_selection_count_single
)
}
}
val selectionCount = tracker?.selection?.size() ?: 0
viewModel.onSelectionChanged(selectionCount)

@@ -197,6 +199,7 @@ class ProductListViewModel @Inject constructor(
}

fun onLoadMoreRequested() {
if (isSelecting()) return
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Would pagination in a selectable state break something? If not, maybe we could allow users to load more products even if they are in selectable state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it breaks the selection state. I'll take a closer look to see if it is possible to add pagination to the selection

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's generally a low priority. I think we can proceed with this feature with the behavior you proposed in this PR and then improve it later.

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 found out what the problem was, so we have list selection with pagination now 😁

Comment on lines 533 to 538
sealed class ProductListState : Parcelable {
@Parcelize
object Selecting : ProductListState()

@Parcelize
object Browsing : ProductListState()
Copy link
Contributor

Choose a reason for hiding this comment

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

NP: as those classes have no state, maybe we could consider using enum to reduce code

Comment on lines 464 to 465
ProductListViewModel.ProductListState.Browsing -> {
onListSelectionActiveChanged(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚙️ We should also clear selection when entering browsing state. Otherwise, the state is inconsistent.

Suggested change
ProductListViewModel.ProductListState.Browsing -> {
onListSelectionActiveChanged(false)
ProductListViewModel.ProductListState.Browsing -> {
tracker?.clearSelection()
onListSelectionActiveChanged(false)

@atorresveiga atorresveiga removed the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Dec 6, 2022
@ThomazFB ThomazFB self-assigned this Dec 9, 2022
Alejo added 11 commits December 9, 2022 07:43
…ulk-product-status-update

# Conflicts:
#	WooCommerce/src/main/kotlin/com/woocommerce/android/ui/products/ProductListViewModel.kt
…add-bulk-products-update-tracks

# Conflicts:
#	WooCommerce/src/main/kotlin/com/woocommerce/android/ui/products/ProductListViewModel.kt
#	WooCommerce/src/test/kotlin/com/woocommerce/android/ui/products/ProductListViewModelTest.kt
@ThomazFB
Copy link
Contributor

ThomazFB commented Dec 9, 2022

Thanks for this implementation @atorresveiga! The selection code and strategy look great, but I'm noticing some weird behavior with the selection itself. At first, I managed to use it and test all scenarios without issues, but now, when I try to select a product during the selection mode, the Product Details is called, and the selection enters a weird state. Here are some captures showing how this is happening:

screencapture-1670611067907.mp4

It's possible to verify that when I come back from the Product Details view, the selection state gets inconsistent, displaying 25 selected products, but the list is not reflecting that.

…s-update-tracks

feat: add tracks events for product bulk update
Copy link
Contributor

@ThomazFB ThomazFB left a comment

Choose a reason for hiding this comment

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

Considering our talk on Slack and how hard it's to reproduce the issue mentioned above in a physical device. I'll approving this PR as the feature overall looks to be working nicely!

@ThomazFB
Copy link
Contributor

Thanks for moving forward with the fix @atorresveiga! 🙇 It's working perfectly! I didn't manage to reproduce the issue again in any way! Amazing work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: product list Related to the product list. needs: design Requires design input/work from a designer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement "select all" feature Implement "selection state" of products list view
5 participants