[FIX] duplicate container_deposit products in history

[gfcapalbo]

When visualizing paid orders, the container_products associated to a product would show duplicated.

this is because after loading an existing pos.order from the backend, the code would still add another container_deposit_product, therefore duplicating them all , in visualization.

the problem was seen by users when accessing the "orders" icon, and visualizing paid orders, they would appear with duplicates. also the receipts, generated from that visualization were also incorrect.

This would only be a POS visualization problem, because the pos.order had been payed and closed and was not editable, but the visualization was broken, and also the receipts were printed wrong on every step of the way.

I initially tried to put a check on init_from_Json, to stop the orders coming from json to have duplicates re-added, but thiswould not stop the duplicates from appearing on the currently active order, wich was also, at times loaded form json (e.g. page reload of the same session).

a simple way to stop the addition of the lines was to check the state, an open active order needs the container_deposit_product , all the orders that have a state do not, they already have the container products added.

i prefer to check for order.state == undefined to determine currently active order instead of !['done', 'invoiced', 'paid'].includes(order.state) to cover future state extensions.

after few temporary solutions, i found no need to add flags in constructor of the order or in init_from_Json.

The option of sanitizing json source from the container_products and leaving the auto-add active for all orders is a bad idea, because it de-syncs odoo and pos. if someone were to manually removes the container_deposit_product line from the backend , this will show the correct lines, without re-adding.

tested 25 minutes in various settings. undergoing more testing from others.

[gfcapalbo]

summary of technical findings TL;DR:

The module adds an extra orderline for products that have a deposit product.
These deposit product orderlines are then saved in the backend upon confirmation.

Unfortunately, when loading history, the "deposit product" orderline , is both loaded from the backend and then re-added with the same logic, resulting in a double deposit product line. We need to distinguish orders loaded from backend from live orders loaded from pos.db.

We have found that the order state is a reliable criteria to stop re-adding the deposit product , and determine it is already coming from the backend. https://github.com/odoo/odoo/blob/16.0/addons/point_of_sale/static/src/js/models.js#L2664
Locked orders cannot be saved to DB (wich in pos language means saved to pos.db) https://github.com/odoo/odoo/blob/16.0/addons/point_of_sale/static/src/js/models.js#L2578
the order is saved as pos.order here:

https://github.com/odoo/odoo/blob/16.0/addons/point_of_sale/static/src/js/models.js#L1032

We can see it is removed from the pos.db, and not synced anymore.
The automatically added deposit products are saved to the backend, from this moment on, it will be loaded with a state , and with the auto added deposit products present in the pos.order, and should not be re-adedded.

In conclusion: Orders loaded from backend will have "state" in there Json and be set as "locked" (unmodifiable). These orders are written as pos.order records in the backend at the moment of payment. All other orders are saved in pos.db, do not have state.

Implementing the connection between the product and it's added deposit in the backend too, would also be a solution and stop the addition , but if the user changes the backend in any way (quantity, removing or adding deposit products) this type of sync would force additions customer may not want. We do not want this module to force modifications the user may have overridden.

[thomaspaulb]

So yes I feel I can approve this because even though I think the "proper" solution would have been that we also save "container_deposit_line" on the sale.order.line id in the backend, to hard-link the product line with its deposit line, so that when reloading it from the backend the both are linked:

1. The "state === undefined" criterion also works because at least in 16.0 point_of_sale, the only function that really loads orders from the backend is _fetchSyncedOrders, and that one always loads finished orders anyway, so state !== undefined and it does solve the bug of the historic orders that are loaded, having the duplicate lines.

2. Looking at state is actually better, because in fact you never want a deposit product to be (re)added if it is not already present in a paid order: the order is finalized and should not be changed. People could have manually removed the deposit product as a correction and then that manual correction should be respected.

So I can live with this

[gfcapalbo]

@thomaspaulb In certain circumstances we find our order.state is still undefined. new solution is better.

To distinguish payed orders and not payed orders , all we need to do is fetch unpayed orders from posGlobalState object:
https://github.com/OCA/OCB/blob/16.0/addons/point_of_sale/static/src/js/models.js#L698-L701

If our line.order is not in the unpayed orders, we do not apply the logic.
Only payed orders are recorded in backend , with the container_deposit permanently added, and are removed from pos.db once payment happens and pos.order is created:
https://github.com/odoo/odoo/blob/16.0/addons/point_of_sale/static/src/js/models.js#L1043

[thomaspaulb]

If anything this would be pos.db, but get_order_list does not get from pos.db (=localStorage), but directly from this.orders, which as far as I can see is just a list of all orders that have been touched since this particular browser tab was opened. So the claim that this comment does is false.

[gfcapalbo]

@thomaspaulb
pos.db.. i will correct my typo now. but get_orders is connected to pos.db

You can see that the orders attribute of the pos object is taken from pos.db: https://github.com/odoo/odoo/blob/16.0/addons/point_of_sale/static/src/js/models.js#L246

get_orders fetches pos.orders, pos.orders is taken on session load from pos.db.orders

the comment is true.

[thomaspaulb]

This looks like it is a good criterium, I tested on Runboat and it seems like this is indeed a measure of which orders are open in the current session and not paid yet.

[thomaspaulb]

nonblocking but the comment is technically false