Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reimplement User Cart Revise Cancellation #2166

Open
solverat opened this issue Jan 3, 2023 · 12 comments
Open

Reimplement User Cart Revise Cancellation #2166

solverat opened this issue Jan 3, 2023 · 12 comments

Comments

@solverat
Copy link
Contributor

solverat commented Jan 3, 2023

Q A
Bug report? yes
Feature request? no
BC Break report? /no
RFC? no

Now

Original

$this->get('coreshop.state_machine_applier')->apply($order, OrderTransitions::IDENTIFIER, OrderTransitions::TRANSITION_CANCEL);
if ($order instanceof Concrete) {
$this->get(HistoryLogger::class)->log(
$order,
'User Cart Revise Cancellation'
);
}
$cart = $this->get('coreshop.repository.cart')->findCartByOrder($order);
if ($cart instanceof CartInterface) {
$cart->setOrder(null);
$this->get('coreshop.cart.manager')->persistCart($cart);
$session = $request->getSession();
$session->set(
sprintf('%s.%s', $this->getParameter('coreshop.session.cart'), $cart->getStore()->getId()),
$cart->getId()
);
return $this->redirectToRoute('coreshop_cart_summary');
}
return $this->redirectToRoute('coreshop_index');

@solverat
Copy link
Contributor Author

@dpfaffenbauer Is there a reason why this has been removed? Otherwise, we could provide an PR which implements the original behavior.

@dpfaffenbauer
Copy link
Member

@solverat It never was implemented with the new Order Flow. How would you implement it?

@solverat
Copy link
Contributor Author

@dpfaffenbauer: I just tested it via this roughly implemented snippet:

if ($cancelButton instanceof ClickableInterface && $form->isSubmitted() && $cancelButton->isClicked()) {

    if ($order instanceof Concrete) {
        $this->get(HistoryLogger::class)->log(
            $order,
            'User Cart Revise Cancellation'
        );
    }

    $order->setSaleState(OrderSaleStates::STATE_CART);
    $this->get('coreshop.cart.manager')->persistCart($order);

    $request->getSession()->set(
        sprintf('%s.%s', 'coreshop.cart', $order->getStore()->getId()),
        $order->getId()
    );

    return $this->redirectToRoute('coreshop_cart_summary');
}

Which just will bring back the user's cart.

Of course, this snippet should:

  • use injected services
  • use the sessionKeyName from the compiled (storageList) parameters

BTW: While testing this, I found some orphans:

  • Parametercoreshop.session.cart: Unused
  • CoreShop\Bundle\OrderBundle\EventListener\SessionCartSubscriber: Unused

@dpfaffenbauer
Copy link
Member

Let's think about the consequences of reseting the state instead of creating a new cart:

  • What about missing Order Numbers? Eg. O1 get's reset, then O2 get's created, O1 is never visible as Order again
  • What about some other culprits that might be connected to the Order Workflow that do stuff on the transition?

Not sure if it woudl be better to create a new cart and populate it with the same items. Sort of similar like it was with CS 2

@solverat
Copy link
Contributor Author

In CS2, the original cart object has been re-assigned, so it was a bit easier. However, creating a new cart object leads to the same problem you've mentioned (second item of your list). What if the cart has been populated with other data (additional fields during the checkout, for example)?

I still think, the re-assignment is the better way to go. I admit, the Order-Number could be an issue, so resetting that field would solve that. Everything else can be ignored: The order is still invalid at this point. Also: The workflow will trigger again.

@dpfaffenbauer
Copy link
Member

the Order-Number could be an issue, so resetting that field would solve that

No, that is not what I meant. The O1 Number will never be reassigned and there is no order then. Once a Order has a Order Number it should stay an Order.

My suggestion: Copy the Order and make it a cart. Then additional Data is also copied.

@solverat
Copy link
Contributor Author

Ah, I forgot about the sequence generator. Well, yes, cloning the object is the only way to go.

@hSpille
Copy link

hSpille commented Aug 21, 2023

Same Problem here. Is there a solution for this yet?

@dpfaffenbauer
Copy link
Member

@hSpille has not been implemented on our end

@m0nken
Copy link
Contributor

m0nken commented Aug 22, 2023

Maybe the order could just have its state switched back to OrderSaleStates::STATE_CART and keep its assigned order number. Then of course the OrderCommitter would have to be adapted in function commitOrder(OrderInterface $order) so that it would only generate a new order number if the order does not already have an order number. Or am I missing something here?

@dpfaffenbauer
Copy link
Member

@m0nken I don't really like that, since the Order should get cancelled so the number isn't missing. What if the User never checks out? The Order O0010 would then be missing and never be found.

It honestly shouldn't be too difficult, just take the OrderItems, Copy them, and attach them to a new Cart.

@m0nken
Copy link
Contributor

m0nken commented Aug 22, 2023

@dpfaffenbauer Good point! Just wanted to check if that could have been an option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants