16.0 mig stock reserve sale #1788

hugo-cordoba · 2023-07-11T09:47:14Z

[MIG] stock_reserve_sale: migrate from v13.0 to v16.0

Hi.

After trying to push the developments in version 14 and version 16, and receiving no feedback, I have decided to create a PR and take responsibility for the migration.

I am open to any kind of suggestions.

I have included improvements in the module that I find useful and improve the module at user and usability level.

We will find a button to access the reservations associated to the order and the reservation slip. They will only be visible if the quotation has reservations.

Within the reservations, we will also be able to access the associated reservation delivery note, the reservation movement and even release (undo) the reservation.

A button is added in the header, along with the rest of the buttons, to cancel the reservation made. When pressed, it will cancel the reservation movements and the reservation delivery note, so the corresponding smart buttons will no longer be visible.

The button to reserve stock will only be visible if there are units to be reserved. If all units have already been reserved, the button will disappear.

…_reserve and stock_reserve_sale.

…CA#50.

before running the tests. This fixes the failure experienced when running the test in --init mode rather than --update mode.

TT39113

gdgellatly

Just initial install errors - functional review to come

gdgellatly · 2023-08-14T21:06:17Z

stock_reserve_sale/model/sale.py

+    has_stock_reservation = fields.Boolean(
+        compute="_compute_stock_reservation",
+        readonly=True,
+        multi="stock_reservation",


Suggested change

multi="stock_reservation",

gdgellatly · 2023-08-14T21:06:29Z

stock_reserve_sale/model/sale.py

+    is_stock_reservable = fields.Boolean(
+        compute="_compute_stock_reservation",
+        readonly=True,
+        multi="stock_reservation",


Suggested change

multi="stock_reservation",

gdgellatly · 2023-08-14T21:09:53Z

stock_reserve_sale/model/sale.py

+        string="Can Have Stock Reservations",
+    )
+    reserves_count = fields.Integer(compute="_compute_reserves_count")
+    all_lines_reserved = fields.Boolean(


this will need a compute_sudo

2023-08-14 21:04:18,128 1 WARNING mig odoo.modules.registry: sale.order: inconsistent 'compute_sudo' for computed fields: reserves_count, all_lines_reserved

gdgellatly · 2023-08-14T21:11:01Z

stock_reserve_sale/view/sale.xml

+                <button
+                    name="%(action_sale_stock_reserve)d"
+                    type="action"
+                    icon="fa-lock"


2023-08-14 21:09:09,611 1 WARNING mig odoo.addons.base.models.ir_ui_view: A button with icon attribute (fa-lock) must have title in its tag, parents, descendants or have text

gdgellatly · 2023-08-14T21:11:36Z

stock_reserve_sale/view/sale.xml

+                <button
+                    name="release_stock_reservation"
+                    type="object"
+                    icon="fa-undo"


2023-08-14 21:09:09,612 1 WARNING mig odoo.addons.base.models.ir_ui_view: A button with icon attribute (fa-undo) must have title in its tag, parents, descendants or have text

rousseldenis · 2023-08-16T06:40:03Z

/ocabot migration stock_reserve_sale

OCA-git-bot · 2023-08-16T08:14:21Z

The migration issue (#1494) has not been updated to reference the current pull request because a previous pull request (#1695) is not closed.
Perhaps you should check that there is no duplicate work.
CC @XanderDeJaegere

hugo-cordoba · 2023-08-17T10:32:29Z

Thanks for the feedback @gdgellatly. In the afternoon, when I have some free time, I'll check it out!!!

FrancoMaxime

Maybe we could improve these remark

FrancoMaxime · 2023-09-08T09:55:18Z

stock_reserve_sale/model/sale.py

+        line_ids = [line.id for order in self for line in order.order_line]
+        lines = self.order_line.browse(line_ids)


Hello, thanks for your works.
I think we can optimize this function.
[EDIT 11/09] Adds @gdgellatly suggestion instead of self.mapped("order_line")

Suggested change

line_ids = [line.id for order in self for line in order.order_line]

lines = self.order_line.browse(line_ids)

lines = self.order_line

not just self.order_line?

If self is a recordset, i don't remember if odoo gives all order_line for all record without a mapped.

@FrancoMaxime In v16, you can call the relation field on multi recordset

@rousseldenis Thanks

FrancoMaxime · 2023-09-08T10:01:07Z

stock_reserve_sale/model/sale.py

+        stock_picking = (
+            self.env["stock.picking"]
+            .search([("origin", "=", self.name)])
+            .filtered(lambda a: a.state not in "cancel")
+        )


I think we can make a search with both conditions.

Suggested change

stock_picking = (

self.env["stock.picking"]

.search([("origin", "=", self.name)])

.filtered(lambda a: a.state not in "cancel")

)

stock_picking = (

self.env["stock.picking"]

.search([

("origin", "=", self.name),

("state", "!=", "cancel")

])

)

Generally a != condition for a state field in search is a bad idea. The problem with these is often state fields are indexed and the other conditions aren't. But because of the generally bad distribution of states it is usually faster if you aren't using '=' for a state which is a small percentage, the queries can be quite bad. The search would have prefetched anyway, the cancelled records will be minimal. IDK, I feel like I would want testing on big databases first as one of the performance bottlenecks we always look for is exactly this suggestion. It is almost always better to reverse it to a state in type query or use filtered.

Just looking at this, the filtered clause is wrong anyway, it should be != instead of not in. But as it happens, in testing the suggested query on a database with 1m pickings performed basically identically as the original BEFORE the filtered clause and planned identically using standard Odoo indexes, so I think it is a good suggestion.

@gdgellatly Thanks for your tests

@gdgellatly Have you tried with a condition like "state", "in", "[....]" ? Did they give the same result (in time)?

Doesn't really matter I think. The existing code is clearly a mistake. But also the planner will have statistics on uniqueness and origin field is indexed so regardless, for this query, I think no matter the state condition, it will pick the origin index.

FrancoMaxime · 2023-09-08T10:06:00Z

stock_reserve_sale/model/sale_order_line.py

+            wh_routes = warehouse.route_ids
+            wh_route_ids = [route.id for route in wh_routes]


We can optimize this

Suggested change

wh_routes = warehouse.route_ids

wh_route_ids = [route.id for route in wh_routes]

wh_route_ids = warehouse.route_ids.ids

FrancoMaxime · 2023-09-08T10:43:31Z

stock_reserve_sale/model/sale_order_line.py

+        reserv_ids = [reserv.id for line in self for reserv in line.reservation_ids]
+        reservations = self.env["stock.reservation"].browse(reserv_ids)


We can optimize

Suggested change

reserv_ids = [reserv.id for line in self for reserv in line.reservation_ids]

reservations = self.env["stock.reservation"].browse(reserv_ids)

reservations = self.mapped("reservation_ids")

why mapped? surely just self.reservation_ids

See comment #1788 (comment)

FrancoMaxime · 2023-09-08T10:53:29Z

stock_reserve_sale/model/stock_reserve.py

+            stock_picking = (
+                self.env["stock.picking"]
+                .search([("origin", "=", self.sale_id.name)])
+                .filtered(lambda a: a.state not in "cancel")
+            )


We can add both conditions in the search

Suggested change

stock_picking = (

self.env["stock.picking"]

.search([("origin", "=", self.sale_id.name)])

.filtered(lambda a: a.state not in "cancel")

)

stock_picking = (

self.env["stock.picking"]

.search([

("origin", "=", self.sale_id.name),

("state", "!=", "cancel"),

])

)

As above, I think a suggestion like this which could have big performance implications should at least be tested. I can do it, just not right now.

rousseldenis · 2023-08-16T06:42:25Z

stock_reserve_sale/model/sale_order_line.py

+                len(line.reservation_ids) > 0 or line.order_id.state != "draft"
+            )
+
+    reservation_ids = fields.One2many(


Could you put fields on top ?

rousseldenis · 2023-08-16T06:43:28Z

stock_reserve_sale/model/sale_order_line.py

+            for line in self:
+                if not line.reservation_ids:
+                    continue
+                raise except_orm(


except_orm has been deprecated a long time ago. Could you change to UserError ?

rousseldenis · 2023-08-16T06:49:22Z

stock_reserve_sale/model/stock_reserve.py

@@ -10,10 +10,26 @@ class StockReservation(models.Model):
        "sale.order.line", string="Sale Order Line", ondelete="cascade", copy=False
    )
    sale_id = fields.Many2one(
-        "sale.order", string="Sale Order", related="sale_line_id.order_id"
+        "sale.order", string="Sale Order", store=True, related="sale_line_id.order_id"
    )

    def release_reserve(self):
        for rec in self:
            rec.sale_line_id = False


self.update({"sale_line_id": False})

rousseldenis · 2023-09-11T06:44:21Z

stock_reserve_sale/model/sale.py

+        line_ids = [line.id for order in self for line in order.order_line]
+        lines = self.order_line.browse(line_ids)


@FrancoMaxime In v16, you can call the relation field on multi recordset

rousseldenis · 2023-09-11T06:47:18Z

stock_reserve_sale/model/sale.py

+        )
+        for order in self:
+            if reserve_ids:
+                order.reserves_count = reserve_ids[0]["sale_id_count"]


@HugoCordobaLeal Is that correct to take only the first one ???

jappi00 · 2023-12-15T10:08:15Z

@HugoCordobaLeal are you working on this? If you want I can help on this to implement the last change requests.

acsonefho

Thanks for this migration 😃
⚠️ Some importants changes/fixes are required before merging this PR

acsonefho · 2023-12-19T10:04:28Z

stock_reserve_sale/wizard/sale_stock_reserve.py

+    @api.model
+    def _default_location_id(self):
+        return self.env["stock.reservation"].get_location_from_ref(
+            "stock.stock_location_stock"


That'll work only if you're connected using the default company (ID 1).
Should be something like:

@api.model def _default_location_id(self): domain = [ '|', ('company_id', '=', self.env.company.id), ('company_id', '=', False), ] return self.env["stock.warehouse"].search(domain, limit=1).lot_stock_id

acsonefho · 2023-12-19T10:05:22Z

stock_reserve_sale/wizard/sale_stock_reserve.py

+
+        if active_model == "sale.order":
+            sales = env["sale.order"].browse(active_ids)
+            line_ids = [line.id for sale in sales for line in sale.order_line]


Simplest way to read that:
line_ids = sales.order_line.ids

acsonefho · 2023-12-19T10:07:20Z

stock_reserve_sale/model/sale.py

+
+    def _compute_reserves_count(self):
+        reserve_ids = self.env["stock.reservation"]._read_group(
+            domain=expression.AND(


useless.
domain=[("sale_id", "in", self.ids)]

acsonefho · 2023-12-19T10:14:48Z

stock_reserve_sale/model/sale_order_line.py

+        StockRule = self.env["stock.rule"]
+        product = self.product_id
+        product_route_ids = [
+            x.id for x in product.route_ids + product.categ_id.total_route_ids


More readable:
product_route_ids = (product.route_ids + product.categ_id.total_route_ids).ids

acsonefho · 2023-12-19T10:17:54Z

stock_reserve_sale/model/sale_order_line.py

+                ("route_id", "in", wh_route_ids),
+            ]
+            rules = StockRule.search(domain, order="route_sequence, sequence")
+        if rules:


This if section should be inside of the previous if section.
Because you already have a limit=1.
And this is useless: you should add a limit=1 in the search(...) on the line before.

acsonefho · 2023-12-19T10:18:36Z

stock_reserve_sale/model/sale_order_line.py

+            rules = StockRule.search(domain, order="route_sequence, sequence")
+        if rules:
+            fields.first(rules)
+        return False


This function should return a rule (_get_line_rule(...)) but return False.

So you're looking for a rule but you never return it.

And if you really don't find a rule, you should return an empty recordset.

acsonefho · 2023-12-19T10:21:11Z

stock_reserve_sale/model/sale_order_line.py

+    is_readonly = fields.Boolean(compute="_compute_is_readonly", store=False)
+
+    def release_stock_reservation(self):
+        reserv_ids = [reserv.id for line in self for reserv in line.reservation_ids]


More readable:
reserv_ids = self.reservation_ids.ids
But it's completely useless as you browse them just after.
You should do this:
reservations = self.reservation_ids

acsonefho · 2023-12-19T12:35:58Z

stock_reserve_sale/model/sale.py

+                order.reserves_count = reserve_ids[0]["sale_id_count"]
+            else:
+                order.reserves_count = 0
+            if order.reserves_count == len(lines):


How is this possible?
That can not happens in a multi-recordset computation.
If this compute is called on a recordset with more than one sale.order, this will be never True.

acsonefho · 2023-12-19T12:48:22Z

stock_reserve_sale/model/sale.py

+        stock_picking = (
+            self.env["stock.picking"]
+            .search([("origin", "=", self.name)])
+            .filtered(lambda a: a.state not in "cancel")


This is not correct at all.
The value of state could be "draft", "cancel",...

As is, this mean:
"draft" in ("c", "a", "n", "c", "e", "l") ? => True/False

It should be:

# Use limit=1 because we use the record with .id later stock_picking = self.env["stock.picking"].search([("origin", "=", self.name), ("state", "!=", "cancel")], limit=1)

acsonefho · 2023-12-19T12:56:10Z

stock_reserve_sale/model/stock_reserve.py

+            stock_picking = (
+                self.env["stock.picking"]
+                .search([("origin", "=", self.sale_id.name)])
+                .filtered(lambda a: a.state not in "cancel")


This is not correct. Like my previous comment

acsonefho · 2023-12-19T12:57:27Z

stock_reserve_sale/model/stock_reserve.py

+        if stock_picking:
+            view_id = self.env.ref("stock.view_picking_form").id
+            action.update(views=[(view_id, "form")], res_id=stock_picking.id)
+            return action


This should be outstide of this if section.

rousseldenis · 2024-01-16T12:12:47Z

@username-hugo Do you plan to attend comments ?

rousseldenis · 2024-01-25T13:53:29Z

@username-hugo

astirpe · 2024-04-22T08:06:31Z

I'm working on the migration of this module to V17: #2004

rousseldenis · 2024-04-22T08:11:26Z

@hugo-cordoba

github-actions · 2024-08-25T12:29:10Z

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

guewen and others added 26 commits July 10, 2023 15:05

[ADD] started stock_reserve_sale to reserve lines of quotations

c4c8019

mark unported module as uninstallable

a57767e

reactivate module stock_reserve_sale

53e04de

lint

052c480

fix pep8 W503

4e6edb2

make stock_reserve_sale owner_aware

574e215

Added the translation template file and Finnish translations to stock…

8250055

…_reserve and stock_reserve_sale.

Add README.rst and remove description from __openerp__.py

3a55b66

Multiple reservations can now be released at once. This fixes issue O…

c85d76e

…CA#50.

Add Spanish translation of stock reserve modules

e8c2f8f

force recomputation of stock.location hierachy

8fc78f8

before running the tests. This fixes the failure experienced when running the test in --init mode rather than --update mode.

OCA Transbot updated translations from Transifex

f6da8ba

[UPD] prefix versions with 8.0

ec895a3

[MIG] Make modules uninstallable

7c16b12

[FIX] remove en.po that was erroneously created by transbot

53dd4ce

[MIG] Rename manifest files

8b2255e

remove obsolete .pot files [ci skip]

d0d2841

[IMP] stock_reserve_sale: black, isort, prettier

5de10de

[MIG] stock_reserve_sale: Migration to v13.0

ab70510

[UPD] Update stock_reserve_sale.pot

e265f3e

[UPD] README.rst

f1cfed8

[ADD] icon.png

3f99cce

[FIX+IMP] Update dotfiles + switch to GH actions

7ce144b

[IMP] stock_reserve_sale: Show Reserve Stock button in sent orders.

45cd741

TT39113

[IMP] stock_reserve_sale: pre-commit stuff

11e6ca0

git commit -m "[MIG] stock_reserve_sale: Migration to 16.0"

668f2c1

hugo-cordoba force-pushed the 16.0-mig-stock_reserve_sale branch from ed2b844 to 668f2c1 Compare July 11, 2023 09:55

hugo-cordoba mentioned this pull request Jul 11, 2023

Migration to version 16.0 #1494

Open

69 tasks

rousseldenis mentioned this pull request Jul 28, 2023

[14.0][ADD] sale_stock_prebook: Add process to prebook a sale order's stock OCA/sale-workflow#2624

Open

gdgellatly reviewed Aug 14, 2023

View reviewed changes

rousseldenis added needs review migration labels Aug 16, 2023

rousseldenis mentioned this pull request Aug 16, 2023

[MIG] stock_reserve_sale: Migration to 16.0 #1695

Closed

OCA-git-bot added this to the 16.0 milestone Aug 16, 2023

FrancoMaxime reviewed Sep 8, 2023

View reviewed changes

rousseldenis reviewed Sep 11, 2023

View reviewed changes

acsonefho suggested changes Dec 19, 2023

View reviewed changes

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Aug 25, 2024

github-actions bot closed this Sep 29, 2024

		line_ids = [line.id for order in self for line in order.order_line]
		lines = self.order_line.browse(line_ids)

	line_ids = [line.id for order in self for line in order.order_line]
	lines = self.order_line.browse(line_ids)
	lines = self.order_line

		wh_routes = warehouse.route_ids
		wh_route_ids = [route.id for route in wh_routes]

	wh_routes = warehouse.route_ids
	wh_route_ids = [route.id for route in wh_routes]
	wh_route_ids = warehouse.route_ids.ids

		reserv_ids = [reserv.id for line in self for reserv in line.reservation_ids]
		reservations = self.env["stock.reservation"].browse(reserv_ids)

	reserv_ids = [reserv.id for line in self for reserv in line.reservation_ids]
	reservations = self.env["stock.reservation"].browse(reserv_ids)
	reservations = self.mapped("reservation_ids")

16.0 mig stock reserve sale #1788

16.0 mig stock reserve sale #1788

Conversation

hugo-cordoba commented Jul 11, 2023 • edited Loading

gdgellatly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousseldenis commented Aug 16, 2023

OCA-git-bot commented Aug 16, 2023

hugo-cordoba commented Aug 17, 2023

FrancoMaxime left a comment

Choose a reason for hiding this comment

FrancoMaxime Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jappi00 commented Dec 15, 2023

acsonefho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousseldenis commented Jan 16, 2024

rousseldenis commented Jan 25, 2024

astirpe commented Apr 22, 2024

rousseldenis commented Apr 22, 2024

github-actions bot commented Aug 25, 2024

hugo-cordoba commented Jul 11, 2023 •

edited

Loading

FrancoMaxime Sep 8, 2023 •

edited

Loading