-
-
Notifications
You must be signed in to change notification settings - Fork 759
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
[9.0] [MIG] project_task_materials_stock module #208
[9.0] [MIG] project_task_materials_stock module #208
Conversation
c0bb675
to
d94fca2
Compare
d94fca2
to
ba7de77
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor things to change
============= | ||
|
||
#. If you are a project manager, go to Project > Configuration > Stages and | ||
check option 'Consume Material' in Task Stage to generate a stock move when |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indent these lines 3 spaces to get a correct visualization
|
||
#. Go to a task, edit, and add materials to be consumed on tab "Materials". | ||
#. Move task to an stage on consume material are activated and moves and | ||
analytic lines will be created. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indent this
@@ -0,0 +1,62 @@ | |||
# Translation of Odoo Server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please squash your migration commits. Runbot will be correct when the dependency is merged.
ae30140
to
cfab78f
Compare
cfab78f
to
9bb6b53
Compare
@cubells, can you improve tests for avoiding dropping coverage? |
2e7c1ff
to
b19cdb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some not required implementation notes, but at least please give your thoughts on them.
_inherit = 'project.task.type' | ||
|
||
consume_material = fields.Boolean( | ||
string='Consume Material', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the string equals the field name, you don't need it.
|
||
consume_material = fields.Boolean( | ||
string='Consume Material', | ||
help="If you mark this check, when a task goes to this state," |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a space at the end of the string, or it will join without it.
elif task.stock_move_ids.filtered(lambda r: r.state == 'assigned'): | ||
task.stock_state = 'assigned' | ||
elif task.stock_move_ids.filtered(lambda r: r.state == 'done'): | ||
task.stock_state = 'done' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably this would be faster & shorter:
if not task.stock_move_ids:
task.stock_state = 'pending'
else:
states = task.mapped("stock_move_ids.state")
for ("confirmed", "assigned", "done") as state:
if state in states:
task.stock_state = state
break
if not moves_done: | ||
moves.filtered(lambda r: r.state == 'assigned').do_unreserve() | ||
moves.filtered( | ||
lambda r: r.state in ['waiting', 'confirmed', 'assigned'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a set instead of a list, it's way faster in this case.
task.material_ids.create_analytic_line() | ||
else: | ||
if task.unlink_stock_move(): | ||
if any(task.material_ids.mapped( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need any
here AFAIK.
|
||
@api.multi | ||
def action_assign(self): | ||
self.mapped('stock_move_ids').action_assign() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you need to return
it? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not needed because this method is introduced in this module
|
||
@api.multi | ||
def action_done(self): | ||
self.mapped('stock_move_ids').action_done() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you need to return
it? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not needed because this method is introduced in this module
comodel_name='stock.move', string='Stock Move') | ||
analytic_line_id = fields.Many2one( | ||
comodel_name='account.analytic.line', string='Analytic Line') | ||
product_uom = fields.Many2one( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add _id
suffix. And possibly an oldname
if this was like this in v8.
ctx['uom'] = self.product_uom.id | ||
# Compute based on pricetype | ||
amount_unit = \ | ||
self.product_id.with_context(ctx).price_get( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better do self.product_id.with_context(uom=self.product_uom.id).price_get(
and remove those ctx
lines above.
Status? |
6f3e4a3
to
4e69a0b
Compare
All is done @dreispt @pedrobaeza @yajo |
* Singleton unlink tasks * Several suggestions * PEP8 * Warning if invoiced lines and move to not consume stage
4e69a0b
to
052df80
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review OK, just a little question...
<page string="Products"> | ||
<field name="material_ids" attrs="{'readonly':[('stock_state','!=', 'pending')]}"> | ||
<tree string="Materials used" editable="top"> | ||
<field name="product_id" domain="[('type', 'in', ['consu', 'product'])]"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense under some scenarios to be able to put other product types? Such as considering the task has consumed a service product?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carlosdauden, do you remember why we finally discard that option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consume action create an stock move and stock move only can be generated with products that not are services.
That's the reason.
cc @Tecnativa