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.

Sign up for GitHub

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

Jump to bottom

Link order generation to state machine #283

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

Link order generation to state machine #283

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1b76e14
OrderPlacedProducer linked to state machine in order to Doctrine post…
ehibes Jun 9, 2022
a817d69
OrderPlaced Event based on orderId in order to orderNumber
ehibes Jun 15, 2022
380941a
Linked to state machine with orderNumber
ehibes Jun 30, 2022
7e36063
Merge branch 'state-machine'
ehibes Jun 30, 2022
3ec77e8
Spec order number fix
ehibes Jun 30, 2022
f77bc23
Php doc and orderNumber fix
ehibes Jun 30, 2022
3c3a1fd
remove php-version
ehibes Jul 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions spec/Creator/InvoiceCreatorSpec.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ function it_creates_invoice_for_order(
): void {
$invoicePdf = new InvoicePdf('invoice.pdf', 'CONTENT');

$orderRepository->findOneByNumber('0000001')->willReturn($order);
$orderRepository->findOneBy(['number' => '0000001'])->willReturn($order);

$invoiceRepository->findOneByOrder($order)->willReturn(null);

Expand Down Expand Up @@ -94,7 +94,7 @@ function it_creates_invoice_without_generating_pdf_file(
false
);

$orderRepository->findOneByNumber('0000001')->willReturn($order);
$orderRepository->findOneBy(['number' => '0000001'])->willReturn($order);

$invoiceRepository->findOneByOrder($order)->willReturn(null);

Expand All @@ -121,7 +121,7 @@ function it_removes_saved_invoice_file_if_database_update_fails(
): void {
$invoicePdf = new InvoicePdf('invoice.pdf', 'CONTENT');

$orderRepository->findOneByNumber('0000001')->willReturn($order);
$orderRepository->findOneBy(['number' => '0000001'])->willReturn($order);

$invoiceRepository->findOneByOrder($order)->willReturn(null);

Expand All @@ -144,7 +144,7 @@ function it_throws_an_exception_when_invoice_was_already_created_for_given_order
OrderInterface $order,
InvoiceInterface $invoice
): void {
$orderRepository->findOneByNumber('0000001')->willReturn($order);
$orderRepository->findOneBy(['number' => '0000001'])->willReturn($order);
$invoiceRepository->findOneByOrder($order)->willReturn($invoice);

$invoiceDateTime = new \DateTimeImmutable('2019-02-25');
Expand Down
12 changes: 6 additions & 6 deletions spec/Creator/MassInvoicesCreatorSpec.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,19 @@ function it_requests_invoices_creation_for_multiple_orders(
OrderInterface $secondOrder,
OrderInterface $thirdOrder
): void {
$firstOrder->getNumber()->willReturn('0000001');
$secondOrder->getNumber()->willReturn('0000002');
$thirdOrder->getNumber()->willReturn('0000003');
$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 ?


$firstInvoiceDateTime = new \DateTimeImmutable('2019-02-25');
$secondInvoiceDateTime = new \DateTimeImmutable('2019-02-25');
$thirdInvoiceDateTime = new \DateTimeImmutable('2019-02-25');

$dateTimeProvider->__invoke()->willReturn($firstInvoiceDateTime, $secondInvoiceDateTime, $thirdInvoiceDateTime);

$invoiceCreator->__invoke('0000001', $firstInvoiceDateTime)->shouldBeCalled();
$invoiceCreator->__invoke('0000002', $secondInvoiceDateTime)->shouldBeCalled();
$invoiceCreator->__invoke('0000003', $thirdInvoiceDateTime)->shouldBeCalled();
$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


$this->__invoke([$firstOrder->getWrappedObject(), $secondOrder->getWrappedObject(), $thirdOrder->getWrappedObject()]);
}
Expand Down
4 changes: 2 additions & 2 deletions spec/EventListener/CreateInvoiceOnOrderPlacedListenerSpec.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ function it_requests_invoice_creation(InvoiceCreatorInterface $invoiceCreator):
{
$issuedAt = new \DateTimeImmutable();

$invoiceCreator->__invoke('0000001', $issuedAt)->shouldBeCalled();
$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 😉

}
}
86 changes: 2 additions & 84 deletions spec/EventProducer/OrderPlacedProducerSpec.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,11 @@ function it_dispatches_an_order_placed_event_for_persisted_order(
$order->getNumber()->willReturn('000666');
$order->getCheckoutState()->willReturn(OrderCheckoutStates::STATE_COMPLETED);

$postPersistEvent = new LifecycleEventArgs($order->getWrappedObject(), $objectManager->getWrappedObject());
$orderPlacedEvent = new OrderPlaced('000666', $dateTime);

$eventBus->dispatch($orderPlacedEvent)->shouldBeCalled()->willReturn(new Envelope($orderPlacedEvent));

$this->postPersist($postPersistEvent);
$this->__invoke($order);
}

function it_dispatches_an_order_placed_event_for_updated_order(
Expand All @@ -74,91 +73,10 @@ function it_dispatches_an_order_placed_event_for_updated_order(

$order->getNumber()->willReturn('000666');

$postUpdateEvent = new LifecycleEventArgs($order->getWrappedObject(), $entityManager->getWrappedObject());
$orderPlacedEvent = new OrderPlaced('000666', $dateTime);

$eventBus->dispatch($orderPlacedEvent)->shouldBeCalled()->willReturn(new Envelope($orderPlacedEvent));

$this->postUpdate($postUpdateEvent);
}

function it_does_nothing_after_persisting_if_event_entity_is_not_order(
MessageBusInterface $eventBus,
LifecycleEventArgs $event
): void {
$event->getEntity()->willReturn('notAnOrder');

$eventBus->dispatch(Argument::any())->shouldNotBeCalled();

$this->postPersist($event);
}

function it_does_nothing_after_update_if_event_entity_is_not_order(
MessageBusInterface $eventBus,
LifecycleEventArgs $event
): void {
$event->getEntity()->willReturn('notAnOrder');

$eventBus->dispatch(Argument::any())->shouldNotBeCalled();

$this->postUpdate($event);
}

function it_does_nothing_after_persisting_if_order_is_not_completed(
MessageBusInterface $eventBus,
LifecycleEventArgs $event,
OrderInterface $order
): void {
$event->getEntity()->willReturn($order);

$order->getCheckoutState()->willReturn(OrderCheckoutStates::STATE_CART);

$eventBus->dispatch(Argument::any())->shouldNotBeCalled();

$this->postPersist($event);
}

function it_does_nothing_after_update_if_order_checkout_state_has_not_changed(
MessageBusInterface $eventBus,
LifecycleEventArgs $event,
EntityManagerInterface $entityManager,
OrderInterface $order
): void {
$event->getEntity()->willReturn($order);

$event->getEntityManager()->willReturn($entityManager);

/** @var UnitOfWork|MockInterface $unitOfWork */
$unitOfWork = Mockery::mock(UnitOfWork::class);
$unitOfWork->shouldReceive('getEntityChangeSet')->withArgs([$order->getWrappedObject()])->andReturn([]);

$entityManager->getUnitOfWork()->willReturn($unitOfWork);

$eventBus->dispatch(Argument::any())->shouldNotBeCalled();

$this->postUpdate($event);
}

function it_does_nothing_after_update_if_order_checkout_state_has_not_changed_to_completed(
MessageBusInterface $eventBus,
LifecycleEventArgs $event,
EntityManagerInterface $entityManager,
OrderInterface $order
): void {
$event->getEntity()->willReturn($order);

$event->getEntityManager()->willReturn($entityManager);

/** @var UnitOfWork|MockInterface $unitOfWork */
$unitOfWork = Mockery::mock(UnitOfWork::class);
$unitOfWork->shouldReceive('getEntityChangeSet')->withArgs([$order->getWrappedObject()])->andReturn([
'checkoutState' => [OrderCheckoutStates::STATE_CART, OrderCheckoutStates::STATE_ADDRESSED],
]);

$entityManager->getUnitOfWork()->willReturn($unitOfWork);

$eventBus->dispatch(Argument::any())->shouldNotBeCalled();

$this->postUpdate($event);
$this->__invoke($order);
}
}
10 changes: 6 additions & 4 deletions src/Creator/InvoiceCreator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,16 @@

namespace Sylius\InvoicingPlugin\Creator;

use Webmozart\Assert\Assert;
use Doctrine\ORM\ORMException;
use Sylius\Component\Core\Model\OrderInterface;
use Sylius\Component\Core\Repository\OrderRepositoryInterface;
use Sylius\InvoicingPlugin\Doctrine\ORM\InvoiceRepositoryInterface;
use Sylius\InvoicingPlugin\Entity\InvoiceInterface;
use Sylius\InvoicingPlugin\Exception\InvoiceAlreadyGenerated;
use Sylius\Component\Core\Repository\OrderRepositoryInterface;
use Sylius\InvoicingPlugin\Generator\InvoiceGeneratorInterface;
use Sylius\InvoicingPlugin\Generator\InvoicePdfFileGeneratorInterface;
use Sylius\InvoicingPlugin\Manager\InvoiceFileManagerInterface;
use Sylius\InvoicingPlugin\Doctrine\ORM\InvoiceRepositoryInterface;
use Sylius\InvoicingPlugin\Generator\InvoicePdfFileGeneratorInterface;

final class InvoiceCreator implements InvoiceCreatorInterface
{
Expand All @@ -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

$order = $this->orderRepository->findOneByNumber($orderNumber);
$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.

Assert::notNull($order, sprintf('Order with number "%s" does not exist.', $orderNumber));

/** @var InvoiceInterface|null $invoice */
$invoice = $this->invoiceRepository->findOneByOrder($order);
Expand Down
41 changes: 1 addition & 40 deletions src/EventProducer/OrderPlacedProducer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@

namespace Sylius\InvoicingPlugin\EventProducer;

use Doctrine\ORM\Event\LifecycleEventArgs;
use Sylius\Component\Core\Model\OrderInterface;
use Sylius\Component\Core\OrderCheckoutStates;
use Sylius\InvoicingPlugin\DateTimeProvider;
use Sylius\InvoicingPlugin\Event\OrderPlaced;
use Symfony\Component\Messenger\MessageBusInterface;
Expand All @@ -32,44 +30,7 @@ public function __construct(MessageBusInterface $eventBus, DateTimeProvider $dat
$this->dateTimeProvider = $dateTimeProvider;
}

public function postPersist(LifecycleEventArgs $event): void
{
$order = $event->getEntity();

if (
!$order instanceof OrderInterface ||
$order->getCheckoutState() !== OrderCheckoutStates::STATE_COMPLETED
) {
return;
}

$this->dispatchOrderPlacedEvent($order);
}

public function postUpdate(LifecycleEventArgs $event): void
{
$order = $event->getEntity();

if (!$order instanceof OrderInterface) {
return;
}

$entityManager = $event->getEntityManager();

$unitOfWork = $entityManager->getUnitOfWork();
$changeSet = $unitOfWork->getEntityChangeSet($order);

if (
!isset($changeSet['checkoutState']) ||
$changeSet['checkoutState'][1] !== OrderCheckoutStates::STATE_COMPLETED
) {
return;
}

$this->dispatchOrderPlacedEvent($order);
}

private function dispatchOrderPlacedEvent(OrderInterface $order): void
public function __invoke(OrderInterface $order): void
{
$this->eventBus->dispatch(
new OrderPlaced($order->getNumber(), $this->dateTimeProvider->__invoke())
Expand Down
7 changes: 7 additions & 0 deletions src/Resources/config/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@ winzou_state_machine:
on: ['complete']
do: ['@sylius_invoicing_plugin.event_producer.order_payment_paid', '__invoke']
args: ['object']
sylius_order_checkout:
callbacks:
after:
sylius_invoicing_plugin_order_placed_producer:
on: ['complete']
do: ['@sylius_invoicing_plugin.event_producer.order_placed', '__invoke']
args: ['object']
Comment on lines +28 to +34
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 •
edited
Loading

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.


sylius_grid:
templates:
Expand Down
2 changes: 0 additions & 2 deletions src/Resources/config/services/events.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
<service id="sylius_invoicing_plugin.event_producer.order_placed" class="Sylius\InvoicingPlugin\EventProducer\OrderPlacedProducer">
<argument type="service" id="sylius.event_bus" />
<argument type="service" id="sylius_invoicing_plugin.date_time_provider" />
<tag name="doctrine.event_listener" event="postPersist" />
<tag name="doctrine.event_listener" event="postUpdate" />
</service>

<service id="sylius_invoicing_plugin.listener.order_payment_paid" class="Sylius\InvoicingPlugin\EventListener\OrderPaymentPaidListener">
Expand Down