-
-
Notifications
You must be signed in to change notification settings - Fork 33
[16.0][ADD] stock_location_orderpoint_change_priority #20
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
base: 16.0
Are you sure you want to change the base?
[16.0][ADD] stock_location_orderpoint_change_priority #20
Conversation
Hi @mt-software-de, |
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
Hi @mt-software-de, |
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.
Thx for this change.
Why isn't that implemented in the base addon?
Why hasn't this been implemented in places where we know that the priority might change? For example on _merge_moves
and on __prepare_procurements
?
replenish_move_manual = self._get_replenishment_move(manual_orderpoint) | ||
|
||
self.assertTrue(replenish_move_manual) | ||
self.assertEqual(12.0, replenish_move_manual.product_uom_qty) |
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.
self.assertEqual(12.0, replenish_move_manual.product_uom_qty) | |
self.assertEqual(12.0, replenish_move_manual.product_uom_qty) | |
self.assertEqual("0", replenish_move_manual.priority) |
domain = [ | ||
("location_dest_id", "child_of", self.location_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.
This should have a self.ensure_one()
check. Otherwise self.location_id.id
could fail.
domain = [ | |
("location_dest_id", "child_of", self.location_id.id), | |
self.ensure_one() | |
domain = [ | |
("location_dest_id", "child_of", self.location_id.id), |
replenishment_moves_by_location = ( | ||
orderpoint._find_current_replenishment_moves_for_orderpoint_location( | ||
products=products | ||
) | ||
) | ||
|
||
for ( | ||
__location, | ||
replenishment_moves, | ||
) in replenishment_moves_by_location.items(): | ||
replenishment_moves_to_update |= replenishment_moves | ||
|
||
if replenishment_moves_to_update: | ||
replenishment_moves_to_update.write( | ||
{ | ||
"priority": orderpoint.priority, | ||
} | ||
) |
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 is _find_current_replenishment_moves_for_orderpoint_location
returning the moves grouped by location, when the location is never used? By not grouping it it would ease the code.
replenishment_moves_by_location = ( | |
orderpoint._find_current_replenishment_moves_for_orderpoint_location( | |
products=products | |
) | |
) | |
for ( | |
__location, | |
replenishment_moves, | |
) in replenishment_moves_by_location.items(): | |
replenishment_moves_to_update |= replenishment_moves | |
if replenishment_moves_to_update: | |
replenishment_moves_to_update.write( | |
{ | |
"priority": orderpoint.priority, | |
} | |
) | |
replenishment_moves = ( | |
orderpoint._find_current_replenishment_moves_for_orderpoint_location( | |
products=products | |
) | |
) | |
if replenishment_moves: | |
replenishment_moves.write( | |
{ | |
"priority": orderpoint.priority, | |
} | |
) |
We consider the orderpoint has been triggered, so we need | ||
""" | ||
replenishment_moves_to_update = self.env["stock.move"].browse() |
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.
This shouldn't be defined outside the loop. Otherwise i could write on moves which not belong to the orderpoint.
a9ab73e
to
55f5eca
Compare
The unit test `test_stock_location_orderpoint_priority_change` was failing in `stock_location_orderpoint` because the associated logic was moved to `stock_location_orderpoint_change_priority`. This commit removes the now-obsolete test from `stock_location_orderpoint` as an identical test already exists and passes within the `stock_location_orderpoint_change_priority` module, where the relevant priority change logic now resides.
@mt-software-de Thanks for the review. I will come back later on this. I was jyst fixing this branch as I need it for our client. |
…e in replenishment logic. This commit delays the priority update to occur *after* moves have been merged. This ensures that the function merging replenishment moves still has access to the "not yet updated" priorities preventing information loss.
Some test was failing because the code saying "day +1" fails if we are the last day of a month.
@rousseldenis @nicolas-delbovier-acsone Can we close this one in favour of #55. |
That's fine with me. |
No description provided.