Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: variations exported as draft were imported as draft #36933

Merged
merged 4 commits into from Mar 7, 2023

Conversation

Konamiman
Copy link
Contributor

@Konamiman Konamiman commented Feb 23, 2023

All Submissions:

Changes proposed in this Pull Request:

When a product export that involves a variable product in draft status is performed, the "published" row of the generated CSV has a status of "draft" not only for the main variable product but also for the variations, even though the variations themselves are always in "published" state (otherwise they aren't shown when editing the product).

The product import code is supposed to handle variations as a special case, creating them a status of "published" even if they were set as "draft" in the CSV; but due to a bug (a piece of code that assumed that 1 === 1.0 was true) that wasn't happening. This pull request fixes that.

Closes #34962.

How to test the changes in this Pull Request:

  1. Go to WooCommerce - Products and create a variable product with some variations. Don't publish it; instead, click on "Save as draft".
  2. Generate a product export (WooCommerce - Products - "Export" button) that includes all the columns. Optionally, first create a new product category and assing it to the product you just created, then export only that category, so that the generated CSV file contains only that product.
  3. Delete the product.
  4. Import the CSV file you just created ((WooCommerce - Products - "Import" button).
  5. Open the product details and verify that you can see the variations. Without this fix the main product would have been imported but variations would remain invisible (they would have a status of "draft" in the database).

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?
  • Have you included testing instructions?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@Konamiman Konamiman self-assigned this Feb 23, 2023
@Konamiman Konamiman requested review from a team and jorgeatorres and removed request for a team February 23, 2023 15:10
@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Feb 23, 2023
@codecov
Copy link

codecov bot commented Feb 23, 2023

Codecov Report

Merging #36933 (5da6a63) into trunk (48ec7b8) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             trunk   #36933     +/-   ##
==========================================
- Coverage     46.7%    46.7%   -0.0%     
- Complexity   17178    17187      +9     
==========================================
  Files          429      429             
  Lines        64779    64823     +44     
==========================================
+ Hits         30240    30254     +14     
- Misses       34539    34569     +30     
Impacted Files Coverage Δ
.../includes/import/class-wc-product-csv-importer.php 75.1% <100.0%> (-0.5%) ⬇️
...s/Version1/class-wc-rest-coupons-v1-controller.php 42.9% <0.0%> (-0.2%) ⬇️
...rters/class-wc-product-csv-importer-controller.php 36.1% <0.0%> (-0.1%) ⬇️
...rs/Version1/class-wc-rest-orders-v1-controller.php 46.2% <0.0%> (-0.1%) ⬇️
plugins/woocommerce/includes/class-wc-ajax.php 4.3% <0.0%> (-0.1%) ⬇️
plugins/woocommerce/includes/class-wc-coupon.php 73.9% <0.0%> (ø)
...lugins/woocommerce/includes/wc-order-functions.php 76.3% <0.0%> (ø)
...ugins/woocommerce/includes/wc-coupon-functions.php 100.0% <0.0%> (ø)
...ocommerce/includes/admin/class-wc-admin-assets.php 0.0% <0.0%> (ø)
... and 6 more

@github-actions
Copy link
Contributor

github-actions bot commented Feb 23, 2023

Test Results Summary

Commit SHA: 5da6a63

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202611m 8s
E2E Tests189006019516m 55s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

Now ArrayUtil::get_value_or_default($array, $key, $default) will return
null, instead of $default, when $array[$key] exists and is null.
@jorgeatorres
Copy link
Member

@Konamiman: This looks good to me overall, but I wonder if changing how the method works at this point has any implications. I did a quick search through WC's codebase but there are quite a few results and didn't really test much.

What are your thoughts? Could this be considered a breaking change (despite of what the intent was for that method when added)? (related to p7bje6-4Ly-p2#comment-7756)

@Konamiman
Copy link
Contributor Author

@jorgeatorres The method was intended to act as with the fix (and documented as such), so I'd say that it should be fixed; if that's a problem for existing code, then that code should be fixed (just replacing method usages with a ?? expression where appropriate would be enough).

That said, maybe fixing the method and the usages is a task best suited for its own pull request, instead of in this one (the other code modifications in this pull request don't involve using ArrayUtil::get_value_or_default after all).

@jorgeatorres
Copy link
Member

That said, maybe fixing the method and the usages is a task best suited for its own pull request, instead of in this one (the other code modifications in this pull request don't involve using ArrayUtil::get_value_or_default after all).

I agree. I'm not against the change, and we're probably the only users (right now) of that function, so changing it should be fine but we should at least audit the code that currently uses it.

If you're willing to split the PR, that'd be great.

@Konamiman
Copy link
Contributor Author

Pull request for the change to ArrayUtil::get_value_or_default: #37053

Copy link
Member

@jorgeatorres jorgeatorres left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you @Konamiman!

@jorgeatorres jorgeatorres merged commit 8786c19 into trunk Mar 7, 2023
19 of 20 checks passed
@jorgeatorres jorgeatorres deleted the fix/import-of-draft-variations branch March 7, 2023 16:57
@github-actions github-actions bot added this to the 7.6.0 milestone Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On CSV import, product variations are not visible if parent is draft
2 participants