This is a follow-up to http://drupal.org/node/734690. The solution there works fine but generates very long codes (min 16 chars) for coupons purchased in bulk. Please consider this patch, which attempts to remedy by dividing the number of characters specified in bulk_length between the newly purchased coupon's base code and its bulk_length. The following logic is adopted:
-for single coupons, the new coupon code length is equal to the specified bulk_length
-for multiple (bulk order) coupons, the new coupon base length is set to the specified bulk_length-8, and the bulk_length is set to 8
eg: Base coupon specified with code of PURCHASE, bulk_length of 12
- singe coupon will be PURCHASE[12-char-code]
- mult coupon will be PURCHASE[4-char-base][8-char-bulk]
The net result is that all purchased coupons will have the same length whether purchased singly or in bulk.
Thanks.

CommentFileSizeAuthor
#29 shorter-code-length.patch1.76 KBwodenx
#13 uc_coupon-956840-3.patch13.46 KBwodenx
#12 uc_coupon-956840-2.patch11.79 KBwodenx
#4 uc_coupon-956840.patch6.03 KBwodenx
#3 uc_coupon_purchase-956840.patch1.76 KBwodenx
uc_coupon_purchase.code-length-1.patch1.02 KBwodenx

Comments

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

This seems like a sensible request, but what if you set up a bulk purchase coupon with a bulk length of 8? It seems that the base length will be zero so all the purchased coupons will have the same prefix, which will cause problems. Perhaps bulk purchased coupons need a minimum initial bulk length setting of 12?

wodenx’s picture

wodenx commented

Sure, that sounds reasonable. Out of curiosity, though, what are the dire consequences of all the purchased coupons having the same prefix? Is it just a tracking issue (i.e. so you can know from the number which coupons were part of the same order)? Presumably the bulk code generator does a good enough job of assuring unique coupon numbers without the added step of a prefix unique to each order--or am I missing something?
Perhaps this could be left up to the store administrator's look-out - would it suffice to issue a warning if the initial bulk length is less than 12?

wodenx’s picture

wodenx commented
StatusFileSize
new uc_coupon_purchase-956840.patch1.76 KB

ok - attached patch generates an error if the bulk length of the base coupon <12

wodenx’s picture

wodenx commented
StatusFileSize
new uc_coupon-956840.patch6.03 KB

Actually, upon reflection, I think there's a better way.
The attached patch modifies the uc_coupon_add_form to include a fieldset for purchased coupons where you can specify the prefix length, and there's a checkbox to specify whether or not generated coupons should always have the same code length (regardless of whether they are purchased singly or in bulk). The data are stored in the "data" field of the base coupon table. Then, when a new purchased coupon is generated, if these fields are present they are used to govern the new code creation - if not, the fallback is the old default (to make the prefix length==the bulk length).
It seemed to make sense to make the form modification in the uc_coupon_add_form rather than in the uc_coupon_purchase_feature_form because those settings will affect *all* coupon purchase features based on that coupon.
While i was at it, I added an option to reset the coupon valid dates based on the purchase date. The default behavior is as it was, but in this version there is a check box which will cause the "valid_from" date for purchased coupons to be set to the purchase date. If enabled, the "valid_until" date will be calculated by adding the original term to the new start date -- i.e., if the base coupon were valid from 1/1/08 to 1/1/09 and the new coupon were purchased on 11/11/10, the new coupon would be valid 11/11/10-11/11/11.
Note that the attached patch (unlike the previous ones) is a multiple-file patch that should be applied to the uc_coupon base directory.
Thanks
-wodenx

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

That patch looks good, and the recalculate dates option is useful. However, I would prefer those settings to stay in a hook_form_alter in uc_coupon_purchase, so there's no uc_coupon_purchase code in uc_coupon.module - but there's the problem of adding the new fields to the coupon data array on submit. I think uc_coupon_add_form_submit() needs to be reworked so it saves any extra fields straight into the data array, then uc_coupon_purchase can do everything internally.

wodenx’s picture

wodenx commented

I agree hook_form_alter() would be better. Are you suggesting that *any* extra fields found in the form be saved to the data array? One would then have to keep a list of uc_coupon-defined field names to know which one's didn't need to be saved. Also, other modules altering the form would have to make sure their new field names didn't conflict. How 'bout one of these three options:

  1. A naming convention for field names - external modules who wanted to have form data saved into the array could prefix the names of the fields they wanted added - e.g. '%%field_name' - then we'd only have to search for fields beginning with the prefix and add them.
  2. A 'value' type field in the form which modules could populate with their own data - that way they could do preprocessing of the raw form data and store it in whatever format they chose. eg.
    $form_state['values']['uc_coupon_extra data']['mymodule'] = array( 'mykey' => $mydata, &c. );

    executed from a custom '#validate' handler. The elements of the array would then be added to the coupon's data array.

  3. Add a uc_coupon_save() hook. Probably the most drupalish solution, but it means the external module would have to hold on to its data temporarily after processing the form, waiting for the hook to be invoked.

Personally, I like #2 best - but you tell me which you prefer and I'll implement it.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Yeah, I think we could stuff the remainder of $form_state['values'] into the $data array after removing the fields that uc_coupon stores separately, so other modules can use hook_form_alter and expect their data to be automatically saved. I think we can also use array_filter() to remove a number of the current checks.

Maybe something like:

$data = $form_state['values'];

// Save values that go directly in the table
$code = ...
$valid_from = ...
$valid_to = ...
unset($data['code']);
unset($data['valid_from']);
unset($data['valid_to']);
[etc]

[filter products, terms and users with preg_match, as before]

// Filter arrays for empty values (not sure if this is needed?)
foreach ($data as &$value) {
  if (is_array($value)) {
    $value = array_filter($value);
  }
}

// Filter data for empty values and arrays
$data = array_filter($data);

If another module does need to change the data before saving, it could add a submit handler that runs before this one and alter $form_state as needed.

Alternatively, similar to option #3, we could put the new cid into $form_state after saving. Other modules can add submit handlers that run afterwards, and update the saved coupon themselves.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Or, build a real $coupon object in the submit handler, then add a new hook_uc_coupon_presave() and pass $coupon by reference and $form_state['values'] so modules can do what they like, and then use drupal_write_record() to save the coupon object instead of the SQL queries that are used at present?

wodenx’s picture

wodenx commented

I think passing the $form_state values in a presave hook is a great idea - that way the other module doesn't have to worry about ordering of submit handlers. But it's your call - let me know what you'd like me to do and i'll do it...

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Yeah, I think the presave hook is probably the most flexible option while keeping it fairly simple for other developers. If we switch to drupal_write_record() as well, then uc_coupon_purchase (and other modules) can use the same technique when creating their own coupons, avoiding breakage if the database schema ever changes.

wodenx’s picture

wodenx commented

ok - sounds good

wodenx’s picture

wodenx commented
Status: Needs work » Needs review
StatusFileSize
new uc_coupon-956840-2.patch11.79 KB

OK - here's the solution outlined in #8.

wodenx’s picture

wodenx commented
StatusFileSize
new uc_coupon-956840-3.patch13.46 KB

This version of the patch is better - abstracts the coupon save operation to uc_coupon_save() - and uc_coupon_purchase_create() calls that rather than writing to the db directly, as per your suggestion in #10.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

I committed a modified version of the first half of the patch in #13, keeping the new uc_coupon_save() and presave hook while cleaning up the form submit handler. Looking at the uc_coupon_purchase part next.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Fixed

Committed the uc_coupon_purchase part, with a few changes. I removed the "same length" option and enabled it by default, because I think this makes the most sense and there's already too many options on the coupon edit page. I also renamed the keys for the other two settings so they are more obvious as to what they do.

Marking this as fixed as we've already deviated from the original issue quite a bit, any further patches should be in new issues. Thanks for your work on these changes!

wodenx’s picture

wodenx commented

This all looks excellent - many thanks for taking it up.

One small thing: I'm curious as to why you enforce a minimum suffix length of 4+(bulk_length). I'm still not convinced that it would be so terrible for two purchased coupons to have the same suffix (excluding the bulk code), as long as the bulk length were long enough to assure uniqueness - and minimum of code length of 12 characters still seems a little long to me for a small store. Also, consider the use-case where a store sells coupons to resellers, who then sell or give those coupons to their customers. There may be only a handful of resellers, each buying thousands of coupons at a time - so for them it would be better if the bulk code were longer, and the purchase-suffix (for want of a better term) shorter.

Anyway, as I say, it's a small think and I can live with it as it is if you really think it's necessary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Currently, the coupon codes (single or bulk prefix) must be unique, this is enforced in other parts of the code, so we need to add a random suffix of some kind. I intend to revisit this and remove this restriction if possible at a later date, and also possibly reduce the minimum bulk length from 8 (which should be possible if you don't need thousands of codes).

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Nutty’s picture

Nutty commented

I'm having trouble reconciling this patch with the latest dev release. Any chance you are ready to revisit these restrictions at this point, longwave?

wodenx’s picture

wodenx commented
Status: Closed (fixed) » Postponed (maintainer needs more info)

The patches in this thread were committed long ago - so I'm not sure what you mean by reconciling them with the latest DEV. What is your use-case and what change are you proposing?

Nutty’s picture

Nutty commented

Yes, ignore the first sentence of my previous comment. I was having trouble because the general purpose of this patch is still intact in the latest release.

The second sentence refers to longwave's quote:
"I intend to revisit this and remove this restriction if possible at a later date, and also possibly reduce the minimum bulk length from 8 (which should be possible if you don't need thousands of codes)."

I'd prefer customers need not enter a (minimum) 13 character code (code name 1 + bulk prefix 4 + code 8), as it is much too long for my use case of few affilliates with relatively few bulk coupon codes required.

On a somewhat related note, an optional limitation to "safe" characters might not be a bad idea, taken to an extreme in the last comment of this node: http://groups.drupal.org/node/25985 (but in any case leaving out 0's and O's in coupon codes).

Nutty’s picture

Nutty commented

I opened a new issue as to my last point: http://drupal.org/node/1164034

wodenx’s picture

wodenx commented

I agree with you about the code length - and when I have time I'll go through the code and try to figure out if the restrictions are really necessary. In the meantime, you could try the following changes:

In uc_coupon.admin.inc at line 398, change

    '#options' => drupal_map_assoc(range(8, 30)),

to

    '#options' => drupal_map_assoc(range(4, 30)),

Similarly in uc_coupon_purchase.module at line 284, change

    '#options' => drupal_map_assoc(range(4, 30)),

to

    '#options' => drupal_map_assoc(range(1, 30)),

Or, really, whatever you want the minimum length to be.

Regarding the "safe" characters option - it's a good idea, but a bit complicated since the algorithm that generates bulk coupon codes has to be repeatable. If you or someone else wants to work up a patch for it, I'll be happy to review.

Nutty’s picture

Nutty commented

The solution in #23 appears to work just fine (although a similar change will need to be done for assigning coupons for the uc_coupon_tracking sandbox submodule)

wodenx’s picture

wodenx commented

actually - uc_coupon_tracking uses the same mechanism as uc_coupon_purchase mechanism for generating new coupons, so these changes should propagate. The only thing to be careful of here is that you make the lengths too short to ensure that every new coupon has a unique code. Two or more coupons sharing the same code would cause problems. I suppose we could add a check to enforce this.

Nutty’s picture

Nutty commented

Hm...

With the settings in 23, if I create a coupon with name "T" as a bulk coupon, the codes are "T" + 4 characters. When I assign this coupon, it becomes "T" + 8 character prefix, followed by 4 character codes.

wodenx’s picture

wodenx commented

Oh I see - yes there's another check. Change line 579 of uc_coupon_purchase.module to

    $suffix_length = max($suffix_length - $coupon->data['bulk_length'], 1);

Then you can set the "Purchased coupon suffix length" to 5 and you should end up with both bulk and single coupons having a code length of 6. But if you assign bulk coupons to more than a very few affiliates you may have problems.

Nutty’s picture

Nutty commented

It looks like you meant line 379. But that didn't do the trick.

I imagine line 375:
$coupon->data['bulk_length'] = 8;
might have something to do with it.

wodenx’s picture

wodenx commented
StatusFileSize
new shorter-code-length.patch1.76 KB

Sorry line 379 yes. To clarify I've attached a patch. With this patch to the latest DEV if I:

1. create a coupon with code T
2. set the "Code Length" under "Bulk Coupon Codes" to 4
3. set the "Purchased coupon code suffix length" under "Coupon purchase/assignment options" to 1.
4. assign this coupon to an affiliate
The assigned coupon has codes that are 6 characters long (T + [one-character-suffix] + [4-character-bulk-code])

But again - this is dangerous. I did a bit of calculation. Unless I've made an egregious statistical error, with the settings above the probability of having two identical codes reaches 50% after creating about 8 100-code bulk coupons.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Yeah, the possibility of code clash was why I was somewhat reluctant to decrease the code length in the first place - I think this needs some proper mathematical investigation to determine the lowest safe number that should be available.