Infinite warning loop
The unique key of user warnings is often composed of the record representation and some fixed string. If a record was created during the transaction and raises a user warning, it leads to an infinite loop of warnings. This happens because once the warning is acknowledged the request is reexecuted and the newly created record gets a new id this time (at least with postgresql, see comment below).
This happens for example with the no_origin warning of stock.move. If the quantity in moves and outgoing moves in a shipment is not balanced, the _sync_inventory_to_outgoing() method creates a new move to compensate for this difference. But this new move has no origin and therefore raises the no_origin warning. If the warning is acknowledged, the request is reprocessed but the newly generated move gets a new id this time and the warning is reraised.
This bug only occurs with the postgresql backend, sqlite works fine. The difference is, that with sqlite the assigned id in both requests is the same, but postgresql assigns a new id every time.
I'm not sure about the best way to fix this issue, so I'm leaving it unassigned. But if there is an agreement about the source of the problem, I'm willing to provide a patch.
Files
| Download | Creator | Timestamp | Type |
|---|---|---|---|
| unnamed | @guillemNaN | 2014-10-20 11:28:00.727000 UTC | text/plain |
| unnamed | @guillemNaN | 2014-10-20 12:26:45.581000 UTC | text/plain |
| unnamed | @guillemNaN | 2014-10-20 13:30:10.986000 UTC | text/plain |
| unnamed | @guillemNaN | 2014-10-20 13:55:43.591000 UTC | text/plain |
| unnamed | @guillemNaN | 2014-10-20 14:23:52.413000 UTC | text/plain |
| unnamed | @guillemNaN | 2014-10-20 16:56:27.839000 UTC | text/plain |
Activity
added trytond type::behavior + 1 deleted label
> I think it could be solved by adding a new button for just synchronization.
> This will create the extra move without trying to assign it and so the id will be kept.
You mean an additional workflow step? IMO the stock move workflow is already
very long. And the issue is rather generic and may occur at various places where
warnings are used. I'd prefer a generic solution if there is one.The solution proposed; create an Outgoing Move with extra quantity generates a problem on how to invoice this extra quantity.
We propose that the pack() function check if there is more quantity (by product, and extensible by lot) in "internal moves" than in outgoing moves and, in this case, DON'T ALLOW to continue (raise user error, not warning).
The workaround, which could be explained to user in the error message, is go to the "Outgoing moves" and change the quantity. With this step, if the sale is invoiced from shipment, it will be invoiced for all shipped quantity, not only the expected one.On 20 Oct 12:55, Guillem Barba wrote:
> The solution proposed; create an Outgoing Move with extra quantity generates a problem on how to invoice this extra quantity.
This is wrong. Because it will be required to put an origin.
> We propose that the pack() function check if there is more quantity (by product, and extensible by lot) in "internal moves" than in outgoing moves and, in this case, DON'T ALLOW to continue (raise user error, not warning).
> The workaround, which could be explained to user in the error message, is go to the "Outgoing moves" and change the quantity. With this step, if the sale is invoiced from shipment, it will be invoiced for all shipped quantity, not only the expected one.
This is too constraintful.2014-10-20 13:02 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:
>
> Cédric Krier <cedric.krier@b2ck.com> added the comment:
>
> On 20 Oct 12:55, Guillem Barba wrote:
> > The solution proposed; create an Outgoing Move with extra quantity
> generates a problem on how to invoice this extra quantity.
>
> This is wrong. Because it will be required to put an origin.
>
The "origin" field is not visible. It should be if this solution is
implemented.
> > We propose that the pack() function check if there is more quantity (by
> product, and extensible by lot) in "internal moves" than in outgoing moves
> and, in this case, DON'T ALLOW to continue (raise user error, not warning).
> > The workaround, which could be explained to user in the error message,
> is go to the "Outgoing moves" and change the quantity. With this step, if
> the sale is invoiced from shipment, it will be invoiced for all shipped
> quantity, not only the expected one.
>
> This is too constraintful.
>
not too much because you can modify the "Outgoing Moves" to fit with the
quantity of inventory moves.
If the "origin" field is shown and editable in Outgoing Move's form, it is
another solution. In this case, we think the warning message when the user
"pack" the shipment should be more specific and explain than it is shipping
more quantity than expected and you (the user) must to complete the
Outgoing Move with the proper origin.
So, the complete use case is:
1. The user try to pack a shipment with more quantity in "Inventory moves"
than in "Outgoing moves"
2. An User Warning is shown explaining this situation. If the user accept
it, the sync_outoint... function creates the extra move without origin an
DON'T assign it to allow the user to set the origin.
3. (Optional) The user sets the origin
4. The user finalize the shipment. If he didn't set the origin a new
warning message (the current ones) is shown to advice about it.
We still prefer our solution, but it is acceptable too.
--
Guillem Barba
http://www.guillem.alcarrer.netOn 20 Oct 13:28, Guillem Barba wrote:
> 2014-10-20 13:02 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:
>
> >
> > Cédric Krier <cedric.krier@b2ck.com> added the comment:
> >
> > On 20 Oct 12:55, Guillem Barba wrote:
> > > The solution proposed; create an Outgoing Move with extra quantity
> > generates a problem on how to invoice this extra quantity.
> >
> > This is wrong. Because it will be required to put an origin.
> >
>
> The "origin" field is not visible. It should be if this solution is
> implemented.
No, the origin should not be shown.
> > > We propose that the pack() function check if there is more quantity (by
> > product, and extensible by lot) in "internal moves" than in outgoing moves
> > and, in this case, DON'T ALLOW to continue (raise user error, not warning).
> > > The workaround, which could be explained to user in the error message,
> > is go to the "Outgoing moves" and change the quantity. With this step, if
> > the sale is invoiced from shipment, it will be invoiced for all shipped
> > quantity, not only the expected one.
> >
> > This is too constraintful.
> >
>
> not too much because you can modify the "Outgoing Moves" to fit with the
> quantity of inventory moves.
>
> If the "origin" field is shown and editable in Outgoing Move's form, it is
> another solution. In this case, we think the warning message when the user
> "pack" the shipment should be more specific and explain than it is shipping
> more quantity than expected and you (the user) must to complete the
> Outgoing Move with the proper origin.
>
> So, the complete use case is:
> 1. The user try to pack a shipment with more quantity in "Inventory moves"
> than in "Outgoing moves"
> 2. An User Warning is shown explaining this situation. If the user accept
> it, the sync_outoint... function creates the extra move without origin an
> DON'T assign it to allow the user to set the origin.
> 3. (Optional) The user sets the origin
> 4. The user finalize the shipment. If he didn't set the origin a new
> warning message (the current ones) is shown to advice about it.
>
> We still prefer our solution, but it is acceptable too.
I will be against your solution because it is not generic and not
flexible.2014-10-20 14:02 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:
>
> Cédric Krier <cedric.krier@b2ck.com> added the comment:
>
> On 20 Oct 13:28, Guillem Barba wrote:
> > 2014-10-20 13:02 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:
> >
> > >
> > > Cédric Krier <cedric.krier@b2ck.com> added the comment:
> > >
> > > On 20 Oct 12:55, Guillem Barba wrote:
> > > > The solution proposed; create an Outgoing Move with extra quantity
> > > generates a problem on how to invoice this extra quantity.
> > >
> > > This is wrong. Because it will be required to put an origin.
> > >
> >
> > The "origin" field is not visible. It should be if this solution is
> > implemented.
>
> No, the origin should not be shown.
You said "It will be required to put an origin", and you also say "the
origin should not be shown".
How is it possible to do the required action without the field?
> > > We propose that the pack() function check if there is more quantity
> (by
> > > product, and extensible by lot) in "internal moves" than in outgoing
> moves
> > > and, in this case, DON'T ALLOW to continue (raise user error, not
> warning).
> > > > The workaround, which could be explained to user in the error
> message,
> > > is go to the "Outgoing moves" and change the quantity. With this step,
> if
> > > the sale is invoiced from shipment, it will be invoiced for all shipped
> > > quantity, not only the expected one.
> > >
> > > This is too constraintful.
> > >
> >
> > not too much because you can modify the "Outgoing Moves" to fit with the
> > quantity of inventory moves.
> >
> > If the "origin" field is shown and editable in Outgoing Move's form, it
> is
> > another solution. In this case, we think the warning message when the
> user
> > "pack" the shipment should be more specific and explain than it is
> shipping
> > more quantity than expected and you (the user) must to complete the
> > Outgoing Move with the proper origin.
> >
> > So, the complete use case is:
> > 1. The user try to pack a shipment with more quantity in "Inventory
> moves"
> > than in "Outgoing moves"
> > 2. An User Warning is shown explaining this situation. If the user accept
> > it, the sync_outoint... function creates the extra move without origin an
> > DON'T assign it to allow the user to set the origin.
> > 3. (Optional) The user sets the origin
> > 4. The user finalize the shipment. If he didn't set the origin a new
> > warning message (the current ones) is shown to advice about it.
> >
> > We still prefer our solution, but it is acceptable too.
>
> I will be against your solution because it is not generic and not
> flexible.
Which solution is not generic nor flexible? why?
Our initial proposition is very similar to current behaviour but it doesn't
create the move for extra quantity automatically and leave the user to
create it or augment the quantity of some of existing moves.
The second proposition is a improvement of yours; it doesn't require a new
step in the shipment workflow and it show to the user a better warning
message because the "not origin" message isn't user friendly (especially if
he can't see the "origin"). IMHO, it is a message for developers which
advice that something is wrong.On 20 Oct 14:26, Guillem Barba wrote:
> You said "It will be required to put an origin", and you also say "the
> origin should not be shown".
> How is it possible to do the required action without the field?
Yes exactly there is no reason to let the user put an origin.
> > > > We propose that the pack() function check if there is more quantity
> > (by
> > > > product, and extensible by lot) in "internal moves" than in outgoing
> > moves
> > > > and, in this case, DON'T ALLOW to continue (raise user error, not
> > warning).
> > > > > The workaround, which could be explained to user in the error
> > message,
> > > > is go to the "Outgoing moves" and change the quantity. With this step,
> > if
> > > > the sale is invoiced from shipment, it will be invoiced for all shipped
> > > > quantity, not only the expected one.
> > > >
> > > > This is too constraintful.
> > > >
> > >
> > > not too much because you can modify the "Outgoing Moves" to fit with the
> > > quantity of inventory moves.
> > >
> > > If the "origin" field is shown and editable in Outgoing Move's form, it
> > is
> > > another solution. In this case, we think the warning message when the
> > user
> > > "pack" the shipment should be more specific and explain than it is
> > shipping
> > > more quantity than expected and you (the user) must to complete the
> > > Outgoing Move with the proper origin.
> > >
> > > So, the complete use case is:
> > > 1. The user try to pack a shipment with more quantity in "Inventory
> > moves"
> > > than in "Outgoing moves"
> > > 2. An User Warning is shown explaining this situation. If the user accept
> > > it, the sync_outoint... function creates the extra move without origin an
> > > DON'T assign it to allow the user to set the origin.
> > > 3. (Optional) The user sets the origin
> > > 4. The user finalize the shipment. If he didn't set the origin a new
> > > warning message (the current ones) is shown to advice about it.
> > >
> > > We still prefer our solution, but it is acceptable too.
> >
> > I will be against your solution because it is not generic and not
> > flexible.
>
> Which solution is not generic nor flexible? why?
>
> Our initial proposition is very similar to current behaviour but it doesn't
> create the move for extra quantity automatically and leave the user to
> create it or augment the quantity of some of existing moves.
This is wrong, user should not create move.
> The second proposition is a improvement of yours; it doesn't require a new
> step in the shipment workflow and it show to the user a better warning
> message because the "not origin" message isn't user friendly (especially if
> he can't see the "origin"). IMHO, it is a message for developers which
> advice that something is wrong.
It is not *generic* because it is focused on one single unique workflow.
The origin check is a generic functionality, you will not fix this issue
with a custom workflow.2014-10-20 14:40 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:
>
> Cédric Krier <cedric.krier@b2ck.com> added the comment:
>
> On 20 Oct 14:26, Guillem Barba wrote:
> > You said "It will be required to put an origin", and you also say "the
> > origin should not be shown".
> > How is it possible to do the required action without the field?
>
> Yes exactly there is no reason to let the user put an origin.
>
> > > > > We propose that the pack() function check if there is more quantity
> > > (by
> > > > > product, and extensible by lot) in "internal moves" than in
> outgoing
> > > moves
> > > > > and, in this case, DON'T ALLOW to continue (raise user error, not
> > > warning).
> > > > > > The workaround, which could be explained to user in the error
> > > message,
> > > > > is go to the "Outgoing moves" and change the quantity. With this
> step,
> > > if
> > > > > the sale is invoiced from shipment, it will be invoiced for all
> shipped
> > > > > quantity, not only the expected one.
> > > > >
> > > > > This is too constraintful.
> > > > >
> > > >
> > > > not too much because you can modify the "Outgoing Moves" to fit with
> the
> > > > quantity of inventory moves.
> > > >
> > > > If the "origin" field is shown and editable in Outgoing Move's form,
> it
> > > is
> > > > another solution. In this case, we think the warning message when the
> > > user
> > > > "pack" the shipment should be more specific and explain than it is
> > > shipping
> > > > more quantity than expected and you (the user) must to complete the
> > > > Outgoing Move with the proper origin.
> > > >
> > > > So, the complete use case is:
> > > > 1. The user try to pack a shipment with more quantity in "Inventory
> > > moves"
> > > > than in "Outgoing moves"
> > > > 2. An User Warning is shown explaining this situation. If the user
> accept
> > > > it, the sync_outoint... function creates the extra move without
> origin an
> > > > DON'T assign it to allow the user to set the origin.
> > > > 3. (Optional) The user sets the origin
> > > > 4. The user finalize the shipment. If he didn't set the origin a new
> > > > warning message (the current ones) is shown to advice about it.
> > > >
> > > > We still prefer our solution, but it is acceptable too.
> > >
> > > I will be against your solution because it is not generic and not
> > > flexible.
> >
> > Which solution is not generic nor flexible? why?
> >
> > Our initial proposition is very similar to current behaviour but it
> doesn't
> > create the move for extra quantity automatically and leave the user to
> > create it or augment the quantity of some of existing moves.
>
> This is wrong, user should not create move.
>
> > The second proposition is a improvement of yours; it doesn't require a
> new
> > step in the shipment workflow and it show to the user a better warning
> > message because the "not origin" message isn't user friendly (especially
> if
> > he can't see the "origin"). IMHO, it is a message for developers which
> > advice that something is wrong.
>
> It is not *generic* because it is focused on one single unique workflow.
> The origin check is a generic functionality, you will not fix this issue
> with a custom workflow.
OK, explain me what is your proposition?On 20 Oct 15:55, Guillem Barba wrote:
> 2014-10-20 15:48 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:
>
> >
> > Cédric Krier <cedric.krier@b2ck.com> added the comment:
> >
> > msg18032
> >
>
> well. After click on this new button/step and a new move with extra
> quantity is created. What is the next step?
Normal workflow, you will have the warning that you will be able to
skip.2014-10-20 16:06 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:
>
> Cédric Krier <cedric.krier@b2ck.com> added the comment:
>
> On 20 Oct 15:55, Guillem Barba wrote:
> > 2014-10-20 15:48 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:
> >
> > >
> > > Cédric Krier <cedric.krier@b2ck.com> added the comment:
> > >
> > > msg18032
> > >
> >
> > well. After click on this new button/step and a new move with extra
> > quantity is created. What is the next step?
>
> Normal workflow, you will have the warning that you will be able to
> skip.
It is wrong for 2 things:
a) it doesn't invoice the real shipped quantity. If the sale that generates
this shipment is configured to invoice from shipment, it works as expected
if the shipped quantity is equal or less than quantity in sale line but it
doesn't have the same behavior (invoice the shipped quantity) when the
shipped quantity is greater than the quantity in invoice.
b) it shows a warning message to the user that he doesn't understand nor
check (the origin field is not visible nor managed by user), he can't
manage the situation to avoid this warning message
Another little issue related to his is that the "Always ignore this
warning" option doesn't have the expected behavior. If there are more
"extra quantity" in the shipment, almost if the user check it for the first
move it will be shown again for the other moves.On 20 Oct 16:23, Guillem Barba wrote:
> Guillem Barba <guillembarba@gmail.com> added the comment:
> 2014-10-20 16:06 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:
> > Cédric Krier <cedric.krier@b2ck.com> added the comment:
> >
> > On 20 Oct 15:55, Guillem Barba wrote:
> > > 2014-10-20 15:48 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:
> > >
> > > >
> > > > Cédric Krier <cedric.krier@b2ck.com> added the comment:
> > > >
> > > > msg18032
> > > >
> > >
> > > well. After click on this new button/step and a new move with extra
> > > quantity is created. What is the next step?
> >
> > Normal workflow, you will have the warning that you will be able to
> > skip.
>
> It is wrong for 2 things:
> a) it doesn't invoice the real shipped quantity. If the sale that generates
> this shipment is configured to invoice from shipment, it works as expected
> if the shipped quantity is equal or less than quantity in sale line but it
> doesn't have the same behavior (invoice the shipped quantity) when the
> shipped quantity is greater than the quantity in invoice.
This is the expected behavior. If you sent more than what was agreed on
the sale, it is normal to not invoice the customer.
> b) it shows a warning message to the user that he doesn't understand nor
> check (the origin field is not visible nor managed by user), he can't
> manage the situation to avoid this warning message
Of course he can not because it must not.
> Another little issue related to his is that the "Always ignore this
> warning" option doesn't have the expected behavior. If there are more
> "extra quantity" in the shipment, almost if the user check it for the first
> move it will be shown again for the other moves.
This is the expected behavior.To be clear, when shipping extra quantity, it is normal by default to have blocker like the warning. It is also normal to not show the origin by default because it is for internal use (message could be perhaps improved by talking about linkage to document instead of origin).
But some could have a specific workflow and in this case they should decide how to handle extra quantity and so in this case the custom module should manage to link the new move to the right origin (which could be a sale).2014-10-20 18:08 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:
>
> Cédric Krier <cedric.krier@b2ck.com> added the comment:
>
> To be clear, when shipping extra quantity, it is normal by default to have
> blocker like the warning. It is also normal to not show the origin by
> default because it is for internal use (message could be perhaps improved
> by talking about linkage to document instead of origin).
>
A better solution is don't create this extra move because it's more
generic: it allow to handle (in a module or manually) this case creating a
new move or changing the quantity of some existing move.
> But some could have a specific workflow and in this case they should
> decide how to handle extra quantity and so in this case the custom module
> should manage to link the new move to the right origin (which could be a
> sale).
We already create custom modules to manage the common cases. But the
default behavior is wrong.On 20 Oct 18:56, Guillem Barba wrote:
>
> 2014-10-20 18:08 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:
>
> >
> > Cédric Krier <cedric.krier@b2ck.com> added the comment:
> >
> > To be clear, when shipping extra quantity, it is normal by default to have
> > blocker like the warning. It is also normal to not show the origin by
> > default because it is for internal use (message could be perhaps improved
> > by talking about linkage to document instead of origin).
> >
>
> A better solution is don't create this extra move because it's more
> generic: it allow to handle (in a module or manually) this case creating a
> new move or changing the quantity of some existing move.
Wrong what is packed must be shipped.
> > But some could have a specific workflow and in this case they should
> > decide how to handle extra quantity and so in this case the custom module
> > should manage to link the new move to the right origin (which could be a
> > sale).
>
> We already create custom modules to manage the common cases. But the
> default behavior is wrong.
No I totaly disagree, the default behavior is right because of all I
said previously. It will not change because don't provide any good
argument.On 20 d’octubre de 2014 19:36:17 CEST, "Cédric Krier" <issue_tracker@tryton.org> wrote:
>
>Cédric Krier <cedric.krier@b2ck.com> added the comment:
>
>On 20 Oct 18:56, Guillem Barba wrote:
>>
>> 2014-10-20 18:08 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:
>>
>> >
>> > Cédric Krier <cedric.krier@b2ck.com> added the comment:
>> >
>> > To be clear, when shipping extra quantity, it is normal by default
>to have
>> > blocker like the warning. It is also normal to not show the origin
>by
>> > default because it is for internal use (message could be perhaps
>improved
>> > by talking about linkage to document instead of origin).
>> >
>>
>> A better solution is don't create this extra move because it's more
>> generic: it allow to handle (in a module or manually) this case
>creating a
>> new move or changing the quantity of some existing move.
>
>Wrong what is packed must be shipped.
Completly agree on this.
>
>> > But some could have a specific workflow and in this case they
>should
>> > decide how to handle extra quantity and so in this case the custom
>module
>> > should manage to link the new move to the right origin (which could
>be a
>> > sale).
>>
>> We already create custom modules to manage the common cases. But the
>> default behavior is wrong.
>
>No I totaly disagree, the default behavior is right because of all I
>said previously. It will not change because don't provide any good
>argument.
But the current implementation forces to create a new move and shows an error message that can be improved. So AFAIU what Guillem is proposing tries to improve both things by having a more explicit message and let the user (or system by a custom module) decide what wants to do with the exceding quantity.
Just my 2 centsI still don't agree.
About the message, it just creates a custom message for one unique specific case, without any possibility of customization.
About the solution, msg18583 is exactly my solution but solved in a generic way and with the possibility of customization. But all others solution proposed are just constraintful, not customizable, unique workflow centric.
Here I'm fighting to keep the code simple, to prevent specific and allow customization.roba was right moving the sync to the assign step solve the problem but for me we should still keep it also on the pack step.
So here is a review that implement it: review5971002
I also make a change to the API to allow only creation of new move on assignation because we must only decrease the quantity of the move when we can no more go back to waiting otherwise we loose information to reset inventory moves.assigned to @ced
New changeset ed41959390ff by C?dric Krier in branch 'default':
Synchronise outgoing move with inventory on assignation
http://hg.tryton.org/modules/stock/rev/ed41959390ffNew changeset e48b1fe6c076 by C?dric Krier in branch 'default':
New API for _sync_inventory_to_outgoing
http://hg.tryton.org/modules/stock_lot/rev/e48b1fe6c076mentioned in issue #4261 (closed)
mentioned in issue #4931 (closed)
mentioned in issue #12503 (closed)
mentioned in merge request !737 (merged)
