-
-
Notifications
You must be signed in to change notification settings - Fork 723
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
[15.0][IMP]stock_cycle_count: several improvements #1997
[15.0][IMP]stock_cycle_count: several improvements #1997
Conversation
44d4221
to
4c963c0
Compare
ee770b8
to
e51735b
Compare
ff64074
to
09edb8a
Compare
@@ -76,6 +62,27 @@ def _domain_cycle_count_candidate(self): | |||
("location_id", "in", self.location_ids.ids), | |||
] | |||
|
|||
def _calculate_inventory_accuracy(self): | |||
for inv in self: | |||
sml = self.env["stock.move.line"].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.
Mmmh, IMHO this has to be refined has multiple searches will be done...
09c6757
to
be2ca4a
Compare
be2ca4a
to
28c1195
Compare
inv.action_state_to_in_progress() | ||
try: | ||
inv.action_state_to_in_progress() | ||
except Exception as e: |
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.
Isn't it a too broad Exception ?
Moreover, shouldn't we manage the exception in a more elegant manner than 'just' logging ?
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.
Hey @rousseldenis
You might be right. I implemented it this way because we don't want the cron job to stop running due to a validation error from a location that already has an inventory adjustment in progress.
Do you have any suggestions for a better approach?
Thanks.
3983318
to
6906e79
Compare
347ea05
to
2e5b1f0
Compare
3235706
to
bd1e6a6
Compare
This PR has the |
stock_inventory_records = self.mapped("stock_adjustment_ids") | ||
for record in stock_inventory_records: | ||
if record.responsible_id.id != vals["responsible_id"]: | ||
record.with_context(no_propagate=True).write( |
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 don't really like such code. Don't you have another mean to avoid this ?
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.
Why not modifying stock_inventory to change the responsible_id fields to computed field (stored/readonly=False) and override the compute here ?
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.
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.
@rousseldenis, we agree with your proposal and have applied it.
Thank you
5a042b4
to
b8fd2aa
Compare
@rousseldenis friendly reminder here, can you update your last review? @JoanSForgeFlow has attended your request |
], | ||
order="create_date asc", | ||
).filtered( | ||
lambda x: not x.company_id.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.
Doesn't it correspond to normal record rule ?
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.
In other words, is the filtered
necessary ?
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.
@rousseldenis You are correct, since we are searching for stock move lines by location, and the locations already belong to a company, it is not needed this filter. I'll make the change.
Thanks
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.
@rousseldenis @ArnauCForgeFlow it is not the normal record rule, it is filtering based on the quant company, the normal record rules filters based on the users companies, we cannot rely on the user having the proper companies.
That been said, I think instead of filtering, it can be done with a domain that includes a couple of company_id clauses.
ec684b0
to
34d68ad
Compare
34d68ad
to
2c9ecf6
Compare
Hey @rousseldenis just a friendly reminder, could you please update your review? |
Let's move forward /ocabot merge major |
On my way to merge this fine PR! |
Congratulations, your PR was merged at 969677c. Thanks a lot for contributing to OCA. ❤️ |
@ArnauCForgeFlow can you update the 16.0 fwport (#2169) to be fully aligned with this one? |
Depends on: #1995
This PR include the following improvements, se commits for more details.