Clear current order when closing your register

This could either clear out automatically or maybe a prompt could appear asking you to void that transaction before you are able to close register. Whichever way would work as long as it is gone by the next open.

Here's another idea. Let's completely remove the concept of current order. It's not necessary. Without a current order this behaviour goes away.

12 files changed, 62 insertions, 195 deletions. is the kind of diff stat I like.

Fixing the tests.

For me this change makes the parked order concept make sense since now we don't save an order unless you park it or complete it.

1. +++ b/commerce_pos.routing.yml
   @@ -32,10 +32,15 @@ commerce_pos.base:
    commerce_pos.main:
   -  path: '/admin/commerce/pos/main'
   +  path: '/admin/commerce/pos/main/{commerce_order}'
      defaults:
        _controller: '\Drupal\commerce_pos\POS::content'
        _title: 'Point of Sale Interface'
   +    commerce_order: null
   +  options:
   +    parameters:
   +      commerce_order:
   +        type: 'entity:commerce_order'
   
   @@ -89,15 +94,3 @@ commerce_pos.close_register:
   -commerce_pos.edit:
   -  path: '/admin/commerce/pos/order/{commerce_order}/edit'
   -  defaults:
   -    _controller: '\Drupal\commerce_pos\POS::content'
   -    _title: 'Point of Sale Edit Order'
   -  options:
   -    parameters:
   -      commerce_order:
   -        type: 'entity:commerce_order'
   -  requirements:
   -    _permission: 'access commerce pos pages'
   

   Another advantage of this change is that now editing an order has the same UI as creating - ie with the park buttons etc...

2. +++ b/tests/src/FunctionalJavascript/ParkOrderTest.php
   @@ -100,9 +101,21 @@ class ParkOrderTest extends JavascriptTestBase {
   +    // Assert that the product is listed as expected.
   +    $web_assert->pageTextContains('Jumper');
   +    $web_assert->fieldValueEquals('order_items[target_id][order_items][0][quantity]', '1.00');
   +    $web_assert->fieldValueEquals('order_items[target_id][order_items][0][unit_price][number]', '50.00');
   +    $web_assert->pageTextContains('Total $50.00');
   +    $web_assert->pageTextContains('To Pay $50.00');
   

   This is a big improvement to this test. We weren't testing that the parked order that was retrieved correctly populated the form.

Re-ordering the files.

This will fail now I've committed #2944411: Add test for commerge_log integration. Here's a fix.

So it behaves like a normal entity. Which is good because we were only saving an order on certain actions but not all. What we need is a piece of JS saying don't leave the page without parking.

But the problem is, for example, at the moment if you change the customer or add an adjustment the order is not saved. We only save the order if you add an order item or add a note.

Anyhow we can still remove the current order service and encapsulate it more to the POS class which should prevent it's usage everywhere - and also just store the ID so we're not providing something so convenient you wouldn't use the form state entity. And the patch adds back all the points where we saved the order before.

No interdiff cause rebase. Also we need to add tests around how current order works so we have the expectations tested. And we need to add tests about what happens if the register is closed.

Here's a test that ensures the expected current order logic works. One difference to the issue title summary is that if the current order matches the newly re-opened register it is not removed. I think that that makes sense. If you open a different register it is cleared.

There was a bot wobble seemingly.

Per Alex's point, we are postponing #2938694: Allow a Register to be changed until this patch lands.

Conversely, this code also prevents you from ever loading up an order on a different register, which you may want to do if you are resuming a parked order from a different till.

This makes sense but then what should the order's register be set to? The register that initiated or the register that finally took payment? My guess is that it should be updated on unparking.

A couple of nits. Leaving open for someone to review for functionality.

1. +++ b/src/Form/POSForm.php
   @@ -761,8 +777,11 @@ class POSForm extends ContentEntityForm {
      /**
       * Clear the existing order, so a new one can be created.
       */
   -  protected function clearOrder() {
   -    \Drupal::service('commerce_pos.current_order')->clear();
   +  protected function clearOrder(FormStateInterface $form_state) {
   

   Missing @param

2. +++ b/src/POS.php
   @@ -76,38 +70,43 @@ class POS extends ControllerBase {
   +      // If we're opening a new register, clear our current order if it exists
   +      // we don't want to pick up some older order at this point.
   +      $this->tempStore->set('current_order_id', FALSE);
   

   run-on sentence. Better:

   // If we're opening a new register, clear our current order. If it exists
   // we don't want to pick up some older order at this point.
   

3. +++ b/src/Plugin/Field/FieldWidget/PosOrderItemWidget.php
   @@ -422,7 +424,7 @@ class PosOrderItemWidget extends WidgetBase implements WidgetInterface, Containe
   +    $order = $form_state->getFormObject()->getEntity();
   
   @@ -454,11 +454,9 @@ class PosOrderItemWidget extends WidgetBase implements WidgetInterface, Containe
   +    $order = $form_state->getFormObject()->getEntity();
   
   @@ -583,11 +572,11 @@ class PosOrderItemWidget extends WidgetBase implements WidgetInterface, Containe
   +    $order = $form_state->getFormObject()->getEntity();
   

   Is this better than getting it from the tempStore current_order_id variable?

4. +++ b/src/Plugin/Field/FieldWidget/PosOrderItemWidget.php
   @@ -499,20 +497,15 @@ class PosOrderItemWidget extends WidgetBase implements WidgetInterface, Containe
   +    $order->addItem($new_order_item);
   

   Naming convention: We changed "$order" to "$commerce_order" above. Perhaps we should settle on one and change it all over?

5. +++ b/tests/src/FunctionalJavascript/PosFormTest.php
   @@ -381,6 +383,93 @@ class PosFormTest extends JavascriptTestBase {
   +    $this->createScreenshot(\Drupal::root() . '/sites/default/files/simpletest/screen.jpg');
   ...
   +    $this->createScreenshot(\Drupal::root() . '/sites/default/files/simpletest/screen2.jpg');
   

   Are we just creating screenshots here or do we test for "expected screenshot similarity"?

Let's file a follow-up for the $order standardisation. We'll need to change route parameters and go through the code with a fine tooth comb. It's just creating a lot of noise on this issue. Reverting this from #30 and fixing the PosForm too.

+++ b/src/Controller/POS.php
@@ -76,38 +70,43 @@ class POS extends ControllerBase {
+      $commerce_order = Order::load($current_order_id);
+      // Order may not have started on this register, so always set the register.
+      $commerce_order->set('field_register', $register->id());

Need to add test coverage of this.

Here's a test which yet again proves the old adage

Untested code is broken code

.

I think fixing the code so that the order on the register that completes it is a major bug fix.

Updating for changes to HEAD.

Rerolled after some cleanup.

Created #2948455: Standardise on order over commerce_order as a followup

I tried to find some knit picks :D. The patch is from @alexpott and very hard to find any knit picks. I checked the functionality and its working fine. I close my register it takes me to end-of-day reports page when I close the register. Then I again opened the POS by selecting the same register and the draft order was already gone.

Setting it to RTBC.