Skip to content

Link order generation to state machine #283

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

Open
ehibes 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 2 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
16 changes: 8 additions & 8 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->find(1)->willReturn($order);

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

Expand All @@ -73,7 +73,7 @@ function it_creates_invoice_for_order(

$invoiceRepository->add($invoice)->shouldBeCalled();

$this->__invoke('0000001', $invoiceDateTime);
$this->__invoke(1, $invoiceDateTime);
}

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

$orderRepository->findOneByNumber('0000001')->willReturn($order);
$orderRepository->find(1)->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->find(1)->willReturn($order);

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

Expand All @@ -134,7 +134,7 @@ function it_removes_saved_invoice_file_if_database_update_fails(
$invoiceRepository->add($invoice)->willThrow(ORMException::class);
$invoiceFileManager->remove($invoicePdf)->shouldBeCalled();

$this->__invoke('0000001', $invoiceDateTime);
$this->__invoke(1, $invoiceDateTime);
}

function it_throws_an_exception_when_invoice_was_already_created_for_given_order(
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->find(1)->willReturn($order);
$invoiceRepository->findOneByOrder($order)->willReturn($invoice);

$invoiceDateTime = new \DateTimeImmutable('2019-02-25');
Expand All @@ -153,8 +153,8 @@ function it_throws_an_exception_when_invoice_was_already_created_for_given_order
$invoiceRepository->add(Argument::any())->shouldNotBeCalled();

$this
->shouldThrow(InvoiceAlreadyGenerated::withOrderNumber('0000001'))
->during('__invoke', ['0000001', $invoiceDateTime])
->shouldThrow(InvoiceAlreadyGenerated::withOrderId(1))
->during('__invoke', [1, $invoiceDateTime])
;
}
}
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->getId()->willReturn(1);
$secondOrder->getId()->willReturn(2);
$thirdOrder->getId()->willReturn(3);

$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(1, $firstInvoiceDateTime)->shouldBeCalled();
$invoiceCreator->__invoke(2, $secondInvoiceDateTime)->shouldBeCalled();
$invoiceCreator->__invoke(3, $thirdInvoiceDateTime)->shouldBeCalled();

$this->__invoke([$firstOrder->getWrappedObject(), $secondOrder->getWrappedObject(), $thirdOrder->getWrappedObject()]);
}
Expand Down
4 changes: 2 additions & 2 deletions spec/Event/OrderPlacedSpec.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ function it_represents_an_immutable_fact_that_an_order_has_been_placed(): void
{
$date = new \DateTimeImmutable();

$this->beConstructedWith('000001', $date);
$this->beConstructedWith(1, $date);

$this->orderNumber()->shouldReturn('000001');
$this->orderId()->shouldReturn(1);
$this->date()->shouldBeLike($date);
}
}
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(1, $issuedAt)->shouldBeCalled();

$this(new OrderPlaced('0000001', $issuedAt));
$this(new OrderPlaced(1, $issuedAt));
}
}
76 changes: 6 additions & 70 deletions spec/EventProducer/OrderPlacedProducerSpec.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,14 @@ function it_dispatches_an_order_placed_event_for_persisted_order(
$dateTime = new \DateTime('2018-12-14');
$dateTimeProvider->__invoke()->willReturn($dateTime);

$order->getNumber()->willReturn('000666');
$order->getId()->willReturn(666);
$order->getCheckoutState()->willReturn(OrderCheckoutStates::STATE_COMPLETED);

$postPersistEvent = new LifecycleEventArgs($order->getWrappedObject(), $objectManager->getWrappedObject());
$orderPlacedEvent = new OrderPlaced('000666', $dateTime);
$orderPlacedEvent = new OrderPlaced(666, $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 @@ -72,14 +71,13 @@ function it_dispatches_an_order_placed_event_for_updated_order(

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

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

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

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

$this->postUpdate($postUpdateEvent);
$this->__invoke($order);
}

function it_does_nothing_after_persisting_if_event_entity_is_not_order(
Expand All @@ -89,8 +87,6 @@ function it_does_nothing_after_persisting_if_event_entity_is_not_order(
$event->getEntity()->willReturn('notAnOrder');

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

$this->postPersist($event);
}

function it_does_nothing_after_update_if_event_entity_is_not_order(
Expand All @@ -100,65 +96,5 @@ function it_does_nothing_after_update_if_event_entity_is_not_order(
$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);
}
}
6 changes: 3 additions & 3 deletions src/Creator/InvoiceCreator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,16 +35,16 @@ public function __construct(
) {
}

public function __invoke(string $orderNumber, \DateTimeInterface $dateTime): void
public function __invoke(int $orderId, \DateTimeInterface $dateTime): void
{
/** @var OrderInterface $order */
Copy link
Copy Markdown
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.

This phpdoc is missing the possibility of null

$order = $this->orderRepository->findOneByNumber($orderNumber);
$order = $this->orderRepository->find($orderId);
Copy link
Copy Markdown
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.

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
Copy Markdown
Author

@ehibes ehibes Jun 30, 2022

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
Copy Markdown
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.

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).


/** @var InvoiceInterface|null $invoice */
$invoice = $this->invoiceRepository->findOneByOrder($order);

if (null !== $invoice) {
throw InvoiceAlreadyGenerated::withOrderNumber($orderNumber);
throw InvoiceAlreadyGenerated::withOrderId($orderId);
}

$invoice = $this->invoiceGenerator->generateForOrder($order, $dateTime);
Expand Down
2 changes: 1 addition & 1 deletion src/Creator/InvoiceCreatorInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@

interface InvoiceCreatorInterface
{
public function __invoke(string $orderNumber, \DateTimeInterface $dateTime): void;
public function __invoke(int $orderId, \DateTimeInterface $dateTime): void;
}
2 changes: 1 addition & 1 deletion src/Creator/MassInvoicesCreator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public function __invoke(array $orders): void
/** @var OrderInterface $order */
foreach ($orders as $order) {
try {
$this->invoiceCreator->__invoke($order->getNumber(), $this->dateTimeProvider->__invoke());
$this->invoiceCreator->__invoke($order->getId(), $this->dateTimeProvider->__invoke());
} catch (InvoiceAlreadyGenerated $exception) {
continue;
}
Expand Down
10 changes: 5 additions & 5 deletions src/Event/OrderPlaced.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,19 @@

final class OrderPlaced
{
private string $orderNumber;
private int $orderId;

private \DateTimeInterface $date;

public function __construct(string $orderNumber, \DateTimeInterface $date)
public function __construct(int $orderId, \DateTimeInterface $date)
{
$this->orderNumber = $orderNumber;
$this->orderId = $orderId;
$this->date = clone $date;
}

public function orderNumber(): string
public function orderId(): int
{
return $this->orderNumber;
return $this->orderId;
}

public function date(): \DateTimeInterface
Expand Down
2 changes: 1 addition & 1 deletion src/EventListener/CreateInvoiceOnOrderPlacedListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public function __construct(InvoiceCreatorInterface $invoiceCreator)
public function __invoke(OrderPlaced $event): void
{
try {
$this->invoiceCreator->__invoke($event->orderNumber(), $event->date());
$this->invoiceCreator->__invoke($event->orderId(), $event->date());
} catch (InvoiceAlreadyGenerated $exception) {
return;
}
Expand Down
43 changes: 2 additions & 41 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,47 +30,10 @@ 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())
new OrderPlaced($order->getId(), $this->dateTimeProvider->__invoke())
);
}
}
4 changes: 2 additions & 2 deletions src/Exception/InvoiceAlreadyGenerated.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@

final class InvoiceAlreadyGenerated extends \DomainException
{
public static function withOrderNumber(string $orderNumber): self
public static function withOrderId(int $orderId): self
{
return new self(sprintf('An invoice for order with number %s was already generated', $orderNumber));
return new self(sprintf('An invoice for order with id %s was already generated', $orderId));
}
}
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:
callbacks:
after:
sylius_invoicing_plugin_order_placed_producer:
on: ['create']
do: ['@sylius_invoicing_plugin.event_producer.order_placed', '__invoke']
args: ['object']
Copy link
Copy Markdown
Contributor

@Prometee Prometee Jun 30, 2022 •
edited
Loading

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
Copy Markdown
Author

@ehibes ehibes Jun 30, 2022

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
Copy Markdown
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.

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.

Comment on lines +28 to +34
Copy link
Copy Markdown
Member

@GSadee GSadee Jul 1, 2022

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
Copy Markdown
Author

@ehibes ehibes Jul 1, 2022

Choose a reason for hiding this comment

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

ping @Prometee

Copy link
Copy Markdown
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
Copy Markdown
Member

@GSadee GSadee Jul 27, 2022

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
Copy Markdown
Contributor

@Prometee Prometee Jul 27, 2022

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
Copy Markdown
Contributor

@Prometee Prometee Jul 27, 2022

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