[16.0][IMP] hr_personal_equipment_stock: Enhance PPE view - #1497
[16.0][IMP] hr_personal_equipment_stock: Enhance PPE view#1497anusriNPS wants to merge 2 commits into
Conversation
|
This PR has the |
|
@OCA/human-resources-maintainers we've been using this successfully in production for a couple of months now. Could you please take a look? Thanks! |
alexey-pelykh
left a comment
alexey-pelykh
left a comment
There was a problem hiding this comment.
The functional intent is sound -- showing serial/lot numbers on accepted equipment requests is useful. However, three issues need addressing:
- The
hr_personal_equipment_request/static/description/index.htmlchange is a regenerated README from a different module and should be dropped from this PR. - OCA convention requires a version bump in
__manifest__.pyfor IMP-type changes (e.g.,16.0.1.0.0->16.0.1.1.0). - No tests cover the new
lot_idsrelated field. At minimum, a test should verify the field is populated correctly when stock moves with lots are created via the equipment request flow.
| @@ -1,4 +1,3 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> | |||
There was a problem hiding this comment.
This file belongs to hr_personal_equipment_request, not hr_personal_equipment_stock. Looks like an auto-regenerated oca-gen-addon-readme artifact that leaked into this PR. Please remove.
9eb3142 to
58a99fb
Compare
marcos-mendez
left a comment
marcos-mendez
left a comment
There was a problem hiding this comment.
Automated Review -- Tests Failed
1. Root Cause
The test failure occurs because the stock.move is not properly linked to the stock.lot records during the test setup. Specifically, the lot_ids field on the stock.move is not correctly assigned, leading to an inconsistency between allocation.lot_ids and allocation.move_ids.lot_ids.
2. Suggested Fix
In test_compute_qty_delivered and related tests, ensure that move.lot_ids is correctly assigned using the write method or by directly assigning the recordset to move.lot_ids. Also, in the test_action_cancel_without_qty_delivered, the assignment of move.lot_ids should be done correctly to reflect actual serial numbers used in the test.
File: hr_personal_equipment_stock/tests/test_hr_personal_equipment_stock.py
Lines: Around 175, 218, and 232
Change:
move.lot_ids = [lot_id.id for lot_id in lot_ids]to:
move.write({'lot_ids': [(6, 0, [lot_id.id for lot_id in lot_ids])]})This ensures the many2many relationship is properly written to the database.
3. Additional Code Issues
-
Missing error handling in
_create_lot_ids: The function assumesproduct.product_variant_idexists and is valid, but no check is performed. This could cause runtime errors ifproducthas no variants.Suggested fix:
if not product.product_variant_id: raise UserError("Product must have a variant to create lots.")
-
Potential performance issue in
_create_lot_ids: Creating many records in a loop without batching may cause performance degradation in large-scale tests.Suggested improvement:
Useself.env["stock.lot"].create()with a list of dictionaries for better performance.
4. Test Improvements
Add the following test cases to improve coverage:
- Test with product tracking = 'none': Ensure
lot_idsis empty when no tracking is enabled. - Test with multiple serial numbers per move: Validate that multiple lots can be assigned to a single move.
- Test
lot_idson cancelled moves: Confirm thatlot_idsare cleared when a move is cancelled. - Test with
lot_idsafter backorder creation: Ensure thatlot_idsare correctly propagated in backorders.
Use OCA patterns:
- Use
TransactionCasefor tests involving data creation and manipulation. - Tag tests using
@tag('post_install', 'manual')if they require installation steps. - Use
SavepointCasefor tests that need database isolation, especially when dealing withstock.moveandstock.lotrelationships.
Example:
@tag('post_install', 'manual')
def test_lot_ids_with_tracking_none(self):
self.product_personal_equipment_1.tracking = 'none'
self.personal_equipment_request.accept_request()
allocation = self.personal_equipment_request.line_ids[0]
self.assertFalse(allocation.lot_ids)This will ensure that the module behaves correctly under various tracking configurations.
⚠️ PR Aging Alert: CRITICAL
This PR by @anusriNPS has been waiting for 223 days — that is over 7 months without being merged or closed.
Every ignored PR is a contributor who might not come back. Review time matters. (OCA Aging Report)
Reciprocal Review Request
Hi everyone! I found some test failures on this PR and left detailed feedback above. I am happy to discuss or help debug. In the meantime, if any of you get a chance, I would appreciate a look at my open PR(s):
My open PRs across OCA:
- server-tools#3554 [MIG] datetime_formatter: Migration to 18.0
- server-tools#3548 [18.0][MIG] base_kanban_stage: Migration to 18.0
- hr-attendance#262 [16.0][ADD] Hr_attendance_idsecure: iDSecure (ControliD) attendance integration
- stock-logistics-workflow#2276 [16.0][ADD] stock_move_line_devaluation
- stock-logistics-workflow#2275 [16.0][ADD] Stock move line analytic account
- stock-logistics-workflow#2268 [16.0][ADD] stock_move_line_picking_partner
- purchase-workflow#2694 [16.0][IMP]Purchase workflow added to review state & exception fix
Reviewing each other's work helps the whole community move forward. Thank you!
Environment via OCA Neural Reviewer: Minikube + K8s Job + oca-ci/py3.10-odoo16.0 | Odoo 16.0
Automated review by OCA Neural Reviewer + qwen3-coder:30b
alexey-pelykh
left a comment
alexey-pelykh
left a comment
There was a problem hiding this comment.
Re-reviewing after the update — good progress:
- Stray
hr_personal_equipment_requestfile — ✅ Removed from the PR. All changes now scoped tohr_personal_equipment_stock. - Tests for
lot_ids— ✅ Comprehensive coverage added: lot creation helper, lot assignment verification on delivery, lot clearing on cancel, partial delivery with lots. Well done. - Version bump — Still missing. OCA convention requires bumping the minor version for IMP changes (e.g.,
16.0.1.0.0→16.0.1.1.0). Can be done at merge time if needed.
All CI green. The functional change and tests are solid.
Co-Reviewed-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
|
This PR has the |
58a99fb to
7bf9eae
Compare
OCA-git-bot
added
series:16.0
mod:hr_personal_equipment_stock
|
Note: Using |
SirPyTech
left a comment
SirPyTech
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I reviewed the code and tried it locally, everything looks good 👏
|
@OCA/human-resources-maintainers this has more than enough reviews, can someone please have a look? Thanks! |
| lambda x: x.picking_id.state != "cancel" | ||
| and not x.origin_returned_move_id | ||
| ) | ||
| line.lot_ids = valid_move_ids.sorted("date", reverse=True)[:1].lot_ids |
There was a problem hiding this comment.
question: Why only the lots of the 1st ([:1]) move?
There was a problem hiding this comment.
When pickings are duplicated in a PPE request, lot_ids are modified for allocations. In order to show the latest lot_ids, using only the latest move lot_ids to display appropriate values under Serial Numbers for allocations in PPE form.
Note: Earlier, lot_ids was defined as related field to move_ids.lot_ids which does not update Serial Numbers field when lot_ids are modified for related moves available in pickings. Hence, lot_ids are now defined as a computed field so that lot_ids of recent move_ids are displayed when any changes handled as part of pickings of PPE request.
There was a problem hiding this comment.
I see, but I don't think getting the last one is the best approach: we should count how many lots are there in the target location instead.
Consider the following use-case:
- Three serial-tracked items are requested
- In the 1st picking: Only two of them are delivered because the 3rd one is not available yet
- In the 2nd picking: The 3rd item is delivered
With the current implementation only the 3rd serial is shown in the line, but all three should be shown instead.
Could you please check?
There was a problem hiding this comment.
Agree, the following use-case will not work with older solution. I have verified the above use case with latest solution and it works as expected. Please let me know your views on latest solution
OCA Boat with similar testings:
There was a problem hiding this comment.
Thanks for working on this, this piece of code has been removed so we can count this one as resolved.
| lambda x: x.picking_id.state != "cancel" | ||
| and not x.origin_returned_move_id | ||
| ) | ||
| line.lot_ids = valid_move_ids.sorted("date", reverse=True)[:1].lot_ids |
There was a problem hiding this comment.
I see, but I don't think getting the last one is the best approach: we should count how many lots are there in the target location instead.
Consider the following use-case:
- Three serial-tracked items are requested
- In the 1st picking: Only two of them are delivered because the 3rd one is not available yet
- In the 2nd picking: The 3rd item is delivered
With the current implementation only the 3rd serial is shown in the line, but all three should be shown instead.
Could you please check?
7bf9eae to
7c8310f
Compare
|
@SirPyTech Can you check this PR? If ready, we will merge |
@Saran440 So, I would like to wait to complete PR with new solution, before merging it. Thank you. |
7c8310f to
888c717
Compare
6a056c4 to
e8f8814
Compare
Display serial number under "Personal Equipment" Tab in Personal Equipment request form view when Personal Equipment Request is in "Accepted" state.
e8f8814 to
46f144e
Compare
Using proper values for qty_delivered based on stock availability when a picking associated with allocation is validated/returned/scrapped.
46f144e to
f1b7efa
Compare
| if line.move_ids.lot_ids: | ||
| available_lot_ids = ( | ||
| self._fetch_lot_ids_with_qty_available_from_dest_loc( | ||
| line.product_id, line.location_id | ||
| ) | ||
| ) | ||
| move_lot_ids = line.move_ids.mapped("lot_ids") | ||
| # When duplication/return of picking happens | ||
| # more than one move_ids are available | ||
| if len(move_lot_ids) > 1: | ||
| line.lot_ids = [ | ||
| ( | ||
| 6, | ||
| 0, | ||
| [ | ||
| lot_id.id | ||
| for lot_id in move_lot_ids | ||
| if available_lot_ids.get(lot_id.id, False) | ||
| ], | ||
| ) | ||
| ] |
There was a problem hiding this comment.
issue: _fetch_lot_ids_with_qty_available_from_dest_loc is searching all the lots that are present for line.product_id in line.location_id and that could be a lot of data, for sure a lot more than we need in this computation.
The product might have hundreds or more of serials/lots in a given location, and maybe none of them have been moved by line but with this code, all of them would be linked to the current line and that's not what we want, please fix it.
Note that some of the lots might have been moved by line.move_ids, but maybe some products already existed in line.location_id for the same lot.
In this method we only need to count how many lots have actually (i.e. removing returns) been moved to line.location_id by line.move_ids.
I suggest a simple for loop over the moves of line.move_ids that counts how much is moved from/to line.location_id for each lot; after the loop is done the lots that have some quantity go into line.lot_ids, the others don't.
| elif len(move_ids) > 1 and ( | ||
| returned_move_ids and not scrapped_move_ids | ||
| ): | ||
| line.qty_delivered = move_ids[-1].quantity_done |
There was a problem hiding this comment.
issue: This and the following [-1]s smell for the reasons I already commented in #1497 (comment): for instance, if the the last move is a return then the qty_delivered would be the returned quantity.
Please consider a loop logic as suggested for lot_ids.
| lot_ids_qty_info = { | ||
| lot_id.id: self.env["stock.quant"]._get_available_quantity( | ||
| line.product_id, | ||
| line.location_id, | ||
| lot_id, | ||
| ) | ||
| for lot_id in line.lot_ids | ||
| } | ||
| moves_to_be_checked = line.move_ids.filtered( | ||
| lambda r: r.state == "done" | ||
| and line.product_id == r.product_id | ||
| and not r.origin_returned_move_id | ||
| and r.lot_ids not in scrapped_lot_ids | ||
| ) | ||
| for move in moves_to_be_checked: | ||
| qty += sum([lot_ids_qty_info.get(lot.id, 0) for lot in move.lot_ids]) |
There was a problem hiding this comment.
issue: Some of this available quantity may not have been moved to location_id by line.move_ids.
| lot_infos = self.env["stock.quant"].read_group( | ||
| domain=[ | ||
| ("product_id", "=", product_id.id), | ||
| ("location_id", "child_of", location_id.id), | ||
| ], | ||
| fields=["lot_id", "quantity: sum"], | ||
| groupby=["lot_id"], | ||
| ) | ||
| available_lot_ids = { | ||
| lot_info["lot_id"][0]: lot_info["quantity"] | ||
| for lot_info in lot_infos | ||
| if lot_info["lot_id"] and lot_info["quantity"] not in (0.0, -1.0) | ||
| } | ||
| return available_lot_ids |
There was a problem hiding this comment.
praise: It's always nice to get data using read_group and a good old dictionary 🙂
| lambda x: x.picking_id.state != "cancel" | ||
| and not x.origin_returned_move_id | ||
| ) | ||
| line.lot_ids = valid_move_ids.sorted("date", reverse=True)[:1].lot_ids |
There was a problem hiding this comment.
Thanks for working on this, this piece of code has been removed so we can count this one as resolved.
10 participants
Display serial number under "Personal Equipment" Tab
in Personal Equipment request form view when Personal Equipment Request is in "Accepted" state.
lot_idsas computed field which helps to update serial number even when pickings are duplicated.qty_deliveredfield to be uptodate based on stock availability in PPE location which helps to avoid wrong field updation when an allocation is returned from pickings or pickings are duplicated.