Create additional download permissions on product save if needed #28521

Konamiman · 2020-12-09T08:50:37Z

All Submissions:

Have you followed the WooCommerce Contributing guideline?
Does your code follow the WordPress' coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

When a simple product with downloads gets converted into a variable product all the existing download permissions for past orders become invalid. This commit adds an extra verification procedure to the product save code:

Get all the existing download permissions for the product and all the children (variations).
For each download permission for the parent product, if there's a variation that offers the same file for download (same file URL) AND an equivalent download permission doesn't exist (equivalent means same file URL, same order id and same user id), then create it. This is done in a scheduled action.

Additionally, a new WC_Customer_Download_Data_Store::create_from_data method is added.

Closes #26475.

How to test the changes in this Pull Request:

Start in master.
Create a simple product as "downloadable" with three downloadable files, let's call them file1, file2 and file3.
Complete an order for the product, you should have access to all three files (in "My Account - Downloads").
Convert the product to variable, with two downloadable variations. Make one variation offer file1 and the other one file2 (use the same file URLs, display names for the files can be different).
Save the product, and verify that you have lost access to the files.
Switch to this branch, go to the product edit page and save without changing anything.
Go to WooCommerce - Status - Scheduled Actions - Pending, you should see one named adjust_download_permissions. Wait until it runs, or run it yourself.
Verify that you got access to file1 and file2 again, but not to file3.
Save the product again, and wait until the scheduled action runs again.
Check the contents of the wp_woocommerce_downloadable_product_permissions table in the database, there should be no duplicate entries.

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

Enhancement - Create additional download permissions for simple downloadable products that are converted to variable products provided that there are variations offering the same files.

When a simple product with downloads gets converted into a variable product all the existing download permissions for past orders become invalid. This commit adds an extra verification procedure to the product save code: - Get all the existing download permissions for the product and all the children (variations) - For each download permission for the parent product, if there's a variation that offers the same file for download (same file URL) AND an equivalent download permission doesn't exist (equivalent means same file URL, same order id and same user id), then create it. Additionally, a new WC_Customer_Download_Data_Store::create_from_data method is added.

Also, move all the new code from the 'WC_Product_Data_Store_CPT' class to a new separate 'DownloadPermissionsAdjuster' class.

…load_permissions Also replace direct invocations of functions with usages of the LegacyProxy whenever needed, and code style ajustments.

…ssions

The DownloadPermissionsAdjuster class hooks to adjust_download_permissions from within its init method. However this method is executed only if the class is resolved, otherwise the hooks doesn't get attached and then the scheduled action is not serviced. To solve this, the class is resolved from WooCommerce::init_hooks. This requires a change in DownloadPermissionsAdjuster::init to use wc_get_container()->get( LegacyProxy::class )->get_instance_of instead of WC()->get_instance_of, since WC() can't be used from WooCommerce::construct (which invokes init_hooks).

roykho · 2021-01-14T17:14:54Z

Was there no way to separate the product from the order once it was ordered? What about just storing the file URL within the order meta? This way you wouldn't need to re-generate permissions.

Konamiman · 2021-01-15T10:25:16Z

Hi @roykho. Thanks for your suggestion, but unless I'm missing/misunderstanding something that wouldn't help:

Downloadable files are ultimately tied to products, not (or not only) orders. If a product ceases to exist or ceases to have the downloadable file then the associated download permissions are no longer valid. That's why the wp_woocommerce_downloadable_product_permissions table holds both the order id and the product id.

So if we were to store the file id (probably that and not the URL, as in wp_woocommerce_downloadable_product_permissions) in order meta then we would have to either store the product id there as well, or join the product permissions table to get it; in any case the original problem (missing permissions when converting to variable product) would still be present.

A seconday concern is that we are planning to move towards dedicated tables, so storing yet more stuff in post meta should be avoided whenever possible. Again, unless there's something I'm missing.

roykho · 2021-01-15T13:24:26Z

Downloadable files are ultimately tied to products, not (or not only) orders.

What you're saying is currently how it works and I understand that however I believe that is not the best approach. What I am suggesting is change how WC works in terms of what is ordered and the products.

Ideally, after an order is made, I can delete the product completely and the order should still have all the information necessary to fulfill the order obligation. That should include the original price of the product, taxes and any downloadable files attached to that product...etc.

So if we were to store the file id (probably that and not the URL, as in wp_woocommerce_downloadable_product_permissions) in order meta then we would have to either store the product id there as well, or join the product permissions table to get it; in any case the original problem (missing permissions when converting to variable product) would still be present.

We would not need the product ID because the download permission table has order product id/order id already so it knows which permissions are tied to the order.

As I said in the comment of the original issue, I believe to make it work correctly will be a heavy lift. Your PR can be used as an interim however if you feel strongly about getting this in.

Konamiman · 2021-01-15T15:02:40Z

Ideally, after an order is made, I can delete the product completely and the order should still have all the information necessary to fulfill the order obligation. That should include the original price of the product, taxes and any downloadable files attached to that product...etc.

That makes sense but it's also a little bit tricky. I'm thinking of a situation where I want to completely discontinue a software product I used to sell because it's too old and maybe it even has security issues; on one hand if a customer has purchased something it seems logical to give him access to the download "at eternum", on the other hand it may be better to remove access (because you provided an alternative or the product is really useless by now). Although this is already kind of contemplated in the "access expires" field.

Anyway, I agree with what you say next:

I believe to make it work correctly will be a heavy lift.

Yep. This would need some extended discussion.

Your PR can be used as an interim however if you feel strongly about getting this in.

I think that if this works as I intend it to work, it's indeed a good solution until we can figure out a better mechanism for download permissions in general.

roykho

Tested well, good job! Since this is a pretty big PR, I just skim through the parts and make sure they made sense. I just left some small comments.

roykho · 2021-01-15T23:03:01Z

includes/data-stores/class-wc-customer-download-data-store.php

+	 * Assumes that all the keys in the passed data are valid.
+	 *
+	 * @param array $data Data to create the permission for.
+	 * @returns int The database id of the created permission, or false if the permission creation failed.


Should be @return here. Without the s

roykho · 2021-01-15T23:03:06Z

includes/data-stores/class-wc-customer-download-data-store.php

+			return $adjusted_date;
+		}
+
+		throw new Exception( "I don't know how to get a date from a " . is_object( $date ) ? get_class( $date ) : gettype( $date ) );


Does this not need to be translatable?

My understanding was that since it's an exception message it should rarely be displayed and if it is, the viewer will probably be a developer. But anyway, I see that our convention in the code is to translate the exception messages so I'll do the same.

roykho

LGTM! Thanks!

Konamiman self-assigned this Dec 9, 2020

Move the download permissions adjustment process to a scheduled action.

f0b834d

Also, move all the new code from the 'WC_Product_Data_Store_CPT' class to a new separate 'DownloadPermissionsAdjuster' class.

Konamiman force-pushed the fix/26475 branch from 3e487df to f0b834d Compare December 10, 2020 14:40

Konamiman added 2 commits January 12, 2021 10:15

Remove one of the duplicate handlers for 'adjust_download_permissions'

10eb9f8

Add a test for the added functionality of LegacyProxy.

afe7bdc

Konamiman force-pushed the fix/26475 branch from fb576ae to afe7bdc Compare January 12, 2021 09:16

Konamiman added 3 commits January 12, 2021 12:07

Add tests for DownloadPermissionsAdjuster::maybe_schedule_adjust_down…

2d83db8

…load_permissions Also replace direct invocations of functions with usages of the LegacyProxy whenever needed, and code style ajustments.

Add unit tests for DownloadPermissionsAdjuster::adjust_download_permi…

2ec002d

…ssions

Konamiman marked this pull request as ready for review January 14, 2021 16:36

Konamiman requested review from a team and roykho and removed request for a team January 14, 2021 16:36

roykho requested changes Jan 15, 2021

View reviewed changes

Set exception message as translatable

544df16

Konamiman requested a review from roykho January 18, 2021 14:31

roykho approved these changes Jan 18, 2021

View reviewed changes

roykho merged commit 4c54895 into master Jan 18, 2021

roykho deleted the fix/26475 branch January 18, 2021 14:51

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 18, 2021

roykho removed the release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] label Jan 18, 2021

Konamiman added this to the 5.0.0 milestone Jan 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create additional download permissions on product save if needed #28521

Create additional download permissions on product save if needed #28521

Konamiman commented Dec 9, 2020 •

edited

Loading

roykho commented Jan 14, 2021

Konamiman commented Jan 15, 2021 •

edited

Loading

roykho commented Jan 15, 2021 •

edited by Konamiman

Loading

Konamiman commented Jan 15, 2021

roykho left a comment

roykho Jan 15, 2021

roykho Jan 15, 2021

Konamiman Jan 18, 2021

roykho left a comment

Create additional download permissions on product save if needed #28521

Create additional download permissions on product save if needed #28521

Conversation

Konamiman commented Dec 9, 2020 • edited Loading

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

Changelog entry

roykho commented Jan 14, 2021

Konamiman commented Jan 15, 2021 • edited Loading

roykho commented Jan 15, 2021 • edited by Konamiman Loading

Konamiman commented Jan 15, 2021

roykho left a comment

Choose a reason for hiding this comment

roykho Jan 15, 2021

Choose a reason for hiding this comment

roykho Jan 15, 2021

Choose a reason for hiding this comment

Konamiman Jan 18, 2021

Choose a reason for hiding this comment

roykho left a comment

Choose a reason for hiding this comment

Konamiman commented Dec 9, 2020 •

edited

Loading

Konamiman commented Jan 15, 2021 •

edited

Loading

roykho commented Jan 15, 2021 •

edited by Konamiman

Loading