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

Modify the logic for deleting variations #27137

Merged
merged 5 commits into from Jan 4, 2021
Merged

Modify the logic for deleting variations #27137

merged 5 commits into from Jan 4, 2021

Conversation

gumgumcode
Copy link
Contributor

@gumgumcode gumgumcode commented Jul 27, 2020

Summary

Implements the following expected behavior which fixes an issue with Subscriptions:

  1. Switch from variable subscription to non-variable one will delete variations.
  2. Switch from variable subscription to variable product and vice-versa will NOT delete any variations.

This shall have no side-effects in WooCommerce Core.

All Submissions:

Changes proposed in this Pull Request:

Closes #26959.
Closes https://github.com/woocommerce/woocommerce-subscriptions/issues/3672

How to test the changes in this Pull Request:

This was originally pushed to WC Subscriptions but it makes more sense to add it directly to WooCommerce. Testing instructions are therefore available in WC Subscriptions PR: https://github.com/woocommerce/woocommerce-subscriptions/pull/3795.

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 successfully run tests with your changes locally?

Changelog entry

Improves the logic for deleting variations when a product type is changed

Copy link
Contributor

@rrennick rrennick left a comment

Choose a reason for hiding this comment

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

@codestor4 This has the potential to change behaviour for third party product types. This could be implemented as a filter that defaulted to the current logic. WC Subs could use that filter to bypass removing the variations.

@rrennick rrennick added needs: triage feedback Issues for which we requested feedback from the author and received it. and removed status: needs review labels Aug 20, 2020
Implements the following expected behaviour:
1. Switch from variable subscription to non-variable one will delete variations.
2. Switch from variable subscription to variable product and vice-versa will NOT delete any variations.

This shall have no side-effects in WooCommerce Core.
This will introduce two new filters:

woocommerce_from_product_type_changed
woocommerce_to_product_type_changed

to filter the $from and $to variables respectively.

This will be useful in WCS pr_3732!
@gumgumcode
Copy link
Contributor Author

gumgumcode commented Sep 8, 2020

Thanks for the feedback @rrennick. I have made the necessary changes now.

The newly introduced filters will be used as follows in https://github.com/woocommerce/woocommerce-subscriptions/pull/3732:

add_filter( 'woocommerce_from_product_type_changed', array( __CLASS__, 'maybe_keep_variations' ) );
add_filter( 'woocommerce_to_product_type_changed', array( __CLASS__, 'maybe_keep_variations' ) );

/**
* Prevent variations from being deleted when product type changes from variable product to another variable product.
*
* @since 3.0.8
*
* @param string $product_type The product type value to filter.
* @return string $product_type The filtered product type value.
*/
public static function maybe_keep_variations( $product_type ) {
if ( strpos( $product_type, 'variable' ) ) {
    $product_type = 'variable';
}
return $product_type;
}

I've tested this and it works. I'll be adding it to the pull request mentioned above after this current pull request gets merged.

@gumgumcode gumgumcode added status: needs review and removed needs: triage feedback Issues for which we requested feedback from the author and received it. labels Sep 8, 2020
Removing the two newly introduced filters and replacing it with one single filter: woocommerce_delete_variations_on_product_type_change.
@gumgumcode
Copy link
Contributor Author

Update: I have removed the two filters and replaced it with one. It still achieves the same end goal.

@gumgumcode
Copy link
Contributor Author

Hey @rrennick, no rush but could you review this once again when you have some time? Thx :)

@peterfabian peterfabian requested review from a team and Konamiman and removed request for a team November 30, 2020 13:57
@Konamiman
Copy link
Contributor

Hi @codestor4, the change looks fine but it would be great if you could add a documentation comment for the newly added filter. See an example here: https://github.com/woocommerce/woocommerce/blob/master/includes/admin/meta-boxes/views/html-variation-admin.php#L165-L174 (please use version 4.9 for the @since field).

Updated @SInCE to 4.9 and added an inline doc for the new filter
'woocommerce_delete_variations_on_product_type_change'
@gumgumcode
Copy link
Contributor Author

@Konamiman I've made the requested changes. Please review and let me know! 👍

includes/class-wc-post-data.php Outdated Show resolved Hide resolved
includes/class-wc-post-data.php Show resolved Hide resolved
@gumgumcode
Copy link
Contributor Author

gumgumcode commented Dec 21, 2020

@abhishek-pokhriyal It's ready for a review 👍
Note to self: never to use @ in commit messages, it appears I accidentally tagged a GH member.

@Konamiman
Copy link
Contributor

Approved. @rrennick do you want to take a second look before merging?

@rrennick
Copy link
Contributor

rrennick commented Jan 4, 2021

@codestor4 Sorry for the delays in getting this through review.

@rrennick rrennick merged commit 82fb9f1 into master Jan 4, 2021
@rrennick rrennick deleted the issue_26959 branch January 4, 2021 17:06
@woocommercebot woocommercebot added release: add changelog Mark all PRs that have not had their changelog entries added. [auto] release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels Jan 4, 2021
@rrennick rrennick added this to the 5.0.0 milestone Jan 4, 2021
@claudiosanches claudiosanches removed the release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] label Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: add changelog Mark all PRs that have not had their changelog entries added. [auto]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete variations when product type changes to a non-variable product
7 participants