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

stock_batch_picking #241

Merged
merged 12 commits into from
Sep 12, 2016
Merged

stock_batch_picking #241

merged 12 commits into from
Sep 12, 2016

Conversation

cyrilgdn
Copy link

@cyrilgdn cyrilgdn commented Jun 30, 2016

Reworked of picking_dispatch (in V8 or less) to match Odoo 9 new picking with pack operations.

@cyrilgdn cyrilgdn changed the title [WIP] stock_batch_picking stock_batch_picking Jul 11, 2016

class ResCompany(models.Model):
_inherit = 'res.company'
default_picker_id = fields.Many2one(
Copy link

Choose a reason for hiding this comment

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

Can we have this at the warehouse level instead? Having this at the company level will be an issue when multiple warehouse at separate locations come into play.

Copy link

Choose a reason for hiding this comment

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

IMO this is a blocking detail as it does not play nicely with multiple warehouses that would have default pickers.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. I'll label this as "Needs fixing" until then

@lasley
Copy link

lasley commented Jul 18, 2016

Thanks for the submission @cyrilgdn - comments inline.

IMO there is a lot of critical logic being introduced here without test coverage. An example would be the BatchAggregation model w/ the syntax errors in the dictionary creations.

@pedrobaeza - Any idea why there is no coverage.io for this repo? Travis config looks fine, is it maybe just not configured in coveralls? It would definitely help us in identifying the test coverage gaps in this PR.

@cyrilgdn
Copy link
Author

@lasley In Python, {'done', 'cancel'} is a set, not a dictionary. It's similar to set(['done', 'cancel']).

The coverage is 100% except for report logic. It was copied and adapt from picking_dispatch, I don't really know how to test it.

@lasley
Copy link

lasley commented Jul 19, 2016

Whoah TIL on the set shorthand - Neat - Thanks!

Understood, I won't force the testing then - although I would also be more than happy to assist if you had some specific questions on it.

<odoo>
<!-- stock.batch.picking form view -->
<record model="ir.ui.view" id="stock_bacth_picking_form">
<field name="name">stock.bacth.picking.form</field>
Copy link
Member

Choose a reason for hiding this comment

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

s/bacth/batch/

@yvaucher
Copy link
Member

Can't we retrieve part of the translations ?

@cyrilgdn
Copy link
Author

Updated with remarks taking in account. (typo, some doc, translations, ...)

@rafaelbn
Copy link
Member

Please @carlosdauden review, thanks!

else:
warehouse = False

return warehouse.default_picker_id if warehouse else False
Copy link
Contributor

Choose a reason for hiding this comment

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

After changes return warehouse.default_picker_id directly

@pedrobaeza pedrobaeza mentioned this pull request Aug 5, 2016
34 tasks
carrier = {}
operations = self.operations_by_loc[locations]
for operation in operations:
p_code = operation.product_id.default_code
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to me to done on purpose, but, why it is not grouped by product id?
If several products have not established default_code, total quantity (all products) is printed in only one line with the description of the first product.
You can sort by default_code anyway.

Copy link
Member

Choose a reason for hiding this comment

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

You still need to fix this point

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I will try to fix this as soon as I can...

It's True that, as I'm still not comfortable with reports, I've just migrated the picking_dispatch report file and made the little adaptations :)
I did not pay attention to existing bugs, my bad.

@@ -43,14 +43,9 @@ def _default_picker_id(self):
else:
warehouse = self.env['stock.warehouse'].search([
('company_id', '=', self.env.user.company_id.id)
], limit=1)
], limit=1)[:1]
Copy link

Choose a reason for hiding this comment

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

Aren't we achieving the desired result already with the limit=1? The result of this should be either a singleton or an empty recordset which would eval to False.

Copy link
Member

Choose a reason for hiding this comment

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

yep, [:1] is useless

Copy link
Author

Choose a reason for hiding this comment

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

@lasley Right! And I changed it (again...)

As it was a very very little change, can we merge this PR ?

@lasley lasley added this to the 9.0 milestone Aug 16, 2016
('company_id', '=', self.env.user.company_id.id)
], limit=1)

if warehouse:
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed

Copy link
Author

Choose a reason for hiding this comment

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

@pedrobaeza Already removed as you commented an outdated diff ;)

@cyrilgdn
Copy link
Author

@lasley @pedrobaeza @carlosdauden Fixes are done. But can someone tell me how to fix the new pylint-odoo error (old-api7-method-defined) in this case?

@lasley
Copy link

lasley commented Aug 26, 2016

I'm as stumped as you on that one @cyrilgdn - I see no examples of an api decorated __init__ in Odoo code. @moylop260 I think we may need an exception in our linter here.

@moylop260
Copy link

moylop260 commented Aug 28, 2016

FYI the red of travis is because pylint show:

stock_batch_picking/report/batch_report.py:7: [W0403(relative-import), ] Relative import 'print_batch', should be 'stock_batch_picking.report.print_batch'
stock_batch_picking/report/print_batch.py:9: [W0403(relative-import), ] Relative import 'batch_aggregation', should be 'stock_batch_picking.report.batch_aggregation'

The line stock_batch_picking/report/batch_report.py:7
You should change it:
s/from print_batch import PrintBatch/from . print_batch import PrintBatch/g (Notice the prefix dot)

The line stock_batch_picking/report/print_batch.py:9
You should change it:
s/from batch_aggregation import BatchAggregation/from . batch_aggregation import BatchAggregation/g (Notice the prefix dot)

It because you have this library in the same path and avoid errors with libraries named with the same "virtual path" the dot use in the "virtual path" and here path

@moylop260
Copy link

moylop260 commented Aug 28, 2016

I'm as stumped as you on that one @cyrilgdn - I see no examples of an api decorated __init__ in Odoo code. @moylop260 I think we may need an exception in our linter here.

Maybe we could use the new api for reporting using AbstractModel
You could find examples in branch master of odoo/odoo with:
rgrep "AbstractModel" addons/*/report --include=*.py

Good examples:

@cyrilgdn
Copy link
Author

cyrilgdn commented Sep 5, 2016

Checks green!

@moylop260 With the new api reporting system, we have to override render_html and redefine the entire docargs dict (with docs, doc_ids, etc....) ??
In old api we just have to update the localcontext dict...

@@ -0,0 +1,395 @@
# Translation of Odoo Server.
Copy link
Member

Choose a reason for hiding this comment

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

Remove POT file

Choose a reason for hiding this comment

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

If the guideline changed after OCA/maintainer-tools#97 could add it in our guidelines?

Copy link
Member

Choose a reason for hiding this comment

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

My opinion on this have changed since that issue, because I found somebody translating from an outdated POT file.

@cyrilgdn
Copy link
Author

@pedrobaeza @moylop260 POT file removed.

@cyrilgdn
Copy link
Author

Can we merge this PR?

@yvaucher
Copy link
Member

Thanks @cyrilgdn
Merging

@yvaucher yvaucher merged commit 1cc6d03 into OCA:9.0 Sep 12, 2016
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

10 participants