Percentage based discounts : validation needs to be elsewhere

Are you suggesting to remove the condition at all? Like this? :

-  if ($rate > 1) {
    $rate = $rate / 100;
-  }

Thanks for the patch, Alienpruts.

Tested the patch successfully, including the admin ui part.

There is a round up part from my prev. task #2458621: Discount rate mis-calculated for discount percentages < 1 that has not been carried over, but that may be another ticket, since has nothing to do with validation. Maybe makes sense to leave it as it is in this patch, and hope for faster RTBC and inclusion upstream.

Tested and it appears to be working fine.

Thanks for the patch.

I'm a fan of this, it cleans things up a bit. So big +1 to this.

Just needs those TODOs resolved though I do like the intent behind them. They also need the LANGUAGE_NONE constant instead of 'und'.
And to be nice an update hook to convert any previous values form decimal to out of 100.

1. +++ b/commerce_discount.rules.inc
   @@ -178,14 +178,10 @@ function commerce_discount_fixed_amount(EntityDrupalWrapper $wrapper, $discount_
   -  if ($rate > 1) {
   -    $rate = $rate / 100;
   -  }
   

   So this will prevent people from using 0.5 as 50%. That makes sense, but maybe we should add an update hook in case people were using this hidden feature?

   Just needs to loop through the field_data_commerce_percentage table and check if there are any percent values and convert them.

2. +++ b/includes/commerce_discount.admin.inc
   @@ -278,6 +278,12 @@ function commerce_discount_form_validate($form, &$form_state) {
   +    // TODO : form_set_error does not highlight the percentage box (wrong element name?)
   

   This todo should be resolved before commit but I like the idea.

Bumping

Those tags don't really help, removing.

Thanks, hopefully you have some time @Alienpruts. I can help with the upgrade path if you want and you can just deal with the @todos?

+++ b/includes/commerce_discount.admin.inc
@@ -304,6 +304,11 @@ function commerce_discount_form_validate($form, &$form_state) {
+  // TODO : should we introduce more form validation?
+  $percentage = $form_state['values']['commerce_discount_fields']['commerce_discount_offer'][LANGUAGE_NONE]['form']['commerce_percentage'][LANGUAGE_NONE][0]['value'];
+  if (!empty($percentage) && $percentage <= 0) {
+      form_set_error('commerce_discount_fields][commerce_discount_offer][und][form][commerce_percentage', t('Percentage should be a positive number, excluding zero!'));

I think we can fix this @TODO by using form_error() and passing the form element. That will read #parents and set error properly.

I meant to roll a patch last night but failed. That's how the field API sets errors I believe.

EDIT: Just saw previous comment say element was highlighting. We're still hard coding [und], though, which is why mentioned solution would be useful.

Here's updated patch to fix tests.

I was thinking we could do something like...

form_error($form['commerce_discount_fields']['commerce_discount_offer'][LANGUAGE_NONE]['form']['commerce_percentage'], t('Percentage should be a positive number, excluding zero!'));

I haven't had the chance to debug the form structure to see if that has proper #parents structure, though. But that should be the "proper" way to set the error on the Field API element.

I also wonder if we could write a test for this, too. There is a Discount UI test, but it only works for fixed discount. Would be nice to have a Discount UI Percentage Validation test. Shows it invalidates with bad value, creates discount properly.

I'm willing to commit this without the UI tests. We can make that a follow-up and I don't mind writing them either.

Attached is an interdiff with nitpicks, could you apply that too with your improvements to the validation @Alienpruts?

Looks good. Update function tested out. Percentage calculations look accurate. In the update function return text add a space like 'update! '

@Alienpruts you're correct that if more fields are added to the 'percentage' bundle, and those fields require validation beyond what the field type already provides, than commerce_discount_form_validate() will need to be updated.

Ok I've committed this as is (with the interdiff). @Alienpruts if you have better validation stuffs or any tweaks you can post them here or in a follow-up issue I don't mine.

Weirdly enough, I downloaded the latest dev of Discount, and it seems I was able to apply a discount even though the coupon error'd out on me, if the Discount was enabled but the Coupon was not. The discount would apply and show up in the cart summary pane, but in the coupon pane I'd get the error "Could not apply coupon".