Max uses not working

This is by design, to deliberately ignore orders that are "in checkout" - otherwise coupons entered for orders that were never completed would be included in the usage count, which is not desired by most sites.

Orders should not stay Pending for long - they should be moved to Payment received or Completed, at which point the coupon usage will be correctly counted. This also means that orders moved to Cancelled at any point will have their coupons become available for use again.

This still doesn't seem right. It can't be "working as designed" because you're making assumptions about the way that the order status will be used, essentially requiring it to be respect the default ubercart behaviour.

The official documentation at ubercart.org on states and statuses clearly outlines that module developers should expect statuses to be freely modified, renamed and re-ordered by site administrators and states are to be used to implement functionality exactly like what uc_coupons is implementing.

http://www.ubercart.org/docs/developer/920/order_states_and_statuses

"Why the duplication and bother with states vs. statuses? Well, this system allows the order statuses to be configurable by store administrators but also dependable in the code. Module developers must be assured that a status exists for a certain order state, but store owners can still have the freedom to rename, add, or reorder more order statuses."

In my case, a client renamed the "pending" status to "New Order" and moved it to the post_checkout state, because they are delivering each order by hand within an hour so "pending" means something different to them that it does to the use case outlined in this thread. The client read the ubercart documentation and argued that there's no reason why they shouldn't be allowed to do this and I'm inclined to agree, but since the weight of the status was still "0" the coupon max limits started behaving unexpectedly.

Using the states provided by ubercart, uc_coupon should definitely validate against orders in the states "post_checkout, payment_received, completed" and possibly "cancelled". Basically, anything other than "in_checkout".

Arguably there should be some way to customize this behaviour to account for states added by third party modules, etc.. but to at least maintain the current behaviour when a store admin re-orders their statuses' uc_coupon_count_usage() needs a little work.

Here's a patch rolled against version 6.x-1.7 to make sure uc_coupon only counts coupons recorded against orders that haven't been cancelled and aren't "in checkout" based on state. I noticed while I was making this that the 7.x-2.x branch seems to be handling this issue way better by checking a status weight defined in a variable, but still uses statuses instead of states.

Is there a good reason to be doing this all by state instead of status or is it just an artefact of the way things have always been done in this project?

updating version number for the issue to 6.x-1.7

Thanks for the patch. Actually, the D7 fix was ported to 6.x-1.x-dev several months ago. Please see #649306: Make coupon usage counting more flexible; Generally it's easier if you roll patches against the current dev branch.

To me, the advantage of checking status instead of state is that it gives admin more fine-grained control over when a coupon is marked as used. For example, you could create a "locked" status that is still "in checkout" and allow some coupons to be marked used after a certain point in the checkout process.

Closing this as a duplicate - please reopen the other thread if you want to discuss that point further.

I still think one of us is misinterpreting the intended usage of states vs. statuses.

Surely if you want to provide a way for admins to lock coupons through status, you should use the ubercart hooks provided to create an extra state. That should allow the same level of fine grained control for administrators while allowing uc_coupon to implement it's code as per the outlined uc "best practices".

As I understand, statuses are "content" whereas states are intended to be something more robust for module developers to rely on for functionality that is core to their operation.

I haven't reviewed the patch in the dev version that you're referencing, but how obvious is it to a store administrator that when they make certain updates to the statuses for their store, they have to update settings for the coupon module or it might break entirely? also, is it safe to assume that a user with permissions to edit the store's statuses and one that can administer coupons are always one and the same?

I agree that the original implementation should probably have used states - but you'd have to take that up with the original author; I can't speak to his reasons for doing it the way he did.

The fix for both D6 and D7 was in response to numerous requests for more flexibility in determining when a coupon is marked used. It's a sort of poor-man's Rule/CA predicate: - "when the order status changes to X, mark the coupon as used" -- and as such is exactly like other rules in Ubercart, which provides an "order status is changed" trigger, not an "order state is changed" trigger.

The Ubercart doc you reference also says "Administrators may also create custom order statuses to further refine the order workflow." -- and that is exactly what we're trying to allow them to do. For example, an admin can create a status that marks a coupon as used while the order is still in the "in checkout" state (which was the primary request, to prevent users from applying the same coupon twice from two different browsers).

Also, implementing your patch would very likely break many stores which depend on the current behavior -- for example, those which have adjusted order status weights to work around the previous limitations.

As longwave points out in the referenced thread, the best solution would be to make this fully Rules/CA based, but that was a more major overhaul of the module than I had time for. I'd gladly review a patch in that direction, though it would have to include default rules to mimic the current default behavior.

so sad that it has to be this way, but I do understand why :(

it means the module can never "just work" right? unless you package it in an installation profile. There's always some configuration required for the coupons and I'm sure we're all aware that Ubercart requires a fair bit of that out-of-the-box.

that said, surely there's enough information in the database that an update script could be written to convert from one paradigm to the next if we did want to move to states. That could be added to the patch.