-
-
Notifications
You must be signed in to change notification settings - Fork 738
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
[10.0][MIG] stock_cycle_count #332
Conversation
d055950
to
8ba2f63
Compare
@alan196 @rousseldenis you might be interested in this one |
PERCENT * (theoretical - abs_discrepancy) / theoretical, | ||
0.0) | ||
if not inv.line_ids and inv.state == 'done': | ||
inv.inventory_accuracy = 100.0 |
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.
100.0 → PERCENT
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.
Sure, thanks
c4fa7c0
to
7497965
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.
LGTM but there are IMHO lot of api issues
@lreficent Can you fix them ?
inverse_name='cycle_count_id', | ||
string='Inventory Adjustment', | ||
track_visibility='onchange') | ||
inventory_adj_count = fields.Integer(compute='_count_inventory_adj') |
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.
Following https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#models
the method name should be _compute...
def do_cancel(self): | ||
self.write({'state': 'cancelled'}) | ||
|
||
@api.model |
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.
@lreficent This one should have been decorated multi with ensure_one. Inconsistency between decorator and self value (not void recordset)
_name = 'stock.cycle.count.rule' | ||
_description = "Stock Cycle Counts Rules" | ||
|
||
@api.one |
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.
Deprecated
('accuracy', _('Minimum Accuracy')), | ||
('zero', _('Zero Confirmation'))] | ||
|
||
@api.one |
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.
Deprecated
else: | ||
self.rule_description = _('(No description provided.)') | ||
|
||
@api.constrains('periodic_qty_per_period', 'periodic_count_period') |
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.
@lreficent self can be a multi recordset
if not quants: | ||
self.create_zero_confirmation_cycle_count() | ||
|
||
def create_zero_confirmation_cycle_count(self): |
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.
@api.multi, self.ensure_one()
help='Number of latest inventories used to calculate location ' | ||
'accuracy') | ||
|
||
@api.one |
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.
Deprecated
('rule_type', '!=', 'zero'), ('warehouse_ids', '=', self.id)]) | ||
return rules | ||
|
||
@api.one |
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.
Deprecated
|
||
@api.model | ||
def _cycle_count_rules_to_compute(self): | ||
rules = self.cycle_count_rule_ids.search([ |
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.
To have more clearer code, could you use self.env[model].search() here ?
@api.model | ||
def _cycle_count_rules_to_compute(self): | ||
rules = self.cycle_count_rule_ids.search([ | ||
('rule_type', '!=', 'zero'), ('warehouse_ids', '=', self.id)]) |
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.
I think this works. But quite weird. I would have done ('warehouse_ids', 'in', [self.id])
01e8395
to
a4d958c
Compare
@rousseldenis yes, I have to clean all those api usages. I will do after the above improvements for v9 are merged |
6380d48
to
2c9cc48
Compare
@rousseldenis Finally I attended your review, Thanks! |
30b87fa
to
0d41688
Compare
I triggered a runbot rebuild |
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 comments
company_id = self.env['res.company']._company_default_get(self._name) | ||
return company_id | ||
|
||
name = fields.Char(string='Name', readonly=True) |
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 follow OCA conventions and set fields definitions above.
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.
done
_description = "Stock Cycle Counts" | ||
_inherit = 'mail.thread' | ||
|
||
@api.multi |
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.
@api.depends('stock_adjustment_ids')
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.
done
) | ||
|
||
@api.onchange('rule_type') | ||
def _compute_rule_description(self): |
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 follow OCA convention for onchange function name
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 both a compute method and an onchange method. https://github.com/Eficent/stock-logistics-warehouse/blob/b9b4c2283fdffebbcdb508e49e07a5e512d70cb6/stock_cycle_count/models/stock_cycle_count_rule.py#L100
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.
If you put
@api.depends('rule_type')
The change on rule_type value will trigger the change on rule_description on the interface.
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.
done
) | ||
|
||
@api.onchange('location_ids') | ||
def _get_warehouses(self): |
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.
Same as above.
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.
fixed
def _compute_rule_periodic(self, locs): | ||
cycle_counts = [] | ||
for loc in locs: | ||
last_inventories = self.env['stock.inventory'].search([ |
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.
Maybe the search can be directly order by date desc and limit=1 to fetch less records.
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.
right, done.
@api.multi | ||
def _compute_loc_accuracy(self): | ||
for rec in self: | ||
history = self.env['stock.inventory'].search([ |
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.
Maybe it could be interesting to have an inventory history field on locations ?
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.
what kind of field, could you please explain a little bit more?
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.
An 'inventory_history_ids' that represent inventories done on a location.
def action_done(self): | ||
super(StockMove, self).action_done() | ||
for move in self: | ||
move.location_id.check_zero_confirmation() |
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.
check_zero_confirmation() function is a real multi, maybe it could be called on all locations ?
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.
done
return rules | ||
|
||
@api.multi | ||
def action_compute_cycle_count_rules(self): |
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.
IMHO, maybe this function could be refactorized as I see some searched records that can become fields(that could help reading - and some could be called on other code parts ?).
And the values set on the create at the end could be retrieved through a _prepare hook function.
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.
agree, but I can't do that at the moment, I would look into it when I have some spare time :)
runbot status unrelated. @lreficent if you rebase it should get green. |
120bf42
to
8c13a56
Compare
* Update README. * Fixes: DEFAULT_SERVER_DATETIME_FORMAT, PERCENT variables and sale price calculation.
…manual creation of cycle counts.
…racking to some of them.
…tment related to a cycle count cannot be modified.
a6556c8
to
0fe8761
Compare
* simplify fetching latest inventory date * use api.depends where needed * add hook for cycle count creation * take advantage of api.multi on check_zero_confirmation method
0fe8761
to
d5a9498
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.
Should add Roadmap for further version to improve the action_compute_cycle_count_rules in stock.warehouse
@rousseldenis would you do the honors of merging this? 😉 |
@lreficent Wait for all green checks, but yes of course! |
[10.0] port stock_auto_move to 10.0
Migration from v9 to v10.
Depends on: