Fix order number generation

[winzou]

And add a scenario to cover it.
Haven't tested the scenario because I have issue running behat here:

$ bin/behat
  [Behat\Testwork\ServiceContainer\Exception\ExtensionInitializationException
  `Behat\MinkExtension\Extension` extension file or class could not be located

I have updated vendors, do you guys have the same error?

[pjedrzejewski]

You need to update local config after V3 update.

[winzou]

Ahah, yes you're right...

[winzou]

OK tests are passing locally. Should be merged asap.
Travis isn't triggered, I got only the stage1 instance instead? edit: OK it was just a bit slow to start.

[umpirsky]

Tests pass because order is flushed more then once. Try to load fixtures and you will see that order number is null.

[winzou]

@umpirsky I don't have the same results at all. I tried with my old data set and with a fresh fixtures load: it's always the same. I see only 1 flush and order number is well generated when using onFlush.

[umpirsky]

@winzou So, after $ ./app/console do:fi:lo --no-interaction you don't have null in:

?

[winzou]

Aah, I didn't understand you were talking about the fixtures data itself.

This is because fixtures are out-dated: it uses $dispatcher->dispatch(SyliusOrderEvents::PRE_CREATE, new GenericEvent($order)); whereas nobody is listening to this event anymore. It should be replaced by the state machine transition "create". Nothing to do with the number generation. Fixed by commit fe0e883 I've just done in this PR.

[umpirsky]

@winzou OK, now move flush out of for loop and try to load fixtures.

--- a/src/Sylius/Bundle/FixturesBundle/DataFixtures/ORM/LoadOrdersData.php
+++ b/src/Sylius/Bundle/FixturesBundle/DataFixtures/ORM/LoadOrdersData.php
@@ -67,8 +68,9 @@ class LoadOrdersData extends DataFixture
             $this->setReference('Sylius.Order-'.$i, $order);

             $manager->persist($order);
-            $manager->flush();
         }
+
+        $manager->flush();
     }

[winzou]

It seems that the NumberListener is not supporting when you flush several enabled entities of the same sequenceType at the same time :)

[umpirsky]

Good, and now change:

--- a/src/Sylius/Bundle/FixturesBundle/DataFixtures/ORM/LoadOrdersData.php
+++ b/src/Sylius/Bundle/FixturesBundle/DataFixtures/ORM/LoadOrdersData.php
@@ -33,7 +34,7 @@ class LoadOrdersData extends DataFixture
         $orderRepository = $this->getOrderRepository();
         $orderItemRepository = $this->getOrderItemRepository();

-        for ($i = 1; $i <= 50; $i++) {
+        for ($i = 1; $i <= 1; $i++) {

and try to load fixtures.

[stloyd]

Guys, maybe stop depending on flush, and try to depend on persist? i.e. postPersist (or more correct probably prePersist) event.

[winzou]

@umpirsky ahah interesting! So I have:

• 2 flushs with the fixtures and number is null
• 1 flush with the scenario (manual order creation) and number is generated

I would say that we don't care about fixtures, the normal way is the manual checkout which is good with this PR. If there is a problem in the fixtures, we should fix them and not the whole way numbers are generated.

@stloyd we use flush because we need transaction here (if order creation fails, no number should be taken).

[umpirsky]

@winzou The problem is that number is generated on flush, but it is not flushed to database until next flush. Order is flushed several times during checkout and that's why your scenario is green, just a lucky coincidence.

I fixed this with preFlush. Please put back preFlush now and you will see that fixtures will work.

[winzou]

@umpirsky fixed everything :)

[umpirsky]

OK, thanks @winzou.

I can't wait to see it merged and test it for Taxon number in my project.

[umpirsky]

Plus, this should work faster.

[pjedrzejewski]

Looks good! http://pr-1560.sylius.sylius.stage1.io/administration/dashboard

[pjedrzejewski]

Thank you guys! I like how Sequence works.

[winzou]

Thanks @umpirsky for having found this tricky and unpredictable issue ;)

[umpirsky]

Tricky in deed!

[umpirsky]

@pjedrzejewski When will this be available in https://github.com/Sylius/SyliusSequenceBundle?

[pjedrzejewski]

Script should run soonish.

[winzou]

@umpirsky what about your taxon numbers?

[umpirsky]

@winzou I must avoid using NumberListener because number is incremented by number of taxons because onFlufh is triggered as many times as count of taxons in database. Probably because of its hierarchical structure.

[winzou]

Uhm I don't get your point. You can call the listener as many time as you want on the same entity, the first pass will fill in $entity->getNumber() so that the following passes won't do anything: https://github.com/Sylius/Sylius/blob/master/src/Sylius/Component/Sequence/Number/AbstractGenerator.php#L32

[umpirsky]

@winzou LooooooooooooooooooooooL:

public function getNumber()
{
    $this->number;
}