Skip to content

Conversation

@malinajirka
Copy link
Contributor

@malinajirka malinajirka commented Oct 7, 2025

#WOOMOB-1068

Description

This PR implements a first version of the Local Catalog settings.

Unit tests - I skipped unit tests for the VM, since the logic is just dummy. I'll add them in the follow up PR.

Question - currently, the full refresh is performed within VM scope. I'm wondering whether we want to keep it like this or if we want the button to schedule an immediate background sync instead. Wdyt?

Testing information

  1. Open POS
  2. Tap on the more icon
  3. Select Settings
  4. Tap on Catalog
  5. Notice the Local Catalog settings

Verify the Catalog section is not visible when the Local Catalog FF is turned off.

The tests that have been performed

The above

Images/gif

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

@malinajirka malinajirka added this to the 23.5 milestone Oct 7, 2025
@malinajirka malinajirka requested a review from Copilot October 7, 2025 16:53
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 implements the initial version of Local Catalog Settings for WooPos, introducing a new settings category that manages local catalog data and synchronization preferences.

  • Adds a new "Catalog" settings category with status information, cellular data toggle, and manual refresh functionality
  • Implements feature flag-based visibility to show/hide the Local Catalog settings
  • Integrates analytics tracking for the new Local Catalog settings section

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
WooCommerce/src/main/res/values/strings.xml Adds string resources for Local Catalog settings UI elements
WooPosAnalyticsEvent.kt Adds analytics event for Local Catalog section tapping
WooPosSettingsLocalCatalogViewModel.kt Implements ViewModel with catalog status loading and sync functionality
WooPosSettingsLocalCatalogState.kt Defines state data classes for catalog status and UI state
WooPosSettingsLocalCatalogScreen.kt Creates Compose UI for Local Catalog settings with status, toggle, and refresh sections
WooPosSettingsDetailPaneScreen.kt Integrates Local Catalog screen into settings navigation
WooPosSettingsCategoriesViewModel.kt Adds feature flag logic to conditionally show Local Catalog category
WooPosSettingsCategoriesState.kt Adds LOCAL_CATALOG enum entry with navigation destination
WooPosSettingsViewModel.kt Adds analytics tracking for Local Catalog category selection
WooPosSettingsState.kt Defines navigation destination for Local Catalog settings

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@dangermattic
Copy link
Collaborator

dangermattic commented Oct 7, 2025

2 Warnings
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️ Class WooPosSettingsLocalCatalogViewModel is missing tests, but unit-tests-exemption label was set to ignore this.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 7, 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
Commit240cb05
Direct Downloadwoocommerce-wear-prototype-build-pr14717-240cb05.apk

@malinajirka malinajirka added the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Oct 7, 2025
@malinajirka malinajirka marked this pull request as ready for review October 7, 2025 17:03
@malinajirka malinajirka requested a review from samiuelson October 7, 2025 17:04
@malinajirka malinajirka marked this pull request as draft October 7, 2025 17:06
@malinajirka malinajirka marked this pull request as ready for review October 7, 2025 17:07
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 7, 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
Commit240cb05
Direct Downloadwoocommerce-prototype-build-pr14717-240cb05.apk

Base automatically changed from issue/introduce-pos-date-formatter to trunk October 7, 2025 17:19
@malinajirka malinajirka removed the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Oct 8, 2025
@malinajirka malinajirka removed the request for review from samiuelson October 8, 2025 08:07
@@ -0,0 +1,14 @@
package com.woocommerce.android.ui.woopos.settings.details.localcatalog

data class WooPosSettingsLocalCatalogState(
Copy link
Contributor

@kidinov kidinov Oct 8, 2025

Choose a reason for hiding this comment

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

I think this state declaration is a bit ambiguous:

  1. Nullable catalogStatus - when it's null, does that mean loading or error - not clear
  2. Overlapping flags - you could have isLoading and isRefreshing both true at the same time, which is can be correct, but I am not sure. When you hit refresh I'd assume we may want to show catalog status "loading" too?

Maybe something like that?

data class WooPosSettingsLocalCatalogState(
    val catalogStatus: CatalogStatus = CatalogStatus.Loading,
    val allowCellularDataUpdate: Boolean = false,
) {
    sealed class CatalogStatus {
        data class Available(
            val catalogSize: String,
            val lastUpdate: String,
            val lastFullUpdate: String
        ) : CatalogStatus()

        object Loading : CatalogStatus()
    }
}

And when it's "loading" maybe to not show "refresh button" at all?

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 callout! Fixed in 541fce3 and be52b50.

@kidinov kidinov self-requested a review October 8, 2025 13:01
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.

LGTM!

I'd just suggest thinking about the state declaration, making sure that it cannot be in an ambiguous state, and checking this in compile time. So ideally, to avoid flags and the nullables there.

@malinajirka malinajirka enabled auto-merge October 9, 2025 05:48
@malinajirka malinajirka merged commit f62764c into trunk Oct 9, 2025
15 checks passed
@malinajirka malinajirka deleted the issue/woomob-1068-woo-poslocal-catalog-implement-sync-section-in-settings branch October 9, 2025 06:04
@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 0% with 76 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.36%. Comparing base (4a856cc) to head (240cb05).
⚠️ Report is 81 commits behind head on trunk.

Files with missing lines Patch % Lines
...ocalcatalog/WooPosSettingsLocalCatalogViewModel.kt 0.00% 43 Missing ⚠️
...ls/localcatalog/WooPosSettingsLocalCatalogState.kt 0.00% 11 Missing ⚠️
...gs/categories/WooPosSettingsCategoriesViewModel.kt 0.00% 9 Missing ⚠️
.../android/ui/woopos/settings/WooPosSettingsState.kt 0.00% 5 Missing ⚠️
...ttings/categories/WooPosSettingsCategoriesState.kt 0.00% 5 Missing ⚠️
...d/ui/woopos/util/analytics/WooPosAnalyticsEvent.kt 0.00% 2 Missing ⚠️
...roid/ui/woopos/settings/WooPosSettingsViewModel.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk   #14717      +/-   ##
============================================
- Coverage     38.37%   38.36%   -0.01%     
- Complexity     9855     9871      +16     
============================================
  Files          2099     2102       +3     
  Lines        117044   117132      +88     
  Branches      15657    15670      +13     
============================================
+ Hits          44913    44938      +25     
- Misses        67964    68026      +62     
- Partials       4167     4168       +1     

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

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