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

Link order generation to state machine #283

Open
wants to merge 7 commits into
base: 1.0
Choose a base branch
from
Open

Conversation

ehibes
Copy link

@ehibes ehibes commented Jun 15, 2022

Moving order generation to state machine transition from doctrine persist event. #281
Solve #174 #235 adding some state configuration :

winzou_state_machine:
    sylius_payment:
        callbacks:
            after:
                sylius_invoicing_plugin_order_placed_producer:
                    on: ['complete']
                    do: ['@sylius_invoicing_plugin.event_producer.order_placed', '__invoke']
                    args: ['object.getOrder()']
                sylius_invoicing_plugin_payment_complete_producer:
                    disabled: true
    sylius_order:
        callbacks:
            after:
                sylius_invoicing_plugin_order_placed_producer:
                    disabled: true

I had to use the identifier instead of the number, because the order number is not yet persisted to the entity at the time of the transition.

@ehibes ehibes requested a review from a team as a code owner June 15, 2022 06:46
Copy link
Contributor

@Prometee Prometee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @ehibes !

Why do you change all the orderNumber references to id ?

Changing the moment where the invoice item is created could help.

Comment on lines 28 to 34
sylius_order:
callbacks:
after:
sylius_invoicing_plugin_order_placed_producer:
on: ['create']
do: ['@sylius_invoicing_plugin.event_producer.order_placed', '__invoke']
args: ['object']
Copy link
Contributor

@Prometee Prometee Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading what the OrderPlacedProducer is doing :

  • onPostPersist : when order.checkout_state === 'completed' then dispatch creation of sylius_invoicing_plugin_invoice new item
  • onPostUpdate : when the new value of order.checkout_state === 'completed' then dispatch creation of sylius_invoicing_plugin_invoice new item

Don't you think the new callback should be set on the state machine named : sylius_order_checkout on ['complete'] ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try this, and check if order_number is available at this time.
All behat tests passed for me... not for you ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order number is assigned here : https://github.com/Sylius/Sylius/blob/master/src/Sylius/Bundle/CoreBundle/Resources/config/app/state_machine/sylius_order.yml#L26
If your callback don't have a priority it should be executed after the order number assigner.

@ehibes
Copy link
Author

ehibes commented Jun 30, 2022

Hello @ehibes !

Why do you change all the orderNumber references to id ?

Changing the moment where the invoice item is created could help.

I did it because orderNumber was not yet generated.

{
/** @var OrderInterface $order */
$order = $this->orderRepository->findOneByNumber($orderNumber);
$order = $this->orderRepository->find($orderId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$order = $this->orderRepository->find($orderId);
/** @var OrderInterface|null $order */
$order = $this->orderRepository->findOneBy(['number' => $orderNumber]);
Assert::notNull($order, sprintf('Order with number "%s" does not exist.', $orderNumber));

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this before but it didn't work, I'll try again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Behat test is maybe missing something, in my current projet I have to prepare my order doing this :

$stateMachine = $this->stateMachineFactory->get($order, OrderTransitions::GRAPH);

$stateMachine->apply(OrderTransitions::TRANSITION_CREATE);

save it to the database and complete the order (OrderCheckoutTransitions::TRANSITION_COMPLETE).

@ehibes
Copy link
Author

ehibes commented Jun 30, 2022

I made changes according to you reviews. PhpSpec & behat tests passed. I don't know how to add your behat scenario...

Comment on lines 37 to 39
$firstOrder->getNumber()->willReturn('000001');
$secondOrder->getNumber()->willReturn('000002');
$thirdOrder->getNumber()->willReturn('000003');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To ease the reviewing of your PR by Sylius core team members, can you revert it by adding the missing zero ?

Comment on lines 47 to 49
$invoiceCreator->__invoke('000001', $firstInvoiceDateTime)->shouldBeCalled();
$invoiceCreator->__invoke('000002', $secondInvoiceDateTime)->shouldBeCalled();
$invoiceCreator->__invoke('000003', $thirdInvoiceDateTime)->shouldBeCalled();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Comment on lines 31 to 33
$invoiceCreator->__invoke('000001', $issuedAt)->shouldBeCalled();

$this(new OrderPlaced('0000001', $issuedAt));
$this(new OrderPlaced('000001', $issuedAt));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here 😉

@@ -38,7 +39,8 @@ public function __construct(
public function __invoke(string $orderNumber, \DateTimeInterface $dateTime): void
{
/** @var OrderInterface $order */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This phpdoc is missing the possibility of null

@Prometee
Copy link
Contributor

Prometee commented Jun 30, 2022

I made changes according to you reviews. PhpSpec & behat tests passed. I don't know how to add your behat scenario...

Perfect, thank you for your work, I will ping the devs about it to start approving it 😉
Don't worry about the BeHat test, I will setup one elaborate one in an other PR.

@ehibes
Copy link
Author

ehibes commented Jun 30, 2022

Perfect, thank you for your work, I will ping the devs about it to start approving it 😉 Don't worry about the BeHat test, I will setup one elaborate one in an other PR.

Thank you for your reactivity @Prometee.
I should write a Behat scenario too, for my example of the first message.

@Prometee
Copy link
Contributor

Yes it will avoid getting the issue again indeed 👌

.php-version Outdated
@@ -0,0 +1 @@
8.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should be removed

/** @var OrderInterface $order */
$order = $this->orderRepository->findOneByNumber($orderNumber);
/** @var OrderInterface|null $order */
$order = $this->orderRepository->findOneBy(['number' => $orderNumber]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change that in this PR?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because findOneByNumber refers to OrderRepository method, which verify order is not on cart state. So we have a null value using repository method.

    public function findOneByNumber(string $number): ?OrderInterface
    {
        return $this->createQueryBuilder('o')
            ->andWhere('o.state != :state')
            ->andWhere('o.number = :number')
            ->setParameter('state', OrderInterface::STATE_CART)
            ->setParameter('number', $number)
            ->getQuery()
            ->getOneOrNullResult()
        ;
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what is the reason the order is in cart state? Because in the moment of creating an invoice the checkout should be completed and the order shouldn't be in the cart state anymore. I guess, but I'm not sure, there is a difference with the moment of dispatching the event between state machine callback and the doctrine events.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this moment the order state is new instead of cart in the database.

Comment on lines +28 to +34
sylius_order_checkout:
callbacks:
after:
sylius_invoicing_plugin_order_placed_producer:
on: ['complete']
do: ['@sylius_invoicing_plugin.event_producer.order_placed', '__invoke']
args: ['object']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it wouldn't be better to put this logic on the callback after create transition on sylius_order state machine 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @Prometee

Copy link
Contributor

@Prometee Prometee Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GSadee yes it will make the same thing, but one issue will happen here if the Order has not been saved yet.
Here is an example :

$order = $this->orderFactory->createNew();
/* add product, address and select payment, shipping etc */
$this->stateMachineFactory
    ->get($order, OrderCheckoutTransitions::GRAPH)
    ->apply(OrderCheckoutTransitions::TRANSITION_COMPLETE)
;

The result will be an error on the InvoiceCreator because the doctrine query will fail finding the messaged order number.

To solve this, the InvoiceCreator could be reshaped to be a state machine callback, WDYT ?

I know it's a big change and it will make useless triggering the OrderPlaced event and it will require moving the PDF creation to the OrderPaymentPaid event.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I correctly understand you, is there a problem with moving the callback to sylius_order state machine, in this implementation or in both cases? What is more, as you probably suggested changing the moment of generating an invoice PDF, IMO it is not possible, it should be generated during invoice generation after the order is placed, not payment paid.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During the @sylius_invoicing_plugin.event_producer.order_placed'->__invoke() the invoice creation is dispatched and cause the error I mentioned. To solve this error we can :

  1. remove the dispatch and simply call the service creating the Invoice with a new method signature for the class
    Sylius\InvoicingPlugin\Creator\InvoiceCreator

    From :

    public function __invoke(string $orderNumber, \DateTimeInterface $dateTime): void

    To:

    public function __invoke(OrderInterface $order, \DateTimeInterface $dateTime): void
    // Maybe the argument `$dateTime` can be removed and we can use `$order->getCheckoutCompletedAt()` instead
  2. Move the PDF creation to another moment like when the PDF is sent payment_state === 'paid'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my point of view the Invoice data stored in the database are the immutable data, the PDF is a view of this data.

@GSadee GSadee added the DX Issues and PRs aimed at improving Developer eXperience. label Jul 1, 2022
@David-Crty
Copy link

I love this feature.
Can I help with anything on those falling checks? Let me know if I can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX Issues and PRs aimed at improving Developer eXperience.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants