search

awesomemotive / easy-digital-downloads

Sell digital downloads through WordPress
https://easydigitaldownloads.com
GNU General Public License v2.0
865 stars 475 forks source link

Move discount codes to custom table #5277

Closed pippinsplugins closed 6 years ago

pippinsplugins commented 7 years ago

For numerous reasons, including performance and flexibility, discount codes should be moved to a custom table.

This will help lay the foundation for moving payments and sale logs to custom tables.

Related: https://github.com/easydigitaldownloads/easy-digital-downloads/issues/5217 https://github.com/easydigitaldownloads/easy-digital-downloads/issues/1427

4576

Pull Request: #6224

New database schema

There are two new tables being introduced:

Schema for wp_edd_discounts:

Field Type Null Key Default Extra
id bigint(20) unsigned NO PRI NULL auto_increment
name varchar(200) NO   NULL  
code varchar(50) NO MUL NULL  
status varchar(20) NO   NULL  
type varchar(20) NO   NULL  
amount mediumtext NO   NULL  
description longtext NO   NULL  
max_uses bigint(20) NO   NULL  
use_count bigint(20) NO   NULL  
once_per_customer int(1) NO   NULL  
min_cart_price mediumtext NO   NULL  
scope varchar(30) NO   all  
date_created datetime NO    
start_date datetime NO   NULL  
end_date datetime NO   NULL  

Schema for wp_edd_discountmeta:

Field Type Null Key Default Extra
meta_id bigint(20) NO PRI NULL auto_increment
edd_discount_id bigint(20) NO MUL NULL  
meta_key varchar(255) YES MUL NULL  
meta_value longtext YES   NULL  

New files

These files hold the DB interaction layers for discounts and discount meta.

Changes to EDD_Discount

The EDD_Discount class has been significantly updated in order to achieve a few goals:

  1. Bring its properties in line with the DB structure. A number of properties have been renamed. For example, is_not_global is now called applies_globally and its value has been adjusted accordingly.
  2. Reduce duplicate logic for adding and updating discounts.
  3. Make it more consistent with EDD_Payment, EDD_Commission, EDD_SL_License, EDD_Subscription, etc.
  4. Magic methods have been heavily utilized to provide a complete backwards compatibility layer for previous method and property names. For example, the expiration property no longer exists as it is now called end_date. Calling $discount->expiration will continue to function exactly the same, however, due to the BC layer that has been added.

The existing helper functions for discounts in includes/discount-functions.php have been left mostly unchanged. Only a few adjustments to account for data type enforcement.

Backwards compatibility layer

  1. EDD_Discount. As a lot of properties and methods have changed, we need to ensure the old names still work when accessed directly. For example, $discount->start doesn't technically exist anymore because it's been renamed to start_date. Calling $discount->start returns the value of $discount->start_date. This has been done for properties and methods.

  2. Post meta. I have implemented a BC layer that redirects calls to get_post_meta(), add_post_meta(), and update_post_meta().

  3. All legacy parameters previously used in edd_store_discount() still work. Those parameters can be passed to edd_store_discount(), EDD_Discount->add(), and EDD_Discount->update() and they will be automatically converted to the new parameters.

  4. A BC layer will be implemented with get_posts() and WP_Query. It will hijack the WP SQL query and instead query the wp_edd_discounts table but return WP_Post objects with the data filled from the wp_edd_discounts. A BC layer is not available for get_post() due to the way it's implemented in WordPress Core.

Upgrade routine

A full upgrade and migration routine has been added that moves discounts from the old wp_posts table to the new wp_edd_discounts table. This upgrade can be run via the UI or through WP CLI with wp edd migrate_discounts.

The upgrade routine includes a second step for removing legacy data.

Unit tests

The discount tests have been up into two sections:

So long as the legacy tests continue to pass without changes, we're pretty safe in knowing the BC layer is good.

tests-discounts-db.php and tests-discount-meta.php have been introduced that holds tests specific to the databases.

Todo

Notes for testing

  1. Please test on brand new installs
  2. Please test on existing installs that need to be migrated
  3. When testing, consider the following items as the most important a. Creating discounts b. Editing discounts c. Applying discounts to the cart (test all permutations of discount restrictions)
  4. Please test the upgrade routine from the UI and from WP CLI (if able)
  5. Please test any known extensions that interact with discount codes, such as Discount Code Generator and EDD Social Discounts
pippinsplugins commented 6 years ago

This is nearly ready for testing on new installs. I believe I have all functionality moved over and will now begin working on testing backwards compatibility layers.

To do that I'm going to first get all existing unit tests passing. Once those pass, I'll begin writing a bunch of unit tests that intentionally call old methods and intentionally interact with previous schemas so that I can account for those.

pippinsplugins commented 6 years ago

This is now ready for serious testing and review!

New database schema

There are two new tables being introduced:

Schema for wp_edd_discounts:

Field Type Null Key Default Extra
id bigint(20) unsigned NO PRI NULL auto_increment
name varchar(200) NO   NULL  
code varchar(50) NO MUL NULL  
status varchar(20) NO   NULL  
type varchar(20) NO   NULL  
amount mediumtext NO   NULL  
description longtext NO   NULL  
max_uses bigint(20) NO   NULL  
use_count bigint(20) NO   NULL  
once_per_customer int(1) NO   NULL  
min_cart_price mediumtext NO   NULL  
product_reqs mediumtext NO   NULL  
product_condition varchar(3) NO   all  
excluded_products mediumtext NO   NULL  
applies_globally int(1) NO   1  
created_date datetime NO   CURRENT_TIMESTAMP  
start_date datetime NO   NULL  
end_date datetime NO   NULL  
notes longtext NO   NULL  

Schema for wp_edd_discountmeta:

Field Type Null Key Default Extra
meta_id bigint(20) NO PRI NULL auto_increment
discount_id bigint(20) NO MUL NULL  
meta_key varchar(255) YES MUL NULL  
meta_value longtext YES   NULL  

Two new files have been introduced:

These files hold the DB interaction layers for discounts and discount meta.

Changes to EDD_Discount

The EDD_Discount class has been significantly updated in order to achieve a few goals:

  1. Bring its properties in line with the DB structure. A number of properties have been renamed. For example, is_not_global is now called applies_globally and its value has been adjusted accordingly.
  2. Reduce duplicate logic for adding and updating discounts.
  3. Make it more consistent with EDD_Payment, EDD_Commission, EDD_SL_License, EDD_Subscription, etc.
  4. Magic methods have been heavily utilized to provide a complete backwards compatibility layer for previous method and property names. For example, the expiration property no longer exists as it is now called end_date. Calling $discount->expiration will continue to function exactly the same, however, due to the BC layer that has been added.

The existing helper functions for discounts in includes/discount-functions.php have been left mostly unchanged. Only a few adjustments to account for data type enforcement.

Backwards compatibility layer

So far I have focused backwards compatibility efforts for two primary parts:

  1. EDD_Discount. As a lot of properties and methods have changed, we need to ensure the old names still work when accessed directly. For example, $discount->start doesn't technically exist anymore because it's been renamed to start_date. Calling $discount->start returns the value of $discount->start_date. This has been done for properties and methods.

  2. Post meta. I have implemented a BC layer that redirects calls to get_post_meta(), add_post_meta(), and update_post_meta().

  3. All legacy parameters previously used in edd_store_discount() still work. Those parameters can be passed to edd_store_discount(), EDD_Discount->add(), and EDD_Discount->update() and they will be automatically converted to the new parameters.

I still need to introduce a BC layer for queries to the wp_posts table.

Upgrade routine

A full upgrade and migration routine has been added that moves discounts from the old wp_posts table to the new wp_edd_discounts table. This upgrade can be run via the UI or through WP CLI with wp edd migrate_discounts.

The upgrade routine includes a second step for removing legacy data.

Unit tests

More than 50 new unit tests have been introduced to help ensure things work properly.

Our pre-existing unit tests for discount codes were pretty messy but I decided to leave them almost entirely as is in order to help test that the backwards compatibility layer works as expected.

I have broken the discount tests up into two sections:

So long as the legacy tests continue to pass without changes, we're pretty safe in knowing the BC layer is good.

I have also introduced a new test file tests-discounts-db.php that holds tests specific to the discounts DB.

Todo

I still have a number of items to address:

Notes for testing

  1. Please test on brand new installs
  2. Please test on existing installs that need to be migrated
  3. When testing, consider the following items as the most important a. Creating discounts b. Editing discounts c. Applying discounts to the cart (test all permutations of discount restrictions)
  4. Please test the upgrade routine from the UI and from WP CLI (if able)
  5. Please test any known extensions that interact with discount codes, such as Discount Code Generator and EDD Social Discounts
JJJ commented 6 years ago

A few cursory thoughts:

pippinsplugins commented 6 years ago

  1. I wouldn't be opposed to it though the use case has never come up in six+ years so I'm not sure it's really worth the effort nor future maintenance.

  2. Our other custom tables use id. I have made ID work in respect to backwards compatibility as the discounts have been moved from wp_posts. discount_id is permitted because . . . I don't remember why but it was introduced at one point, perhaps during the initial construction of EDD_Discount. I would like id to be the standard.

  3. once_per_customer is a boolean flag that indicates if customers are permitted to use individual discounts more than once. max_uses identifies the total number of times the discount can be redeemed, regardless of whether that is from one customer or 100 customers. max_uses is customer agnostic. I do see value in allowing the number of times a customer can use a discount being an actually integer instead of just a yes or no. My only concern with introducing that option is simply due to the lack of requests for it. I'm not sure I've ever seen a case where someone wanted to allow a customer to redeem a discount x times. It's always been unlimited or once.

  4. Ah yes, I like that much better. Will work on adjusting it.

  5. At this time it's only all and any.

  6. I'd be fine with doing it in meta. A separate table seems overkill as it's something that's ever really been needed, though the example scenario makes perfect sense. Putting each product ID into its own meta row would be easy.

  7. I agree, a notes table would be great. Customers, subscriptions, and payments already all have notes. Customers and subscriptions are stored in meta and payments is stored in comments. We'll need to make a decision on this before we complete the migration for payments. If we decide that payments needs to have its notes stored in a table, I vote we put discounts there too. If we do not use a separate table for payment notes, I think an individual column is fine. Most of the time notes are not too extensive so a single column is more than sufficient.

  8. :+1:

  9. Future considerations :)

pippinsplugins commented 6 years ago

Note: appears discount meta is not working yet.

chriscct7 commented 6 years ago

Where is EDD_Download->discount_id defined?

I don't think that's a thing, at least it doesn't appear to be a thing for the last several major versions.

I think you might have misremembered copying customer_id from the customer's table here: https://github.com/easydigitaldownloads/easy-digital-downloads/commit/525e153394817818b608e0b48eb30f9d3e34d4c6#diff-d7c8b380123e5377812c31e1d293cfbbR31

Which in turn simply was named that way because of the way AffiliateWP which did the pre-cursor to EDD's custom tables did it in https://github.com/AffiliateWP/AffiliateWP/issues/537 here: https://github.com/AffiliateWP/AffiliateWP/pull/538/files#diff-a3b0aa41b683448eaa74de8ea168216aR120

and Ryan Duff just called it affiliate_id because there was a column for meta_id (which is/was the ID column value for the post meta from the meta table for each meta item transferred) thus removing the ambiguity in which ID (the affiliate or the post meta ID) it was referring to.

chriscct7 commented 6 years ago

Notes can be really large. I would suggest storing notes in their own table, with an ID and timestamp so you can easily just query for the rows of notes for a specific discount. Also there's a github ticket for that #5794

Perhaps for a table suggestion to make a simple combined table:

Note ID | Date/time | Author | IP address | PostType | ID | Note

By using post type, we can allow extensions to easily use it to store their own notes by them just using the slug of their plugin name or something simple.

If you're thinking of moving payment logs to a custom table they should be in their own table. With the EDD bot auto created logs, that table is gonna get huge in a hurry for most stores.

So maybe 2 tables 1 for payments, one for everything else.

chriscct7 commented 6 years ago

By the way, re Should there be a creator_id or author_id column to identify what user created the discount?, if, and I think we should, let discounts have notes, then {username} created discount at {datetime} should be the first auto-generated note if author ID isn't something going into the main table.

JJJ commented 6 years ago

An imaginary wp_edd_notes database table should have:

This assumes the following:

JJJ commented 6 years ago

Related: I like the use of product over download with this table. I think we should continue that trend and enforce it throughout everything else as we go.

pippinsplugins commented 6 years ago

I'm tempted to just remove notes for now rather than introduce it as a column and have to migrate it later. With as many other pieces of data being migrated in 3.0, I don't think it's a good time to introduce yet another migration for a dataset that is not currently problematic (and in the case of discounts) nor used.

sunnyratilal commented 6 years ago

⚠️ Issue found when adding a new discount code:

If the start and end date are left blank, they default to 1970-01-01 23:59:59 for the the start_date and end_date in the db

sunnyratilal commented 6 years ago

For reference, after discussion with @pippinsplugins, the discount_id column in the edd_discountmeta table has been renamed to edd_discount_id as we've changed the meta type to edd_discount. This was necessary as all meta writes were failing on this line.

mindctrl commented 6 years ago

I'm tempted to just remove notes for now rather than introduce it as a column and have to migrate it later. With as many other pieces of data being migrated in 3.0, I don't think it's a good time to introduce yet another migration for a dataset that is not currently problematic (and in the case of discounts) nor used.

I agree. There's enough stuff going on in 3.0, and since notes aren't part of discounts now, I see no problem punting that to a future release.

sunnyratilal commented 6 years ago

🎉 Product requirements and excluded products now both exist in edd_discountmeta and values are written to meta as well on save().

sunnyratilal commented 6 years ago

Having CURRENT_TIMESTAMP as the default value of the created_date column fails on Travis's Precise environment - most probably because it's using an older version of MySQL.

Also technically CURRENT_TIMESTAMP is incorrect as a default value for a datetime field, it's only really correct for a field of type timestamp. I'm aware MySQL allows it as of version 5.6.5 but we're most probably going to have users who are still on a version of MySQL prior to 5.6.

sunnyratilal commented 6 years ago

🎉 Build is running and unit tests are passing!

sunnyratilal commented 6 years ago

Bug found in the count method. When calling the count() method with no parameters after inserting a new discount, it still returns the old count as the computed value still resides in the object cache.

sunnyratilal commented 6 years ago

Gone ahead and removed the caching from the count() as the same has been done in https://github.com/easydigitaldownloads/edd-software-Licensing/issues/515 by @cklosowski for probably the same reason

sunnyratilal commented 6 years ago

Unit tests have been added that cover the DB layer. All that's left is the search functionality.

sunnyratilal commented 6 years ago

The database schema has been altered to change created_date to date_created so as to maintain consistency with the wp_edd_licenses and wp_edd_customers table.

JJJ commented 6 years ago

@sunnyratilal owning it. Great work here so far!

sunnyratilal commented 6 years ago

Bug found with the unit tests, if update() is called with just updates to the meta and no updates to the columns in wp_edd_discounts, it causes an SQL error.

sunnyratilal commented 6 years ago

EDD_Discount now has full unit test coverage.

sunnyratilal commented 6 years ago

This commit introduces an error message for anyone trying to use get_post()/get_posts()/WP_Query to access discounts. We need to ensure that the link added to the messages references some sort of documentation to query discounts correctly.

pippinsplugins commented 6 years ago

@sunnyratilal looking good. Let's also please log a message with edd_debug_log() that includes a call stack.

sunnyratilal commented 6 years ago

@easydigitaldownloads/core-team I believe we are ready for testing here.

  1. Pull and checkout issue/5277
  2. Run wp edd create_discounts --number=1000 --legacy
  3. Run the migration from the admin UI or the CLI (wp edd migrate_discounts, add --force if you've previously run the migration)

Notes for testing

  1. Please test on brand new installs
  2. Please test on existing installs that need to be migrated
  3. When testing, consider the following items as the most important a. Creating discounts b. Editing discounts c. Applying discounts to the cart (test all permutations of discount restrictions)
  4. Please test the upgrade routine from the UI and from WP CLI (if able)
  5. Please test any known extensions that interact with discount codes, such as Discount Code Generator and EDD Social Discounts
ashleyfae commented 6 years ago

Just ran one quick migration test so far on some existing data.

  1. A number of my discounts got imported without their names. I haven't quite figured out how this happened and I can't find anything odd/different about the ones with missing names.
  2. The Amount column no longer shows the discount type. For example, it displays 50 instead of 50%, and it displays 35 instead of $35.
  3. All of the expired discounts are missing. It looks like the table is using this query: 
    SELECT id 
FROM wp_edd_discounts 
WHERE 1=1 
AND `status` IN( 'active','inactive' ) 
ORDER BY id DESC 
LIMIT 0,30;

    but perhaps it should be:

    SELECT id 
FROM wp_edd_discounts 
WHERE 1=1 
AND `status` IN( 'active','inactive','expired' ) 
ORDER BY id DESC 
LIMIT 0,30;
sunnyratilal commented 6 years ago

@nosegraze Thanks for testing!

  1. I've added a fallback in the migration routine, if _edd_discount_name is not present in the meta, it'll use the post_title from wp_posts.

  2. This has been fixed.

  3. When you say missing, did they not get migrated or are not being displayed? We don't display expired discounts on the table. Although, I can add it as a view.

ashleyfae commented 6 years ago

  1. Ah that may have been it! I'll give it another test later.

  2. They are migrated but not displayed anywhere in the table, whereas they were previously. Before I could see them listed under All. At first this made me think they hadn't been migrated at all because the All count went from like 46 to 21. I later realized it was excluding expired discount codes. Presumably this makes it impossible to edit or delete them, unless you do it manually via MySQL.

sunnyratilal commented 6 years ago

Ah, thanks for the clarification. I'll adjust the queries. :)

sunnyratilal commented 6 years ago

@nosegraze Gone ahead and adjusted the queries and added an Expired view.

pippinsplugins commented 6 years ago

Note, the upgrade routine that is currently used here will need to be updated once our other migrations are completed. See https://github.com/easydigitaldownloads/easy-digital-downloads/issues/6275.

amdrew commented 6 years ago

@sunnyratilal So far so good, can't fault it.

Not sure if you're ready for typos etc. But if you are:

On the initial upgrade notice:

This is a mandatory update that will migrate all discounts records

to

This is a mandatory update that will migrate all discount records

and

This upgrade should provider

to

This upgrade should provide

On the notice after the upgrade:

Easy Digital Downloads has finished migrating discount records, next step is to remove the legacy data. Learn more about this process.

to

Easy Digital Downloads has finished migrating discount records. Click here to remove the legacy data. Learn more about this process.

I think adding the click here text makes it more consistent with the first upgrade notice

All discountss records have been

to

All discount records have been

After the upgrade routine for the legacy data has completed, could we should another success notice? Edit: This is probably a bit premature considering it will be part of a much larger upgrade.

ashleyfae commented 6 years ago

I re-ran my earlier migration and can confirm the name issue is fixed!

One other thing I noticed is that not all the statuses quite match up. I think this is because the status was previously being saved in two places? Not sure if I have some screwed up legacy data or what. But one of my old discounts has the post_status in the wp_posts table set to inactive and on the master branch the discount appears as inactive. However, in the wp_postmeta table for that same discount, there's a meta key for _edd_discount_status with a value active. The discount is then being migrated with the status active.

I'm not sure why I have conflicting data, but in master I have inactive discounts that are being migrated as active instead.

sunnyratilal commented 6 years ago

@nosegraze The status has actually been implemented incorrectly within EDD_Discount. We use post_status to track the status of the discount, but when an instance of EDD_Discount is instantiated, we're actually using the post_meta - not quite sure how we didn't pick this up earlier!

pippinsplugins commented 6 years ago

I've added a small piece of logic to migrate() to ensure non-EDD meta rows get moved over to the new meta table. This will help ensure that data coming from other plugins (like AffiliateWP) gets retained instead of lost into oblivion.

amdrew commented 6 years ago

Here's an example of what other integrations may need to do based on the discount migration. In this case, AffiliateWP: https://github.com/AffiliateWP/AffiliateWP/issues/2499

sunnyratilal commented 6 years ago

@JJJ I've gone ahead and remove the create_table() methods from EDD_DB_Discounts and EDD_DB_Discount_Meta to allow the classes introduced in #6277 to handle the table creation. I've also removed EDD_DB_Discount_Meta::register_table() as again the class in #6277 will register the meta table with $wpdb.

pippinsplugins commented 6 years ago

We need to either re-add the create_table() method (here and other new DBs) or get #6277 finished up asap and merged into release/3.0 as the lack of the create_table() method makes this PR untestable for anyone that doesn't already have the DB created.

JJJ commented 6 years ago

Obviously I vote to merge, since it’s mostly done-done, and those classes handle everything table related very well.

JJJ commented 6 years ago

Should product_condition be bigger than varchar(3)?

The only options today areany or all, but similar columns (like status) are varchar(20).

I think opening this up to 20 now avoids us needing to do it later, and sets up a future roadmap for a product-condition API for discounts to narrow the scope to taxonomies or other things.

@pippinsplugins @sunnyratilal @cklosowski Thoughts?

pippinsplugins commented 6 years ago

Product condition is always any or all so 3 is sufficient unless we want to proactively enable future options.

I'm not sure what we might add there but there are definitely possibilities.

:+1: to changing it to 20.

sunnyratilal commented 6 years ago

Once merged, the backwards compatibility code needs to be grouped with other migrations in #6324.

sunnyratilal commented 6 years ago

issue/5277 has been merged into release/3.0. 🎉

This issue and the issue branch will stay open as there it is very likely that we will things that need changing when we entering the testing phase.

JJJ commented 6 years ago

This issue is technically done, so let's close it! 🥇

If bugs are discovered, new issues should be created.

<3