Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MIG stock_mts_mto_rule to 12.0 #586

Closed
wants to merge 29 commits into from

Conversation

jaredkipe
Copy link
Contributor

There were major changes to the way Warehouses maintain their routes/rules and self heal in 12.0

This necessitated large changes to models/stock_warehouse.py and some changes to test expectations.

In general, I read through the new Odoo code to get a feel for how they are updating and healing routes, then I applied the same methodology to the refactor work. I didn't really comment these methods as a search through purchase, mrp, stock for the same method names will turn up very similar looking code (that is also not commented).

Some not so great things...
In the model stock.warehouse Odoo provides a shim for an old method named get_all_routes_for_wh that is now effectively named _get_all_routes. However, in purchase_stock module, they incorrectly call the parent method that isn't actually the method being called.

    @api.multi
    def _get_all_routes(self):
        routes = super(StockWarehouse, self).get_all_routes_for_wh()

This can lead to infinite loops, so ultimately I ended up just resorting to the same 'shim' approach that Odoo took.

florian-dacosta and others added 28 commits May 5, 2019 13:25
… procurement rules to handle the mts+mto scenario
Add explanation for finding this setting
Conforms to the latest README template: bugtracker, runbot etc.
Fixes bugtracker URL on some modules.
States OCA as maintainer, removes other contributors from the 'Maintainer' section.
…en warehouse is two/three steps delivery propagate move by mts-mto in pick/pack/out
OCA Transbot updated translations from Transifex
There's a traceback when renaming warehouse code (without renaming warehouse name), as name argument is False.
OCA Transbot updated translations from Transifex

OCA Transbot updated translations from Transifex
This method is called from the write() method the one how support
multiple records an api.multi method.

I was making some test when I tried to activate the mto+mts option for
multiple warehouse and the next error appears: ``ValueError: Expected singleton``

In order to fix this error I only added a loop to manage the multiple
registers.
OCA Transbot updated translations from Transifex
@oca-clabot
Copy link

Hey @jaredkipe, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/cla
Here is a list of the users:

Appreciation of efforts,
OCA CLAbot

@rousseldenis rousseldenis added this to the 12.0 milestone May 10, 2019
@OCA-git-bot OCA-git-bot mentioned this pull request May 10, 2019
54 tasks
@manney
Copy link

manney commented May 13, 2019

We've just pulled this branch. I did a quick functional test on it as well as ran all of the tests. Everything seems to pass with flying colors. 👍

@jaredkipe
Copy link
Contributor Author

@manney thank you!

Anyone else interested in trying it, and potentially other beta branches like connector is welcome to use our automated docker builds at registry.gitlab.com/hibou-io/hibou-odoo/suite:12.0-test

Copy link
Sponsor Contributor

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review.

@@ -0,0 +1 @@
from . import model
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{'name': 'Stock MTS+MTO Rule',
'version': '12.0.1.0.0',
'author': 'Hibou Corp.,Akretion,Odoo Community Association (OCA)',
'website': 'http://www.akretion.com',
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you follow OCA conventions for manifest (website, missing entires) ?

https://github.com/OCA/maintainer-tools/blob/master/template/module/__manifest__.py

'summary': 'Add a MTS+MTO route',
'depends': ['stock',
],
'demo': [],
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove void one

if not rule.mts_rule_id or not rule.mto_rule_id:
msg = _('No MTS or MTO rule configured on procurement '
'rule: %s!') % (rule.name, )
raise UserError(msg)
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constrains must raise ValidationError

msg = _('Inconsistency between the source locations of '
'the mts and mto rules linked to the procurement '
'rule: %s! It should be the same.') % (rule.name,)
raise UserError(msg)
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constrains must raise ValidationError

qty_available = product.uom_id._compute_quantity(
virtual_available, product_uom)

if qty_available > 0:
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use float_compare

needed_qty = self.get_mto_qty_to_order(product_id, product_qty,
product_uom, values)

if needed_qty == 0.0:
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use float_is_zero

getattr(self.mts_rule_id, '_run_%s' % self.mts_rule_id.action)(
product_id, product_qty, product_uom, location_id, name,
origin, values)
elif needed_qty == product_qty:
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use float_compare

@@ -0,0 +1,95 @@
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).

from odoo import models, fields
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
from odoo import models, fields
from odoo import fields, models, _

Copy link
Contributor

@florian-dacosta florian-dacosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the migration!
I made some minor comments
Otherwise I did functional tests and it seems to work as expected.

@@ -0,0 +1,92 @@
.. image:: https://img.shields.io/badge/license-AGPL--3-blue.png
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to split the readme, like explain here : https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-12.0

------------

* Florian da Costa <florian.dacosta@akretion.com>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add yourself in the contributor list, but not in the authors


{'name': 'Stock MTS+MTO Rule',
'version': '12.0.1.0.0',
'author': 'Hibou Corp.,Akretion,Odoo Community Association (OCA)',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should put yourself in the contributor and not in the authors

@rousseldenis
Copy link
Sponsor Contributor

@jaredkipe Do you plan to attend comments?

@Jacekdaa
Copy link

nothing specific. at this moment he will give a git chada and honestly say little wien.

@rousseldenis
Copy link
Sponsor Contributor

nothing specific. at this moment he will give a git chada and honestly say little wien.

Ugh, didn't understand...

@jaredkipe
Copy link
Contributor Author

Now that Odoo has merged in odoo/odoo#33288 , I have removed the patch and re-worked the test.

Otherwise I believe I have addressed everyone's concerns other than splitting the README into multiple files.

@rousseldenis
Copy link
Sponsor Contributor

@jaredkipe Just lint errors and LGTM!

@jaredkipe
Copy link
Contributor Author

Should be good now, feel free to squash as there are now a couple of simple 'fix' commits that don't really help.

@rousseldenis
Copy link
Sponsor Contributor

Should be good now, feel free to squash as there are now a couple of simple 'fix' commits that don't really help.

Hi Jared, I cannot just squash your four migration commits during merging. Could you do it ?

Major changes to the way Warehouses update their routes/rules and self heal.
Improved rule code to use `float_compare` and `float_is_zero`.
@jaredkipe jaredkipe force-pushed the 12.0-mig-stock_mts_mto_rule branch from 9d37a0b to aadbde3 Compare June 6, 2019 19:21
@jaredkipe
Copy link
Contributor Author

All done with that, the only major difference is that it now thinks the stock_warehouse.py file is a complete rewrite due to the changes and model => models module rename.

@salbassersyentys
Copy link

Hi Folks, any idea when this PR is likely to be approved? we do have a customer waiting on this - blocking issue for him & his business. Would be very much appreciated, thanks a lot in advance! :-)

Copy link
Sponsor Contributor

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review. LGTM

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@rousseldenis
Copy link
Sponsor Contributor

/ocabot merge

@OCA-git-bot
Copy link
Contributor

Rebased to 12.0-ocabot-merge-pr-586-by-rousseldenis-bump-no, awaiting test results.

@OCA-git-bot
Copy link
Contributor

Merged at 638aa1c. Don't worry if GitHub says there are unmerged commits: it is due to a rebase before merge. All commits of this PR have been merged on the main branch.

OCA-git-bot added a commit that referenced this pull request Jun 21, 2019
Signed-off-by rousseldenis
@jaredkipe jaredkipe mentioned this pull request Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet