Easy-digital-downloads: `edd_has_active_discounts` returns false with active discounts present

[moorscode]

Internally edd_get_discounts uses a 30 item limit, which is used in edd_has_active_discounts to retrieve discounts to check for active status.

This means that if in the first 30 discounts there is no active one, the status is returned as false.

Will provide a PR with a fix.

[pippinsplugins]

Definitely a bug.

I don't think, however, that looping continuously until we find a valid code is a good idea. If a site (such as one of the ones our team runs) has 50-100,000 discount codes (or more), this could cause timeouts and performance problems.

Instead I think we should just do a direct query to the metadata and check for an active code that way.

[moorscode]

It's a very fast solution, because it only fetches the ID's of "active" discounts.
The only reason I did not go for a meta-query is because of timezone problems with date fields where you have to do a heavy SQL check to see if an active discount is actually inactive because it expired.

Plus it also helps to discover expired discounts because the active check disactivates any expired discount, making the next run even faster again.

You should also keep in mind that it will stop at the first discount that is actually active.

Perhaps sorting by date might make it quicker, figuring that the latest discount should have a higher probability of being active.

[cklosowski]

The biggest concern I have with this approach is the use of edd_is_discount_active. Because this actually does more than just checks if it's active but as you stated @moorscode it will update any discounts that it finds to be expired, and will then run DB queries to update them. This could cascade a large number of DB writes, which could cause a load problem in that writes cause a re-index.

This may take a much larger rewrite in that we'll need to do a few things. I'm going to think on it a bit.

[cklosowski]

Ok, I've had some time to think on this. I think we need a 2 phased approach. We can fix this issue up by doing the current method, but we need to update edd_is_discount_active to have a 2nd parameter that allows us to tell it to NOT update the database if it finds an 'active' discount that's actually expired. This prevents the write overrun I talked about before.

Now for the long term, we need to fix the discount code expiration date post meta to be better b/c right now it does not adhere to MySQL date format as we found in #4735 which was causing us problems to properly detect if a discount is expired.

I think we need to make a 2nd phase that runs an upgrade routine to update all discount expiration dates to the proper MySQL format for dates, which would allow us to write a pretty efficient query against the postmeta table to see if we have any active discount codes, without getting into a meta query. That would be need to be in 2.8 more than likely. Thoughts on that @pippinsplugins?

We can start it at the most recent discount codes (as they are likely to be the active ones), check the status (not update anything) and on the first active find, just return true. I think maybe what we do here is set a limit that is filterable though. Start it at only fetching the first 100 maybe (25 per page, 4 pages), and then after that it dies out as false, but allow there to be filters for the number of pages or number per page it pulls for sites that want to alter it further.

This allows us to stop the problem now, but then fix it correctly in 2.8 by fixing the date range and completely rewriting this section once we have the proper data formats.

[cklosowski]

@sunnyratilal would love your input here too since you've dealt with that date bug before too.

[sunnyratilal]

I agree with @cklosowski's approach here. An imminent fix is to add the second parameter to the function and avoid the database writes as it's an expensive operation until we fix the date queries in 2.8.

[sunnyratilal]

PR ready for testing at #5197. It includes an $update parameter to avoid DB writes for each expired discount.

[cklosowski]

@sunnyratilal can you write up a unit test to verify this doesn't affect the writing? Just to be double sure.

[cklosowski]

Ok, after some discussion, @sunnyratilal and I have decided the loop is not an ideal solution. We'll increase the request to look at the 100 most recent discounts. We'll also include adding a new parameter to the is_discount_active() and is_discount_inactive() calls that allow it's status to be checked without running a DB update on it.

Then we'll come up with a more comprehensive plan of migrating the discount information in the 2.8 cycle (is the plan currently).

[cklosowski]

@pippinsplugins Is the above ok with you?

[pippinsplugins]

I'm still concerned about the possibility of killing a site. What if a site has 10,000 inactive discounts? This will kill it.

Some kind of mitigation routine needs to be added. Perhaps a max loops?

[cklosowski]

@pippinsplugins re-read what I wrote :) We're limiting to JUST 100 discounts. No looping. Just the most recent 100. This gives us 3x more discounts for the sample size, without the overhead of the DB writes.

[cklosowski]

With a more clear path for the future to make this better in time.

[pippinsplugins]

:+1:

[cklosowski]

This looks good to me @sunnyratilal nice work. I'm going to leave this open, for testing.

[cklosowski]

To test this out. Setup a number of discounts.

1) Make all discounts active, verify that the discount field shows on checkout.
2) Make all discounts inactive, verify that the discount field does not show on checkout.
3) Make one of the discounts active again, preferably one in the middle or one of the first you created, verify that the discount field shows.

[SDavisMedia]

Replicated on master and confirmed fixed on issue/4936. :+1: