Skip to content

[WOOMOB-3229] Send image deletion when a variation image is removed#16245

Open
irfano wants to merge 17 commits into
trunkfrom
issue/WOOMOB-3229-variation-image-removal
Open

[WOOMOB-3229] Send image deletion when a variation image is removed#16245
irfano wants to merge 17 commits into
trunkfrom
issue/WOOMOB-3229-variation-image-removal

Conversation

@irfano

@irfano irfano commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Fixes WOOMOB-3229

Description

Removing a variation's image never reached the store: the update mapper only included the image field when the new image was non-blank, so the removal was silently dropped and the image reappeared after the refresh. This was a regression from the bulk-create-variations refactor.

The mapper now sends image: {id: 0} when the image is cleared, which WooCommerce treats as deleting the variation image. Both variation fetches now use the REST edit context, GET wc/v3/products/<id>/variations/<id> for the edit screen and GET wc/v3/products/<id>/variations for the editing list, so the response carries the variation's own image instead of falling back to the parent product's image. This matches how the WooCommerce admin loads variations for editing: the variation's own image, or a placeholder. Other surfaces (the order creation variation picker and the POS lookup) keep the default view context, so the locally cached variation image carries the context of whichever fetch last wrote it. A null image in the response is now mapped to an empty string instead of the literal "null" it used to produce.

On stores older than WooCommerce 4.7, image removal isn't supported. The dedicated error message for this case was never actually shown due to a separate bug in its display condition; this PR fixes that too, so the app now shows it instead of a generic update error.

Test Steps

On a store with a variable product whose variation has an image:

  1. Open the product's Variations list.
  2. Open a variation that has an image.
  3. Remove the variation image.
  4. Tap Update.
  5. Confirm the image placeholder is shown instead of the parent product's image.
  6. Go back to the Variations list and confirm the row shows the placeholder too.
  7. Reopen the variation and confirm the parent image doesn't flash before the placeholder.
  8. Sanity-check that price, stock, and attributes still load correctly.

Images/gif

WooCommerce below 4.7 error message on removing a variation image.
WOOMOB-3229-below-4 7-message

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

@irfano irfano added type: bug A confirmed bug. feature: variation details Related to adding or editing variations and attributes. labels Jul 11, 2026
@irfano irfano added this to the 25.3 milestone Jul 11, 2026
@irfano
irfano marked this pull request as ready for review July 11, 2026 12:07

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

AI Code Review - Found 1 potential issue

The core fix is sound and consistent end-to-end: a removed image maps to a blank WCProductVariationModel.image, the mapper now sends {"id":0} (WCProductImageModel.toJson correctly omits src/name for id 0), and the ViewModel's updated variation.image == null guard matches how removal is represented (Optional(null) → blank image). The isJsonNull guard in the API response and the ifEmpty refactor are correct. Good unit coverage on both the mapper and the ViewModel error path.

PR housekeeping
  • This is the WooCommerce Android (Kotlin) repo; reviewed against AGENTS.md/CLAUDE.md FluxC + store-app conventions rather than the PHP/WooCommerce-core guidance.

Automatic review · claude-opus-4-8 · Workflow run

How to reply to a finding

Reply on this review (or inline at the line the finding refers to) with one of:

  • @claude addressed - I made the change. Bot verifies against the next diff before marking resolved.
  • @claude rejected: <reason> - Will not fix; reason gets quoted on the next review.
  • @claude not-applicable - Finding does not apply (wrong file, already covered elsewhere, etc.).

The bot honours these on the next review pass.

@wpmobilebot

wpmobilebot commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

App Icon📲 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
Build Number772
Version25.2-rc-1
Application IDcom.woocommerce.android.prealpha
Commitdbde534
Installation URL42ibuuo9q95co
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@codecov-commenter

codecov-commenter commented Jul 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 7.69231% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.16%. Comparing base (552970e) to head (355971f).

Files with missing lines Patch % Lines
...network/rest/wpcom/wc/product/ProductRestClient.kt 0.00% 4 Missing ⚠️
...rk/rest/wpcom/wc/product/ProductVariationMapper.kt 25.00% 1 Missing and 2 partials ⚠️
...st/wpcom/wc/product/ProductVariationApiResponse.kt 0.00% 2 Missing ⚠️
...rg/wordpress/android/fluxc/store/WCProductStore.kt 0.00% 2 Missing ⚠️
...ui/products/variations/VariationDetailViewModel.kt 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##              trunk   #16245   +/-   ##
=========================================
  Coverage     41.15%   41.16%           
- Complexity    13501    13508    +7     
=========================================
  Files          2503     2503           
  Lines        146465   146468    +3     
  Branches      21761    21761           
=========================================
+ Hits          60278    60288   +10     
+ Misses        80089    80054   -35     
- Partials       6098     6126   +28     

☔ View full report in Codecov by Harness.
📢 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.

@AdamGrzybkowski AdamGrzybkowski self-assigned this Jul 15, 2026

@AdamGrzybkowski AdamGrzybkowski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 👍 but:

Both variation fetches now use the REST edit context, GET wc/v3/products//variations/ for the edit screen and GET wc/v3/products//variations for the editing list, so the response carries the variation's own image instead of falling back to the parent product's image.

I think it's worth confirming with the Peacock team because the fetchSingleVariation is also used by POS.

@irfano irfano added the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Jul 15, 2026
@dangermattic

Copy link
Copy Markdown
Collaborator
1 Error
🚫 This PR is tagged with status: do not merge label(s).

Generated by 🚫 Danger

@irfano

irfano commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

I think it's worth confirming with the Peacock team because the fetchSingleVariation is also used by POS.

Good point, Adam. This slightly changes the behavior of how variation images are displayed on the order details and POS screens. Also, this PR uses the existing image field in the DB, but populates it from two different sources (edit and view context fetches). I fixed that in a follow-up PR. I’ll merge this after the follow-up is approved, and I’ll ask Peacock to review the follow-up PR (#16272).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: variation details Related to adding or editing variations and attributes. status: do not merge Dependent on another PR, ready for review but not ready for merge. type: bug A confirmed bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants