When adding a coupon via the admin order UI form, the coupon is not validated.

Before I add a custom call to commerce_coupon_code_is_valid in a form_alter, I'm just wondering if perhaps it makes sense to merge the two functions and to call

commerce_coupon_code_is_valid
within
commerce_coupon_redeem_coupon

That way the coupon will always be validated prior to redemption, rather than manually validating elsewhere.

Let me know if suits and can submit a patch.

Cheers,

DT

Comments

pcambra’s picture

Status: Active » Closed (works as designed)

Yeah, I was thinking on this one when I developed the UI for orders, but in the end I think that when you're in the admin UI you know what you're doing and have the right permissions so the validation doesn't make much sense.

About joining them, I think it's better for the API (we should have API docs btw), to have them separated.

If we can somehow manage this by settings, I.E one specific permission to redeem coupons via UI admin form without validation and another one that requires validation, I'm fine with that.

davidwhthomas’s picture

Thanks for the reply.

...in the end I think that when you're in the admin UI you know what you're doing and have the right permissions so the validation doesn't make much sense.

I'd have to disagree on that one, for example the ability to add expired coupons, inactive coupons and coupons that have exceeded their maximum use should proabably be restricted.

A user who can administer orders may not necessarily be able to administer coupons.

Our current project has a subscription administrator type role managing subscription orders via the admin interface and I need to prevent them shooting themselves in the foot via adding invalid coupons to subscription, that are then processed via the payment gateway.

It looks like that form_alter may be the ticket as I did get the coupons validating by that approach.

If I get time, I'll look into adding a permission to bypass validation on the admin page, along with the form_alter to submit as a patch.

Cheers,

DT

pcambra’s picture

Status: Closed (works as designed) » Active

It makes totally sense, let's reuse the same issue.

Probably we need to check that permission in the validation of the redeem of the coupon so the form alter is not required?

Thanks

davidwhthomas’s picture

Well, if the validation check commerce_coupon_code_is_valid was run when the coupon was redeemed ( in the same function ) then the permission check could be there only.

I'm not sure when a coupon would need to be validated without redeeming it - is there a use case for the separate API functions there?
If there's a permission to control validation bypass, then there's presumably no use case for redeeming a coupon without validating it ( assuming the access permission check passes )

Given they are currently two separate functions, and there's no validation in commerce_coupon_redeem_coupon I think it will still need the form_alter to prevent the coupon being directly redeemed when invalid.

Alternatively, merging the validation with coupon redemption and adding a permission to bypass validation could be the way forward here - is that what you mean?

Thanks again,

DT

pcambra’s picture

Category: bug » feature

Let's call this a feature request, but it would be really nice to have and clear up a little the validation process.

davidwhthomas’s picture

I ended up doing this in hook_form_commerce_order_ui_order_form_alter by adding an #element_validate handler to the entityreference field commerce_coupon_order_reference.

The validation is borrowed from entityreference, but adds some additional checks for a valid coupon.

First the hook_form_commerce_order_ui_order_form_alter code

<?php
function EXAMPLE_form_commerce_order_ui_order_form_alter(&$form, &$form_state, &$form_id) {
  // Add element validate handler to coupon reference field
  if(isset($form['commerce_coupon_order_reference'])){
      $language = isset($form_state['commerce_order']->language) ? $form_state['commerce_order']->language : LANGUAGE_NONE
      foreach($form['commerce_coupon_order_reference'][$language] as $key => $value){
        if(is_numeric($key)){
          $form['commerce_coupon_order_reference'][$language][$key]['target_id']['#element_validate'][] = 'EXAMPLE_commerce_coupon_order_reference_validate';
        }
      }
  }
}
?>

then the element validate handler

<?php
function EXAMPLE_commerce_coupon_order_reference_validate($element, &$form_state){

  if (!empty($element['#value'])) {
    // Parse "coupon code (entity id)', match the id from parenthesis.
    // Get the coupon code and the coupon id values
    if (preg_match("/.+\((\d+)\)/", $element['#value'], $matches)) {
      $coupon_id = $matches[1];
      // Coupon code
      $coupon_code = trim(str_replace('('.$matches[1].')', '', $element['#value']));
    }else{
      $coupon_code = $element['#value'];
    }
    // Pass if coupon already applied to order
    $language = isset($form_state['commerce_order']->language) ? $form_state['commerce_order']->language : LANGUAGE_NONE
    if($coupon_id && isset($form_state['commerce_order']->commerce_coupon_order_reference[$language])){
      foreach($form_state['commerce_order']->commerce_coupon_order_reference[$language] as $key => $value){
        if($value['target_id'] == $coupon_id){
          // Already added to order
          return TRUE;
        }
      }
    }
    if(commerce_coupon_code_is_valid($coupon_code, $element['#entity'])){
      return TRUE;
    }else{
      // Show validation error
      form_set_error($element['#field_name'], t('Invalid coupon code')); 
      return FALSE;
    }
  }
  
}
?>

If interested, perhaps I can roll it as a patch.

Cheers,

David

liupascal’s picture

By not "Validated" you mean that the coupon's conditions are not checked before attaching it to the order ?

I just create an order with coupon fixed amount on a fresh kickstart install, and it seems that the coupon's "effect " (say -20€ on the order) is not applied on the order either.
-> Should I open a new issue for that ?
-> Is it the intended behavior ? (not to automatically apply the coupon through the ui, just let an admin attach it ?)
-> Or should we consider this related to this issue ?

Many thanks

pcambra’s picture

I just create an order with coupon fixed amount on a fresh kickstart install, and it seems that the coupon's "effect " (say -20€ on the order) is not applied on the order either.

I think that's nothing to do with this issue, does it?

davidwhthomas’s picture

@liupascal #7 That's a different issue, this issue is about validating the coupon when adding as admin.

I'm still using the snippet from #6, tis working ok.

DT

pcambra’s picture

@davidwhthomas could you please provide a patch for what you think should fix this issue?
Thanks

liupascal’s picture

I was indeed talking about coupons being added to an order while "manually" creating the order.
But it's clear that this topic is about the conditions of the coupon. Sorry I misunderstood, i'll create another topic !