Allow for bulk creation of coupons

Why would it be a contib if we have this feature request for it?

This initial patch has a form implementation but no batch and no tests yet. I will add test code incrementally and then start working on the batch functionality.

Added kernel test for coupon code provider service.

Misplacement of file and syntax error fixed.

Missing order type config.

Still missing modules/schema. Added others.

Added batch processing to the form. Still need testing for the form.

Added functional test code for bulk coupon generation (CouponTest).

Reduced coupon quantity.

@lisastreeter

Does this test pass on your local install?

@drugan Sorry to clog up the issue with all my failures!

Unfortunately, I don't have local testing set up for myself yet. (I'm still new to Drupal development.) But at the moment, I'm putting it off until after an upcoming training session (related to that) I've registered for at MidCamp. So for now, I'm just using the test server here to test patches.

This is my first time working on a Functional test, and my first time implementing a batch in a form, so there have been lots of little bugs as I'm learning as I go. I think I'm getting close. Hope so, at least.

OK, if the #31 will fail I'll try to debug the patch on my local install.

@drugan Success!! Thank you for the support and offer to help. I appreciate it. Last night the big thing I was stuck on was getting drupalPostForm to work. After that, it was just little things. Gotta get my local dev environment properly set up soon!

Congrats!

Yes, running FunctionalJavascript tests might be seen as a bit tricky. Feel free to ping me on the question if you'd have some troubles with it.

Oops..

Great work so far!
And now my favorite hobby, nitpicking naming :)

+entity.commerce_promotion_coupon.generator_form:
+  path: '/promotion/{commerce_promotion}/coupons/generate'
+  defaults:
+    _form: '\Drupal\commerce_promotion\Form\CouponGeneratorForm'
+    _title: 'Generate coupons'
+  options:
+    parameters:
+      commerce_promotion:
+        type: 'entity:commerce_promotion'
+  requirements:
+    _permission: 'generate bulk commerce_promotion_coupon'

We tend to use NounVerbForm, so we want CouponGenerateForm and entity.commerce_promotion_coupon.generate_form.

+generate bulk commerce_promotion_coupon:
+  title: 'Generate bulk coupons'

Is this valid grammar-wise? I've only seen "Bulk generate" before (VS "generate bulk").

+    $base_coupon = $this->couponStorage->create([
+      'promotion_id' => $this->promotion->id(),
+      'usage_limit' => $values['usage_limit'],
+      'status' => $values['status'],
+    ]);

+      'operations' => [[[$this, 'batchAddCoupons'], [$quantity, $base_coupon, $pattern]]],

You are serializing an entire entity into the batch, which is very expensive. Would be much better to pass the three values in an array.

+      $newCoupon->setCode($code);

You should not use camelCase naming for local variables.

+  public function __construct($type, $prefix, $suffix, $length) {
+    $this->type = $type;

We're missing sensible defaults for $prefix, $suffix, $length ($prefix and $suffix can be empty strings, $length should be the default from the form). We're also missing code that validates $type and throws an \InvalidArgumentException if the value is unknown. I've explained below that we should have the possible types as constants on the value object.

+/**
+ * Provides functionality for checking coupon code uniqueness and randomly
+ * generating new coupon codes.
+ *
+ * Coupon codes are the unique, machine-readable identifiers for a coupon.
+ */
+interface CouponCodeProviderInterface {

"Provider" is a very generic name, currently only used for the CartProvider in Commerce.
I would go for "CouponCodeGeneratorInterface", cause the primary purpose of the class is to generate coupon codes.
That also matches your form naming. Your docblock can then be more concrete as well:
"Generates coupon codes (unique, machine readable coupon identifiers)."
or
"Generates coupon codes (unique, machine readable identifiers for coupons)."

+  /**
+   * Gets the list of pattern types handled by the coupon code provider.
+   *
+   * @return array
+   *   An array of coupon code pattern type labels keyed by string ids.
+   */
+  public function getCodePatternTypes();

Having this method here is odd, I'd expect the CouponCodePattern to have constants for the three types (CouponCodePattern::TYPE_NUMERIC, etc). Then the form itself would provide the labels for each constant.

+  /**
+   * Gets whether the code pattern is valid for the coupon quantity.
+   *
+   * @param \Drupal\commerce_promotion\CouponCodePattern $pattern
+   *   The coupon code pattern.
+   * @param int $quantity
+   *   The coupon quantity.
+   *
+   * @return bool
+   *   TRUE if the pattern is valid, FALSE otherwise.
+   */
+  public function isCodePatternValidForQuantity(CouponCodePattern $pattern, $quantity);

"Gets" is for getters, this isn't one, it has real logic.
I'd say "Validates the given pattern for the specified quantity." and call it validatePattern($pattern, $quantity = 1);

+  /**
+   * Generates a new coupon code.
+   *
+   * @param \Drupal\commerce_promotion\CouponCodePattern $pattern
+   *   The coupon code pattern.
+   *
+   * @return string|null
+   *   The coupon code or NULL if a unique code cannot be generated.
+   */
+  public function getCode(CouponCodePattern $pattern);

The verb in the method name should match the verb in the docblock. We have generates/get here. Since "get" is used for getting properties, we tend to avoid it in methods that do something, so this method should be generateCode. Furthermore, I'd argue that we actually want generateCodes(CouponCodePattern $pattern, $quantity = 1), cause that would allow for performance optimizations in the method itself. More on that later.

+    // Attempt to generate unique code. Limit to 1000 iterations.
+    for ($i = 0; $i < 1000 && $code_found == FALSE; $i++) {
+      $code = $prefix;
+
+      // Create the code per character.
+      for ($c = 0; $c < $length; $c++) {
+        $rand_index = mt_rand(0, $character_set_size - 1);
+        $code .= $character_set[$rand_index];
+      }
+
+      $code .= $suffix;
+      // Check code uniqueness.
+      if (!$this->codeExists($code)) {
+        $code_found = TRUE;
+      }
+    }

Each codeExists() call runs a DB query. So we're running up to a thousand queries per batch.
We need to pass a $quantity to this method, generate the codes all at once, then check their uniqueness all at once.
I suggest limiting the batch to 25 codes per run. Then, this method generates $quantity * 2 codes, checks their uniqueness at once, removes all non-unique codes, and returns the first $quantity ones. The form could also check the total number of generated codes and suggest that a prefix/suffix is used the next time.

+  protected function getCharacterSet($pattern_type) {

This method needs an explanation for why not every letter/number is used (just like the D7 code had).

Patch provides changes in response to code review #37 (@bojanz)

Removed unused use statements and cleaned up a few after things after reviewing interdiff.

Thanks! This looks almost ready.

+    // Check whether any codes already exist.
+    $result = $this->connection->select('commerce_promotion_coupon', 'c')
+      ->fields('c', ['code'])
+      ->condition('code', $unique_codes, 'IN')
+      ->execute();

Why did we switch from an EntityQuery to a database query? Was there a problem with the IN condition there?

Also, let's look at the UI:
1) The generate form is rendered in the frontend theme, which is not consistent with the other promotion pages which all use the backend theme. You are missing a _admin_route: TRUE on the route.

2) "Number of coupons to generate?" -> we don't use question marks in field labels. The field could use a default value (10? 20? 30? what do the others do?), and a longer length, see how it's not the same length as the "Code length" field.

3) "Code format" should have a value (alphanumeric?) preselected.

1. +++ b/modules/promotion/commerce_promotion.routing.yml
   @@ -10,3 +10,15 @@ entity.commerce_promotion_coupon.collection:
   +  options:
   +    parameters:
   +      commerce_promotion:
   +        type: 'entity:commerce_promotion'
   

   Since the route parameter matches an entity type ID, we do not need to explicitly define our options.

2. +++ b/modules/promotion/src/Form/CouponGenerateForm.php
   @@ -0,0 +1,314 @@
   +class CouponGenerateForm extends FormBase {
   

   I wonder if this should actually be based on `\Drupal\Core\Form\ConfirmFormBase`.

   The confirmation form would allow us to say

   * Here's an example coupon code
   * You will be making ### coupons that are (Enabled|Disabled) and can be used (X times|unlimited times)

3. +++ b/modules/promotion/src/Form/CouponGenerateForm.php
   @@ -0,0 +1,314 @@
   +          '%label' => ($results['total_quantity'] == 1) ? 'coupon' : 'coupons',
   ...
   +          '%label' => ($created == 1) ? 'coupon' : 'coupons',
   

   We need to pass these through `t()` still.

4. +++ b/modules/promotion/src/Form/CouponGenerateForm.php
   @@ -0,0 +1,314 @@
   +      '#title' => $this->t('Number of uses'),
   

   Let's add a description to help clarify this.

   "Number of times each generated coupon can be used"

@bojanz @mglaman Thanks for the code review. A quick response to a couple items before I create the patch for the updated UI items.

+ // Check whether any codes already exist.
+ $result = $this->connection->select('commerce_promotion_coupon', 'c')
+ ->fields('c', ['code'])
+ ->condition('code', $unique_codes, 'IN')
+ ->execute();
Why did we switch from an EntityQuery to a database query? Was there a problem with the IN condition there?

The IN condition was fine, but I needed the codes, not the coupon ids. That's why I switched to EntityQuery. (I also tried using a static instead of dynamic query but there was no noticeable performance difference. So I left it in the easier-to-read format.)

+++ b/modules/promotion/commerce_promotion.routing.yml
@@ -10,3 +10,15 @@ entity.commerce_promotion_coupon.collection:
+ options:
+ parameters:
+ commerce_promotion:
+ type: 'entity:commerce_promotion'
Since the route parameter matches an entity type ID, we do not need to explicitly define our options.

I made the change but then submitForm didn't work. For some reason, without the explicit parameter option, the promotion was never set in the constructor.

UI Improvements.

Last round!

+use Drupal\Core\StringTranslation\StringTranslationTrait;
+
+class CouponCodeGenerator implements CouponCodeGeneratorInterface {
+
+  use StringTranslationTrait;

StringTranslationTrait is not used anywhere.

+    // Generate a set of new random codes using the pattern.
+    $new_codes = [];
+    for ($j = 0; $j < ($quantity * 2); $j++) {
+      $code = $prefix;
+      for ($c = 0; $c < $length; $c++) {
+        $rand_index = mt_rand(0, $character_set_size - 1);
+        $code .= $character_set[$rand_index];
+      }
+      $code .= $suffix;
+      $new_codes[] = $code;
+    }

a. The comment needs to explain $quantity * 2. Something like "Generate twice the requested quantity, to improve chances of having the needed quantity after removing non-unique/existing codes."
b. $rand_index should be $random_index, we rarely shorten words.
c. Having nested for loops is a bit hard on the eyes, I'd suggest rewriting the inner one as:

      $code = '';
      while (strlen($code) < $length) {
        $random_index = mt_rand(0, $character_set_size - 1);
        $code .= $character_set[$random_index];
      }
      $new_codes[] = $prefix . $code . $suffix;

+    switch ($pattern_type) {
+      // No 'I', 'O', 'i', 'l', '0', '1' to avoid recognition issues.
+      case 'alphanumeric':

We should be using the defined constants for the pattern type matching.

+  /**
+   * The number of coupons to process in each batch processing pass.
+   */
+  const PASS_COUNT = 25;

A more common constant/variable name would be BATCH_SIZE.

+    $this->maxCodeLength = $entity_field_manager
+      ->getBaseFieldDefinitions('commerce_promotion_coupon')['code']
+      ->getSetting('max_length');

Injecting $entity_field_manager and looking at the field definition feels like a lot of work just to avoid breaching the 255 char limit. I'd rather hardcode it in a constant for simplicity.

+    // Validate maximum coupon quantity.
+    if ($values['coupon_quantity'] > self::COUPON_COUNT) {
+      $form_state->setError($form['coupon_quantity'], $this->t('Number of coupons must be less than or equal to @max_length.', ['@max_length' => self::COUPON_COUNT]));
+    }

We can replace this check, and the constant with a #max on the quantity field, no?

+    $batch = [
+      'title' => $this->t('Generating Coupons'),
+      'progress_message' => '',
+      'operations' => [[[$this, 'batchAddCoupons'], [$quantity, $coupon_values, $pattern]]],
+      'finished' => [$this, 'batchFinished'],
+    ];

Note that the batch is given batchAddCoupons, then batchFinished, but in code, the methods are defined in reverse, batchFinished comes before batchAddCoupons. I'd change the method ordering to match the order in which they're given to the batch. I'd also rename the methods to follow verbNoun, which is more common, so processBatch() and finishBatch().

+    /* @var \Drupal\commerce_promotion\Entity\Coupon $base_coupon */
+    $base_coupon = \Drupal::service('entity_type.manager')
+      ->getStorage('commerce_promotion_coupon')
+      ->create([
+        'promotion_id' => $coupon_values['promotion_id'],
+        'usage_limit' => $coupon_values['usage_limit'],
+      ]
+    );

You don't need a base coupon at all, just pass ['code' => $code] + $coupon_values when creating the coupon further down.

        drupal_set_message(t('Generated %created %label.', [
          '%label' => ($created == 1) ? t('coupon') : t('coupons'),
          '%created' => $created,
        ]));

Translated messages that depend on a count need to use \Drupal::translation()->formatPlural().

I'll wrap this up.

Houston, we have bulk coupon generation :)

@bojanz Thank you for the detailed description of the last of the code issues. It's very helpful. Thank you also for making the fixes.

Regarding this comment:

// Validate maximum coupon quantity.
    if ($values['coupon_quantity'] > self::COUPON_COUNT) {
      $form_state->setError($form['coupon_quantity'], $this->t('Number of coupons must be less than or equal to @max_length.', ['@max_length' => self::COUPON_COUNT]));
    }

We can replace this check, and the constant with a #max on the quantity field, no?

If I use #max on the quantity field, it doesn't allow me to set #length on the quantity field; instead, it sets it to a size just big enough to accommodate the 4 digit maximum. Which makes the UI look a little off, since the size of that text box doesn't match the size of all the others on the page.

This worked great. Thanks for the hard work.

But it doesn't work any longer (commerce 2.8). I get this error:

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: /batch?id=6233&op=do_nojs&op=do
StatusText: OK
ResponseText: Error: Call to a member function getPromotionId() on null in Drupal\commerce_promotion\Entity\Promotion->postSave() (line 505 of /var/www/html/web/modules/contrib/commerce/modules/promotion/src/Entity/Promotion.php).

Error: Call to a member function getPromotionId() on null in Drupal\commerce_promotion\Entity\Promotion->postSave() (line 505 of /var/www/html/web/modules/contrib/commerce/modules/promotion/src/Entity/Promotion.php)
#0 /var/www/html/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php(469): Drupal\commerce_promotion\Entity\Promotion->postSave(Object(Drupal\commerce_promotion\PromotionStorage), true)
#1 /var/www/html/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php(615): Drupal\Core\Entity\EntityStorageBase->doPostSave(Object(Drupal\commerce_promotion\Entity\Promotion), true)
#2 /var/www/html/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php(395): Drupal\Core\Entity\ContentEntityStorageBase->doPostSave(Object(Drupal\commerce_promotion\Entity\Promotion), true)
#3 /var/www/html/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php(820): Drupal\Core\Entity\EntityStorageBase->save(Object(Drupal\commerce_promotion\Entity\Promotion))
#4 /var/www/html/web/core/lib/Drupal/Core/Entity/Entity.php(391): Drupal\Core\Entity\Sql\SqlContentEntityStorage->save(Object(Drupal\commerce_promotion\Entity\Promotion))
#5 /var/www/html/web/modules/contrib/commerce/modules/promotion/src/Entity/Coupon.php(155): Drupal\Core\Entity\Entity->save()
#6 /var/www/html/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php(469): Drupal\commerce_promotion\Entity\Coupon->postSave(Object(Drupal\commerce_promotion\CouponStorage), false)
#7 /var/www/html/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php(615): Drupal\Core\Entity\EntityStorageBase->doPostSave(Object(Drupal\commerce_promotion\Entity\Coupon), false)
#8 /var/www/html/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php(395): Drupal\Core\Entity\ContentEntityStorageBase->doPostSave(Object(Drupal\commerce_promotion\Entity\Coupon), false)
#9 /var/www/html/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php(820): Drupal\Core\Entity\EntityStorageBase->save(Object(Drupal\commerce_promotion\Entity\Coupon))
#10 /var/www/html/web/core/lib/Drupal/Core/Entity/Entity.php(391): Drupal\Core\Entity\Sql\SqlContentEntityStorage->save(Object(Drupal\commerce_promotion\Entity\Coupon))
#11 /var/www/html/web/modules/contrib/commerce/modules/promotion/src/Form/CouponGenerateForm.php(238): Drupal\Core\Entity\Entity->save()
#12 /var/www/html/web/core/includes/batch.inc(294): Drupal\commerce_promotion\Form\CouponGenerateForm::processBatch('50', Array, Object(Drupal\commerce_promotion\CouponCodePattern), Array)
#13 /var/www/html/web/core/includes/batch.inc(137): _batch_process()
#14 /var/www/html/web/core/includes/batch.inc(93): _batch_do()
#15 /var/www/html/web/core/modules/system/src/Controller/BatchController.php(55): _batch_page(Object(Symfony\Component\HttpFoundation\Request))
#16 [internal function]: Drupal\system\Controller\BatchController->batchPage(Object(Symfony\Component\HttpFoundation\Request))
#17 /var/www/html/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array(Array, Array)
#18 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(582): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#19 /var/www/html/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(124): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure))
#20 /var/www/html/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array)
#21 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(151): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#22 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(68): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)
#23 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/Session.php(57): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#24 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(47): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#25 /var/www/html/web/core/modules/ban/src/BanMiddleware.php(50): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#26 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(47): Drupal\ban\BanMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#27 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(52): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#28 /var/www/html/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#29 /var/www/html/web/core/lib/Drupal/Core/DrupalKernel.php(666): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#30 /var/www/html/web/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#31 {main}.

Someone having the same problem
(do I need to make a new issue for this?)