Fixed unique order number error #904

umpirsky · 2014-01-24T13:09:12Z

Order with latest completedAt value, is not always order with greatest order number. That is why you can get:

An exception occurred while executing 'UPDATE sylius_order SET number = ?, completed_at = ?, updated_at = ? WHERE id = ?' with params ["000000048", "2014-01-24 13:06:05", "2014-01-24 13:06:05", 52]:

SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '000000048' for key 'UNIQ_6196A1F996901F54'

This should be fixed now.

pjedrzejewski · 2014-01-24T13:38:59Z

MAX will work on varchar field as long as we keep them numbers. In case someone wants to use codes ... he will need to replace the order number generator right? (my main point here is that "number" is varchar, because sometimes you want stuff like "ABCFOO-1424" or whatever)

umpirsky · 2014-01-24T13:48:01Z

What about getting order with last id?
On 24 Jan 2014 14:39, "Paweł Jędrzejewski" notifications@github.com wrote:

MAX will work on varchar field as long as we keep them numbers. In case
someone wants to use codes ... he will need to replace the order number
generator right? (my main point here is that "number" is varchar, because
sometimes you want stuff like "ABCFOO-1424" or whatever)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/904#issuecomment-33222964
.

umpirsky · 2014-01-24T14:36:39Z

Ping @pjedrzejewski

pjedrzejewski · 2014-01-24T15:48:37Z

Yeah, but ... we have to take the order with a) last ID b) only if it has been completed (completedAt property is not empty). This way it should work properly and avoid this error.

umpirsky · 2014-01-24T15:56:10Z

@pjedrzejewski What if there is order number 000000052 and for that order completedAt is null and that is the order with latest ID. Then last order number returned from getLastOrderNumber() will be 000000051 and we will get error again?

umpirsky · 2014-01-24T15:58:30Z

@pjedrzejewski Fixed according to your instructions anyway, but I would remove ->andWhere('o.completedAt IS NOT NULL').

pjedrzejewski · 2014-01-24T16:02:27Z

This will show you the carts (not completed orders). We should base the id on the number, not the sql id. What do you think?

umpirsky · 2014-01-24T16:16:55Z

I dont understand your last comment. Is my PR ok now? I'm fine if we merge
it like this :)
On 24 Jan 2014 17:02, "Paweł Jędrzejewski" notifications@github.com wrote:

This will show you the carts (not completed orders). We should base the id
on the number, not the sql id. What do you think?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/904#issuecomment-33234973
.

kayue · 2014-01-24T17:40:10Z

src/Sylius/Bundle/OrderBundle/Doctrine/ORM/OrderRepository.php

+        return $queryBuilder
+            ->select('o.number')
+            ->andWhere('o.completedAt IS NOT NULL')
+            ->orderBy('o.id', 'desc')


@umpirsky I think @pjedrzejewski is referring to this line. Should we order by o.number instead?

@kayue That was my original idea, but then @pjedrzejewski wanted to cover order numbers like ABCFOO-1424. See #904 (comment).

The ID definitely does not work for this (that is what it originally was). The issue is that the order number sequence is related to the completedAt of the order, not the original insertion time. See #631

Richtermeister · 2014-01-24T17:49:57Z

👎 on ordering by ID. I had some more thoughts over here: #880 (comment)

Basically, I think the only really reliable way is to use a separate table to track an autoincrement for completed orders only.

umpirsky · 2014-01-24T18:45:49Z

@Richtermeister In that case I vote for this logic, or previous + adding unique check, which means if order number already exists, increment it and try again until we find non existing one.

Thoughts? @pjedrzejewski @kayue

umpirsky · 2014-01-27T09:11:43Z

Ping @pjedrzejewski

kayue · 2014-01-27T09:20:51Z

@umpirsky I vote for sorting by order.number, and MySQL is able to ORDER BY or MAX() string like "ABC1234", "ABC1235", I can't see why this method doesn't work.

And people can always introduce new generator if they want something fancy.

umpirsky · 2014-01-27T09:24:32Z

@kayue I agree. I just think @pjedrzejewski want to make number generator as flexible as possible and non sortable order numbers.

@pjedrzejewski Please help us make decision and I will fix it since we need this fix.

winzou · 2014-01-27T10:02:10Z

I'm in favor of @Richtermeister's solution, aka not sorting a field without knowing its content.

pjedrzejewski · 2014-01-27T10:07:06Z

I'll have a look at existing solutions... it's a bit tricky for me as well.

umpirsky · 2014-01-28T09:29:31Z

@pjedrzejewski Any decision? This is a blocker for us.

pjedrzejewski · 2014-01-28T09:39:58Z

Ok, after some research I think there is no other clean way than going with a separate table like @Richtermeister said. So if we want the default order number generator work with sequential numbers, then we need to go this way.

BUT, I'd like to have a separate customizable generator, which would handle something like this.

umpirsky · 2014-01-28T09:43:18Z

@pjedrzejewski OK. Please suggest table name. Customizable number generator can be handled in separate PR. Agree?

pjedrzejewski · 2014-01-28T09:45:58Z

@umpirsky Yes! sylius_order_increment_id or sylius_order_id? Maybe someone will have a better idea!

stloyd · 2014-01-28T09:47:33Z

I would vote for: sylius_order_identifier or sylius_order_number, as id is confusing, because this could not be real identifier but some string like SKU =)

winzou · 2014-01-28T09:47:40Z

sylius_number:

Number because we are not talking about id here
no mention to order to be able to handle other entities later

pjedrzejewski · 2014-01-28T09:49:43Z

Okay, @winzou 's sylius_number is simplest and clear. Let's go with this one. @umpirsky Will you work on this?

umpirsky · 2014-01-28T09:52:29Z

@pjedrzejewski Yes, thanks! So, I just need to insert into sylius_number on SyliusCheckoutEvents::FINALIZE_PRE_COMPLETE event?

winzou · 2014-01-28T09:55:31Z

I would say ORDER_PRE_CREATE.
Unless we want to generate a number only for paid order, in that case it will be ORDER_PRE_PAY (event not merged yet).

The paid/not paid order with or without number is a real problem. If no number we can't use it as idnetifier to pass to the payment gateway. If number, then some numbers will be taken by orders and then deleted if no payment received.

umpirsky · 2014-01-28T09:58:11Z

@winzou Right, ORDER_PRE_CREATE.

umpirsky · 2014-01-28T11:29:21Z

If it is in order bundle Sylius\Bundle\OrderBundle\Model\Number then it will not be reusable for other entities later.
If it is in core bundle Sylius\Bundle\CoreBundle\Model\Number then we cant rely on it in Sylius\Bundle\OrderBundle\Generator\OrderNumberGenerator.

pjedrzejewski · 2014-01-28T11:43:17Z

I'd say it should go to the OrderBundle for now. When we have another use case, we could move it somehow.

umpirsky · 2014-01-28T16:57:32Z

@pjedrzejewski Please check this PR. It is not finished yet. I want to remove persisting from listener https://github.com/Sylius/Sylius/pull/904/files#diff-a3e834f246ffed7bca4e312d71f0e3d8R74 but how to persist this number then? Making bidirectional one to one relation order - number does not make much sense, and we already have number property in order model which can cause confusion. I am also not sure if number and order models should be related at all.

Any ideas how to handle this? @winzou @Richtermeister @stloyd

winzou · 2014-01-29T01:58:55Z

src/Sylius/Bundle/OrderBundle/Doctrine/ORM/NumberRepository.php

+
+class NumberRepository extends EntityRepository implements NumberRepositoryInterface
+{
+    public function getLastNumber()


Could you add directly a type or any better name for future entities distinction? So that this method would become getLastNumber('order') to get the last number for order.

umpirsky · 2014-01-29T12:48:36Z

@pjedrzejewski Is it good to go now?

umpirsky · 2014-01-30T09:40:30Z

@pjedrzejewski Piiing :)

umpirsky · 2014-02-03T08:13:10Z

@pjedrzejewski Please, 5 days...

winzou · 2014-02-03T08:27:37Z

src/Sylius/Bundle/OrderBundle/Doctrine/ORM/NumberRepository.php

+    {
+        try {
+            return $this->getQueryBuilder()
+                ->select('o.id')


Could you use $this->getAlias instead?

winzou · 2014-02-03T08:29:41Z

Apart from the $this->getAlias in the repository (which can leads to serious wtf, we really need to force it in every repository we have), seems good to merge.
I would have prefered to make it generic at first shot (we know we will need it several times), but this is something we can change later.

umpirsky · 2014-02-03T08:33:07Z

@winzou Fixed.

pjedrzejewski · 2014-02-03T08:43:22Z

@umpirsky Waiting for travis. Why wouldn't you use composer repository override method, instead of waiting for merge?

umpirsky · 2014-02-03T08:45:58Z

@pjedrzejewski I can wait, but I see no reason simple PRs like this stand and wait for several days.

pjedrzejewski · 2014-02-03T09:02:10Z

@umpirsky Because merging them does not involve only clicking the merge button. This PR is not that simple as you would think.

umpirsky · 2014-02-03T09:03:41Z

:)

Fixed unique order number error

pjedrzejewski · 2014-02-03T09:18:42Z

Thank you patient Sasha! 👍

umpirsky · 2014-02-03T09:32:52Z

Richtermeister · 2014-02-03T23:01:06Z

@umpirsky Nice work man! Thanks for getting this done.

lucian-v · 2014-02-05T19:18:27Z

I have encountered an issue related to this.
After arriving at the last step in the checkout process, if the user hits the back button (on the page) he is taken to the cart (not the previous page) and when completing the process for a second time it ends with an error.
Any idea where this is coming from or how could it be fixed?

SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '100' for key 'UNIQ_ACB1F1A88D9F6D38'" 
{"exception":"[object] (Doctrine\\DBAL\\DBALException: 
An exception occurred while executing 'INSERT INTO sylius_number (order_id) VALUES (?)' with params [100]

pjedrzejewski · 2014-02-05T19:27:40Z

@lucian-v Could you open a separate issue?

iglijin · 2014-03-13T16:14:56Z

src/Sylius/Bundle/OrderBundle/EventListener/OrderNumberListener.php

+        $number = $this->numberRepository->createNew();
+        $number->setOrder($order);
+
+        $this->numberManager->persist($number);


There shouldn't be a Number persist if the order already has a number, bug reproduces on :

Fail on paypal express checkout -> payment page

Dummy payment -> place order runs into a fatal error doctrine can't sava another sylius_number with same order_id

kayue reviewed Jan 24, 2014
View reviewed changes

kayue mentioned this pull request Jan 28, 2014

Unique Order Id's #922

Closed

winzou reviewed Jan 29, 2014
View reviewed changes

winzou reviewed Feb 3, 2014
View reviewed changes

Fixed unique order number error

272077a

pjedrzejewski pushed a commit that referenced this pull request Feb 3, 2014

Merge pull request #904 from umpirsky/fix/last-order-number

ebfe7a2

Fixed unique order number error

pjedrzejewski merged commit ebfe7a2 into Sylius:master Feb 3, 2014

umpirsky deleted the fix/last-order-number branch February 3, 2014 09:30

lucian-v mentioned this pull request Feb 6, 2014

Unique sylius_number error #993

Closed

iglijin reviewed Mar 13, 2014
View reviewed changes

Fixed unique order number error #904

Fixed unique order number error #904

Conversation

umpirsky commented Jan 24, 2014

pjedrzejewski commented Jan 24, 2014

umpirsky commented Jan 24, 2014

umpirsky commented Jan 24, 2014

pjedrzejewski commented Jan 24, 2014

umpirsky commented Jan 24, 2014

umpirsky commented Jan 24, 2014

pjedrzejewski commented Jan 24, 2014

umpirsky commented Jan 24, 2014

kayue Jan 24, 2014

Choose a reason for hiding this comment

umpirsky Jan 24, 2014

Choose a reason for hiding this comment

Richtermeister Jan 24, 2014

Choose a reason for hiding this comment

Richtermeister commented Jan 24, 2014

umpirsky commented Jan 24, 2014

umpirsky commented Jan 27, 2014

kayue commented Jan 27, 2014

umpirsky commented Jan 27, 2014

winzou commented Jan 27, 2014

pjedrzejewski commented Jan 27, 2014

umpirsky commented Jan 28, 2014

pjedrzejewski commented Jan 28, 2014

umpirsky commented Jan 28, 2014

pjedrzejewski commented Jan 28, 2014

stloyd commented Jan 28, 2014

winzou commented Jan 28, 2014

pjedrzejewski commented Jan 28, 2014

umpirsky commented Jan 28, 2014

winzou commented Jan 28, 2014

umpirsky commented Jan 28, 2014

umpirsky commented Jan 28, 2014

pjedrzejewski commented Jan 28, 2014

umpirsky commented Jan 28, 2014

winzou Jan 29, 2014

Choose a reason for hiding this comment

umpirsky commented Jan 29, 2014

umpirsky commented Jan 30, 2014

umpirsky commented Feb 3, 2014

winzou Feb 3, 2014

Choose a reason for hiding this comment

winzou commented Feb 3, 2014

umpirsky commented Feb 3, 2014

pjedrzejewski commented Feb 3, 2014

umpirsky commented Feb 3, 2014

pjedrzejewski commented Feb 3, 2014

umpirsky commented Feb 3, 2014

pjedrzejewski commented Feb 3, 2014

umpirsky commented Feb 3, 2014

Richtermeister commented Feb 3, 2014

lucian-v commented Feb 5, 2014

pjedrzejewski commented Feb 5, 2014

iglijin Mar 13, 2014

Choose a reason for hiding this comment