Gift Receipt/enclosure

Hi, this patch contains the work. Price is removed from the receipt and gift receipt text is added in the footer. Please review.

Thanks

@sorabh.v6 patch looks good at first glance, can you roll it for 8.x first. I'd like to do any new functionality in 8 first and backport to 7.

Note: this issue was flipped to 8.x, flip back to 7.x once 8.x is committed.

@smccabe Sure, I will create the patch for 8.x also.

Patch uploaded for 8.x

Please review.

1. +++ b/modules/receipt/src/Controller/PrintController.php
   @@ -94,19 +101,25 @@ class PrintController extends ControllerBase {
   +    $order_date = \Drupal::service('date.formatter')->format($commerce_order->getCompletedTime());
   

   As best practice, services should be loaded via dependency injection. Sources: https://docs.acquia.com/articles/drupal-8-dependency-injection, https://www.drupal.org/docs/8/api/services-and-dependency-injection/serv...

2. +++ b/modules/receipt/src/Controller/PrintController.php
   @@ -94,19 +101,25 @@ class PrintController extends ControllerBase {
   +      $gift_order = '<strong>GIFT ORDER</strong>';
   

   String translation missing.

3. +++ b/modules/receipt/src/Controller/PrintController.php
   @@ -94,19 +101,25 @@ class PrintController extends ControllerBase {
   +    else {
   +      $gift_order = '';
   +    }
   

   $gift_order can be defaulted above. Then we won't need this `else` statement.

4. +++ b/src/Form/POSForm.php
   @@ -285,6 +292,12 @@ class POSForm extends ContentEntityForm {
   +      if ($form_state->getValue('gift_order')) {
   +        $this->entity->setData('gift_order', TRUE);
   +      }
   +      else {
   +        $this->entity->setData('gift_order', FALSE);
   +      }
   

   $this->entity->setData('gift_order', $form_state->getValue('gift_order')) maybe?

I will work on them.

Sorry some important work came up.

Patch rerolled

:face palm:

1. +++ b/modules/receipt/src/Controller/PrintController.php
   @@ -62,44 +90,52 @@ class PrintController extends ControllerBase {
   +        if (!empty($adjustment)) {
   

   if ($adjustment)?

2. +++ b/modules/receipt/src/Controller/PrintController.php
   @@ -111,19 +147,22 @@ class PrintController extends ControllerBase {
   +        '#markup' => check_markup($config->get('header'), $config->get('header_format')) . "<br>$order_date<br><strong>Order# {$commerce_order->id()}</strong><br>$gift_order<hr>",
   

   The concatenated string is not translated.

Stepping out for a while. Not yet started #15

1. +++ b/modules/receipt/src/Controller/PrintController.php
   @@ -111,19 +147,22 @@ class PrintController extends ControllerBase {
   +    if ($is_gift) {
   +      $gift_order = '<strong>' . $this->t('GIFT ORDER') . '</strong>';
   +    }
   ...
   -        '#markup' => check_markup($config->get('header'), $config->get('header_format')),
   +        '#markup' => check_markup($config->get('header'), $config->get('header_format')) . "<br>$order_date<br><strong>Order# {$commerce_order->id()}</strong><br>$gift_order<hr>",
   ...
   -        '#markup' => check_markup($config->get('footer'), $config->get('footer_format')),
   +        '#markup' => '<hr>' . check_markup($config->get('footer'), $config->get('footer_format')),
   

   Rather than chucking markup in here how about passing the order to the template and putting as much markup as possible in the template.

2. +++ b/src/Form/POSForm.php
   @@ -93,6 +93,8 @@ class POSForm extends ContentEntityForm {
   +    // Gift order default value.
   +    $gift_value = $order->getData('gift_order');
   

   Do we want to store this information in the dumping ground that is the data field or should we store this in its own field? I can see arguments for both. I guess this fits into Used to store temporary data during order processing (i.e. checkout).

Added some changes as suggested by Alex #17. Not complete though. #15 still needs to be done.

Still not complete.

And also when I wanted to print the receipt, it didn't work it says `/ajax-receipt` 404 not found.

@subhojit777 I don't think the Show receipt action works at the moment - it only seems to work directly from the POSForm.

@alexpott Yes I was talking about the POSForm. You select Print and click Complete Order and you get an error in the console.

Partially fixed the problem as mentioned in #22

+++ b/modules/receipt/templates/commerce-pos-receipt.html.twig
@@ -17,7 +17,7 @@
+            <br>{{ order.getChangedTime|format_date('medium') }}<br>

This is just a workaround. getCompletedTime will never be obtained unless the order is completed. And that is not going to happen due to this #2935353-5: Site error during "Show receipt" operation

I checked Drupal 7. Couldn't find this "gift order" functionality. I have changed the date in receipt to order creation time. The changes also includes fix suggestions on #15.

Re #17:

Do we want to store this information in the dumping ground that is the data field or should we store this in its own field? I can see arguments for both. I guess this fits into Used to store temporary data during order processing (i.e. checkout)

I think associating gift_order with the entity should be better. For future reference. Suppose we need to create a view and would want to list whether the POS order was a gift order, then this would help.

Just a few UI recommendations.

1. +++ b/modules/receipt/templates/commerce-pos-receipt.html.twig
   @@ -13,8 +14,16 @@
   +            <strong>{% trans %}Order# {{ order.id|placeholder }}{% endtrans %}</strong>
   

   Right now the heading looks like this: Order# 32224. I believe it should look like this: Order #121314.

2. +++ b/src/Form/POSForm.php
   @@ -107,7 +109,12 @@ class POSForm extends ContentEntityForm {
   +    // Checkbox if this is gift order.
   

   I totally missed this underneath the order items fieldset. Would this be better placed in the right column next underneath the totals columns and just above the 'Payments and Completion' button?

The cart checkbox appears after "Adustments" fieldset, and it is also not much visible.

Do we have to add tests here?

@subhojit777 whilst jQuery.print is a mandatory requirement and not a progressive enhancement tests for receipt functionality are awkward at best.

1. +++ b/modules/receipt/templates/commerce-pos-receipt.html.twig
   @@ -13,8 +14,16 @@
   +            <strong>{% trans %}Order #{{ order.id|placeholder }}{% endtrans %}</strong>
   

   Are we sure that we want the order ID wrapped in <em class="placeholder">ID</em>. I think that maybe we should just print the order.id here without passing to the placeholder twig filter.

2. +++ b/src/Form/POSForm.php
   @@ -69,16 +69,19 @@ class POSForm extends ContentEntityForm {
   -      $form = $this->buildOrderForm($form, $form_state);
   +      $this->buildOrderForm($form, $form_state);
        }
        elseif ($step == 'payment') {
   -      $form = $this->buildPaymentForm($form, $form_state);
   +      $this->buildPaymentForm($form, $form_state);
   

   I might be missing something but what is the reason for this change? In general passing by reference and using that to affect change is harder to reflect on than just doing return values.