Skip to content

Conversation

atorresveiga
Copy link
Contributor

@atorresveiga atorresveiga commented Nov 19, 2024

Closes: #12941

Description

This pull request addresses the issue of the PTR (Pull-to-Refresh) getting stuck in the user interface after exiting the search view while the search is still in progress.

When we subscribe to the new pagedListWrapper in the activatePagedListWrapper function, the new LiveData starts off as null and does not emit any value until the list changes. We also remove the previous source by calling clearLiveDataSources. However, because _isFetchingFirstPage is a MediatorLiveData, it retains the last cached value. This leads to a problem: when we exit the search view and switch to the new activatePagedListWrapper, the screen state remains stuck in a refreshing state. This occurs because _isFetchingFirstPage does not reflect the state of the current activatePagedListWrapper (which is null), but rather the last cached value.

To resolve this issue, I have chosen to reset the LiveData to its default values within the clearLiveDataSources function.

I considered an alternative solution that involved adding an initial value to the PagedListWrapper LiveData. However, since this class is utilized in other projects as well, I opted for the current solution to minimize the impact on our code and prevent any regressions in those other projects.

Steps to reproduce

  1. Go to Orders
  2. Tap on Search
  3. Search anything
  4. Get back to order while PTR is still displayed
  5. Notice that PTR is there forever now, and PTR is not possible for the orders at all

Testing information

  • Follow the steps to reproduce and check that the issue is not present in this PR-

The tests that have been performed

  • Checked that the issue is not present after following the Steps to reproduce
  • Test that PTR keeps working as expected
  • Test that filters keep working as expected

Images/gif

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

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.

@atorresveiga atorresveiga added type: bug A confirmed bug. feature: order list Related to the order list. labels Nov 19, 2024
@atorresveiga atorresveiga added this to the 21.2 milestone Nov 19, 2024
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Nov 19, 2024

📲 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
Commit6fe6759
Direct Downloadwoocommerce-wear-prototype-build-pr12948-6fe6759.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Nov 19, 2024

📲 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
Commit6fe6759
Direct Downloadwoocommerce-prototype-build-pr12948-6fe6759.apk

@codecov-commenter
Copy link

codecov-commenter commented Nov 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 39.66%. Comparing base (9a83bcc) to head (6fe6759).
Report is 5 commits behind head on trunk.

Additional details and impacted files
@@            Coverage Diff            @@
##              trunk   #12948   +/-   ##
=========================================
  Coverage     39.65%   39.66%           
- Complexity     5949     5952    +3     
=========================================
  Files          1261     1261           
  Lines         72890    72893    +3     
  Branches       9973     9973           
=========================================
+ Hits          28904    28910    +6     
+ Misses        41413    41411    -2     
+ Partials       2573     2572    -1     

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


🚨 Try these New Features:

@kidinov kidinov self-assigned this Nov 19, 2024
Copy link
Contributor

@kidinov kidinov left a comment

Choose a reason for hiding this comment

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

Tests well!

Thanks!

Shell a test be added though?

@kidinov kidinov self-requested a review November 19, 2024 09:59
@atorresveiga atorresveiga merged commit a908f47 into trunk Nov 19, 2024
15 checks passed
@atorresveiga atorresveiga deleted the issue/12941-fix-orders-search-refreshing branch November 19, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: order list Related to the order list. type: bug A confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PTR on the order forever after the order search
4 participants