Skip to content

Conversation

@malinajirka
Copy link
Contributor

@malinajirka malinajirka commented Oct 29, 2025

Description

This PR refactors WooPosSyncAction - removes code duplication by introducing a helper that fetches data from a paginated endpoint. It also consolidates the returning values for products+variations, so both share the same type.

Test Steps

Green unit tests should be enough - this PR impacts only Local Catalog related classes which are behind a FF (if you'd decide to do manual tests I think it's better to do them on the follow up PR).

Images/gif

N/A

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

@dangermattic
Copy link
Collaborator

dangermattic commented Oct 29, 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 23.6. 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

Copy link
Contributor

Copilot AI left a 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 refactors the paginated data fetching logic for products and variations in the WooCommerce POS local catalog synchronization. The main goal is to eliminate code duplication by introducing a common interface and helper class for handling paginated fetch operations.

Key changes:

  • Introduced WooPosPaginatedFetchResult interface to standardize paginated fetch results
  • Created PaginatedSyncHelper to consolidate duplicated pagination logic
  • Refactored both product and variation fetch operations to use the new helper class

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
WooPosPaginatedFetchResult.kt New sealed interface defining the contract for paginated fetch results
WooPosLocalCatalogFetchProductsResult.kt Updated to implement WooPosPaginatedFetchResult interface
WooPosVariationsFetchResult.kt Updated to implement WooPosPaginatedFetchResult interface
WooPosSyncAction.kt Major refactoring: removed duplicate pagination logic, introduced PaginatedSyncHelper class, consolidated error handling
Project_Default.xml Formatting changes (indentation standardization)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 29, 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 NameWooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commit84f1594
Direct Downloadwoocommerce-wear-prototype-build-pr14856-84f1594.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 29, 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 NameWooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commit84f1594
Direct Downloadwoocommerce-prototype-build-pr14856-84f1594.apk

@malinajirka malinajirka added this to the 23.6 milestone Oct 29, 2025
@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 83.92857% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.25%. Comparing base (bf0d3b4) to head (84f1594).
⚠️ Report is 34 commits behind head on trunk.

Files with missing lines Patch % Lines
...android/ui/woopos/localcatalog/WooPosSyncAction.kt 82.60% 10 Missing and 6 partials ⚠️
...s/localcatalog/WooPosLocalCatalogSyncRepository.kt 80.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk   #14856      +/-   ##
============================================
- Coverage     38.26%   38.25%   -0.02%     
+ Complexity    10080    10073       -7     
============================================
  Files          2132     2131       -1     
  Lines        120716   120680      -36     
  Branches      16554    16546       -8     
============================================
- Hits          46190    46164      -26     
+ Misses        69835    69825      -10     
  Partials       4691     4691              

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

@samiuelson samiuelson self-assigned this Oct 30, 2025
@samiuelson samiuelson requested a review from Copilot October 30, 2025 10:03
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/store/pos/localcatalog/WooPosPaginatedFetchResult.kt:4

  • The generic type parameter T should be documented with a KDoc comment to explain what types of items this result can contain (e.g., products or variations).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@samiuelson samiuelson left a comment

Choose a reason for hiding this comment

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

LGTM!

@samiuelson samiuelson merged commit 9c44f97 into trunk Oct 30, 2025
27 of 28 checks passed
@samiuelson samiuelson deleted the issue/woomob-1334-woo-poslocal-catalog-run-both-products-and-variations-within-v2-step2 branch October 30, 2025 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants