Skip to content

Conversation

@malinajirka
Copy link
Contributor

Step 6 needs to be merged first.

Failing CI is expected here since this is a partial migration.

Description

This is part of the migration to POS specific Product Model. There will be many changes and I want to avoid huge PRs. Therefore I created a feature branch - I'll create a PR for each set of changes. The CI won't be able to build this PR, but I personally wouldn't worry about it -> we can ensure everything is green and working as expected when we are merging the feature branch back to trunk.

The Version2 in the name WooPosProductModelVersion2 will eventually be removed.

This PR migrates WooPosCartViewModel, WooPosVariationMapper, WooPosProductSearchPredicate, WooPosSearchProductsDataSource,WooPosSearchResultsIndex

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.

@malinajirka malinajirka added this to the 23.3 milestone Sep 8, 2025
@malinajirka malinajirka requested a review from kidinov September 8, 2025 12:06
@malinajirka malinajirka added status: do not merge Dependent on another PR, ready for review but not ready for merge. feature: POS labels Sep 8, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Sep 8, 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
Commit3085b9e
Direct Downloadwoocommerce-wear-prototype-build-pr14590-3085b9e.apk

@kidinov kidinov self-assigned this Sep 8, 2025
on { events }.thenReturn(parentToChildrenMutableSharedFlow)
}

private val mockedEventForTracking: WooPosAnalyticsEvent.Event.ItemAddedToCart =
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m curious, why do we need this one? And if we do, why is it called “mocked” when it seems to be the real object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Detekt wouldn't let me keep the mocked data object - there is a relatively new rule, that data classes can't be mocked. I kept the word mocked in the name to highlight, the object is kind of random - isn't actually testing which event we are passing/getting, just that it's a WooPosAnalyticsEvent.

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 made a comment about the changes in the test

) : Parcelable {

val variationEnabledAttributes
get() = attributes.filter { it.isVariation }
Copy link
Contributor

@kidinov kidinov Sep 8, 2025

Choose a reason for hiding this comment

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

It's an immutable data class, so do we need to refilter the list every time when it's accessed, or maybe it will be more efficient to have it as a field?

 @IgnoredOnParcel
  val variationEnabledAttributes = attributes.filter { it.isVariation }

?

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 point, I'll create a lazily calculated property - that gives us the best of both worlds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 4bb389f

…ific-product-model-step-6' into issue/woomob-1280-woo-poslocal-catalog-use-new-pos-specific-product-model-step-7
Base automatically changed from issue/woomob-1280-woo-poslocal-catalog-use-new-pos-specific-product-model-step-6 to feature-branch-for-pos-product-migration September 10, 2025 12:48
@malinajirka malinajirka removed the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Sep 10, 2025
@malinajirka malinajirka merged commit 07ee3f6 into feature-branch-for-pos-product-migration Sep 10, 2025
11 of 19 checks passed
@malinajirka malinajirka deleted the issue/woomob-1280-woo-poslocal-catalog-use-new-pos-specific-product-model-step-7 branch September 10, 2025 12:49
@malinajirka malinajirka mentioned this pull request Sep 10, 2025
1 task
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.

4 participants