Allow parking of orders

Are you talking about parking of orders as we do in D7 version?

@smccabe Another question, we will use serialised data or session for storing order data?

@smccabe One last question, should it work on ajax or will it okay without ajax as well?

The uploaded patch includes code that shows all parked orders in the pos form. It also allows unparking of particular order by clicking on its button, thereby making the current order to the parked state. Please review.

First of all, a better UX demands the Retrieve Transaction # button be at the top.
I could not really review the patch functionality because of some issues and errors.

A scenario:
- create a POS order from /admin/commerce/pos
- park it from /admin/commerce/orders/5 (, say order id #5)
- now go back to /admin/commerce/pos
- edit the Order items
- hit save on the right >> this will/should/must not create a new order (not sure)
- click Retrieve Transaction #5
- Order #5 could not be retrieved

@gauravjeet Not sure what you are trying to say. I applied patch on another d8 installation. And did what you mentioned. Its showing no error. And changing state of order from parked to draft. Please see attached gif -

The patch on #10 is out dated. Needs reroll. I am going to work on it.

I have made the reroll but haven't tested it yet. I will have to follow the gif as mentioned in #13 and the steps mentioned in #11

+++ b/src/Form/POSForm.php
@@ -4,6 +4,7 @@ namespace Drupal\commerce_pos\Form;
 use Drupal\Core\Entity\EntityManagerInterface;

This is deprecated. As mentioned by phpstorm.

@subhojit777 Yeah, it's deprecated. EntityTypeManagerInterface should be used instead. But in many places, we have the dependency for the EntityManagerInterface object.

#20 does this:

We're going to need a park button right on the main POS screen, I would add it right between "Add Payment" and 'delete' on page one, use the same styling as "delete"

Some observations and might be code improvement.

I observed that, after deleting the last/current order, when I revisit the main POS screen, it itself creates a new order. I am not sure where that logic is written.

Since it always creates a new order, then I think this return $order_id ? Order::load($order_id) : NULL inside CurrentOrder::get() can be altered to just return Order::load($order_id).

And this will save multiple current order checks (#20).

1. +++ b/src/CurrentOrder.php
   @@ -49,4 +49,36 @@ class CurrentOrder {
   +    if ($current_order) {
   

   Duplicate current order check.

2. +++ b/src/CurrentOrder.php
   @@ -49,4 +49,36 @@ class CurrentOrder {
   +    if ($current_order) {
   

   Duplicate current order check.

Covers

I would also add it to the payment page, put it after "Finish"

as mentioned in #18

Covers

The retrieve parked order button I would probably put at the very top, above the Order Item widget, although only show it if there is at least 1 parked order.

as in #18

As for the deprecated function, I believe we use that multiple places and we should open a separate issue to get it all at once, since it is not being added by this patch, but already exists for this class.

#2933884: Remove usage of deprecated entity.manager service

Do we have to add tests for this parking order feature?

From #18:

We're going to need a park button right on the main POS screen, I would add it right between "Add Payment" and 'delete' on page one, use the same styling as "delete"

Forgot to mention this -- The park order operation looks like a button, not like the Delete button. The Delete operation is actually, a link, not a button. We can add a custom style, style with class say button--secondary, and there we write styles so that the button looks like a link.

Please move this ticket to Needs work if you think this is necessary.

And how about this:

Do we have to add tests for this parking order feature?

From #32 @smccabe

Will work on the failing tests. Then will add some tests supporting the change.

The problem was introduced here #2901030: Receipt Printing and should have been addressed there itself.

Re #40:

Sorry. My bad. Not sure whether the problem got introduced in #2901030: Receipt Printing, since the tests are passing there. I tracked it from the commit.

+++ b/tests/src/FunctionalJavascript/PosFormTest.php
@@ -309,4 +309,42 @@ class PosFormTest extends JavascriptTestBase {
+    $element = $this->getSession()->getPage()->findAll('xpath', "//input[contains(@id, 'edit-parked-')]");

I was expecting to click the button by value Retrieve Transaction #<order_id> but I couldn't find a way to obtain order id from the test. This can be improved.

+++ b/src/CurrentOrder.php
@@ -49,4 +49,36 @@ class CurrentOrder {
+  public function isParked() {
...
+  public function park() {

Will add functional tests for these two.

quick cooked up version. Still working on it.

I don't think this is working as expected at the moment. When an order is parked we need to create a new order.

1. +++ b/src/Form/POSForm.php
   @@ -118,6 +131,15 @@ class POSForm extends ContentEntityForm {
   +    if (!$form_state->get('is_parked')) {
   
   @@ -233,6 +255,14 @@ class POSForm extends ContentEntityForm {
   +    if (!$form_state->get('is_parked')) {
   

   I'm not sure that we need this if. Retrieving an order unparks it.

2. @@ -447,6 +477,22 @@ class POSForm extends ContentEntityForm {
   + $parked_orders = $this->getParkedOrders();
   + if ($parked_orders) {
   + $form['parked'] = [
   + '#type' => 'container',
   + '#weight' => -1,
   + ];
   +
   + foreach ($parked_orders as $parked_order) {
   + $form['parked'][$parked_order] = [
   + '#type' => 'submit',
   + '#value' => t('Retrieve Transaction #@id', ['@id' => $parked_order]),
   + '#submit' => ['::parkedTransactionRetrieve'],
   + ];
   + }
   + }
   

   It's super weird to list parked orders at the top of the order screen. How did it work before?

3. +++ b/src/Form/POSForm.php
   @@ -118,6 +131,15 @@ class POSForm extends ContentEntityForm {
   +      $form['actions']['park_order'] = [
   +        '#type' => 'submit',
   +        '#value' => $this->t('Park Order'),
   +        '#weight' => 6,
   +        '#submit' => ['::parkOrder'],
   +      ];
   +    }
   
   @@ -233,6 +255,14 @@ class POSForm extends ContentEntityForm {
   +      $form['actions']['park_order'] = [
   +        '#type' => 'submit',
   +        '#value' => $this->t('Park Order'),
   +        '#submit' => ['::parkOrder'],
   +      ];
   

   Needs '#limit_validation_errors' => [] or we need validation since parking an empty order makes very little sense.

4. +++ b/src/Form/POSForm.php
   @@ -551,4 +597,44 @@ class POSForm extends ContentEntityForm {
   +  public function parkOrder(array &$form, FormStateInterface $form_state) {
   +    $this->currentOrder->park();
   +  }
   

   I think parking an order should create a new current order. Basically it should call \Drupal\commerce_pos\Form\POSForm::clearOrder().

I've just tried out the D7 functionality and the parked orders can be retrieved from order screen but they appear at the bottom (rather than the top) which makes more sense.

1. +++ b/src/CurrentOrder.php
   @@ -49,4 +49,36 @@ class CurrentOrder {
   +   * Checks if the current order is parked.
   +   *
   +   * @return bool
   +   *   TRUE if the current order is parked, otherwise FALSE.
   +   */
   +  public function isParked() {
   +    /** @var \Drupal\commerce_order\Entity\OrderInterface $current_order */
   +    $current_order = $this->get();
   +
   +    if ($current_order) {
   +      return $current_order->getState()->getValue()['value'] === 'parked';
   +    }
   +
   +    return FALSE;
   +  }
   +
   +  /**
   +   * Parks the current order.
   +   *
   +   * @throws \Drupal\Core\Entity\EntityStorageException
   +   */
   +  public function park() {
   +    /** @var \Drupal\commerce_order\Entity\OrderInterface $current_order */
   +    $current_order = $this->get();
   +
   +    if ($current_order) {
   +      $current_order->set('state', 'parked')
   +        ->save();
   +    }
   +  }
   

   I don't think this is necessary. PosForm::parkOrder() can set the state and save it. And there's never a reason to call isParked() as far as I can see.

2. +++ b/src/Form/POSForm.php
   @@ -551,4 +597,44 @@ class POSForm extends ContentEntityForm {
   +    $currentOrder = $this->currentOrder->get();
   +    $currentOrder->set('state', 'parked');
   +    $currentOrder->save();
   

   Should this park an empty order?

Re #47:

When an order is parked we need to create a new order.

I checked in Drupal 7. There is no new order created after parking an order. The state is changed. The same is happening in Drupal 8.

I'm not sure that we need this if

I am sure I am missing something obvious here. The Park Order button is not supposed to appear if the order is already parked.

It's super weird to list parked orders at the top of the order screen. How did it work before?

Shawn decided to put this at the top. See #18

I think parking an order should create a new current order

I checked the Drupal 7 code. I saw that there the Commerce Order's status is just changed. I do not know whether it is supposed to do it differently in Drupal 8.

Needs '#limit_validation_errors' => [] or we need validation since parking an empty order makes very little sense.

Yeah. Makes sense.

Re #49:

I don't think this is necessary. PosForm::parkOrder() can set the state and save it.

I was trying to expose these as APIs. So that in future if there is a need to check whether CurrentOrder is parked or not, or needs to be parked. Then we can do that from the service itself, rather than calling it from a function.

Should this park an empty order?

Yes. We should put a check here.

@subhojit777 in Drupal 7 when you a park an order the current order is cleared and you have a fresh order - otherwise what is the point in parking? I've tested this on https://posdemo.acromedia.com and that is exactly what occurs. Once you have this behaviour then the if's are unnecessary. In order to create a blank order you need to create a new order.

okay made some changes. Still #47.3 and #49 needs to be addressed.

+++ b/src/Form/POSForm.php
@@ -581,7 +622,47 @@ class POSForm extends ContentEntityForm {
+    $this->clearOrder();

I still would want to move this inside CurrentOrder::park(), so that the logic is in one place. And any other controller can easily park a current order. Please advice.

1. +++ b/src/CurrentOrder.php
   @@ -49,4 +49,36 @@ class CurrentOrder {
   +  /**
   +   * Checks if the current order is parked.
   +   *
   +   * @return bool
   +   *   TRUE if the current order is parked, otherwise FALSE.
   +   */
   +  public function isParked() {
   +    /** @var \Drupal\commerce_order\Entity\OrderInterface $current_order */
   +    $current_order = $this->get();
   +
   +    if ($current_order) {
   +      return $current_order->getState()->getValue()['value'] === 'parked';
   +    }
   +
   +    return FALSE;
   +  }
   

   This is now not used by real code.

2. +++ b/src/CurrentOrder.php
   @@ -49,4 +49,36 @@ class CurrentOrder {
   +  /**
   +   * Parks the current order.
   +   *
   +   * @throws \Drupal\Core\Entity\EntityStorageException
   +   */
   +  public function park() {
   +    /** @var \Drupal\commerce_order\Entity\OrderInterface $current_order */
   +    $current_order = $this->get();
   +
   +    if ($current_order) {
   +      $current_order->set('state', 'parked')
   +        ->save();
   +    }
   +  }
   

   I'm not sure that the CurrentOrder is great class to have. Ideally we'd not have it at all. We'd just use the entity that's already attached to the POSForm. It's state and state introduces a whole interesting class of bugs and we already have form state and we have to keep this in-sync - which introduces another type of bug. Therefore I think we should just ditch this method and do it in the form - this also feels fine because (a) the form is where you interact with parked order (b) the order object already has methods to handle it's state.

Removed unnecessary code.

I noticed that retrieval of a parked order is only possible for an empty current order. This is the behavior of Drupal 7. This is not present in the patch.

+++ b/tests/src/FunctionalJavascript/PosFormTest.php
@@ -309,4 +309,67 @@ class PosFormTest extends JavascriptTestBase {
+    $this->getSession()->getPage()->findButton('Park Order')->click();

This works. But still, as a foolproof test, it would have been better if we checked this from the backend as well.

I have checked Commerce core tests, and they mostly rely on data from the db.

Some tidy-ups. I think we need to remove the current order service. We shouldn't need it and using it makes work that we do on this module harder to bring back into the commerce project. All tests pass apart from Drupal\Tests\commerce_pos_receipt\FunctionalJavascript\PosReceiptTest. I can't work out when this patch is causing that to fail (yet).

lolz. The reason Drupal\Tests\commerce_pos_receipt\FunctionalJavascript\PosReceiptTest was failing locally for me was because I had the jQuery.Print library lying around.

Here's just a little bit more tidy up.

the first thing is some debug I left in accidentally. I'll remove it with a commit. I just is more useful than a screenshot to see the current page if there are fails - you get the current html.

I converted the css selects to xpath because I found it easier to target the specific row - for some reason #edit-result > table > tbody > tr:nth-child(1) > td:nth-child(7) > a didn't work.