OBPIH-7588 Receiving: Split line causes quantities to move down one cell below#5644
OBPIH-7588 Receiving: Split line causes quantities to move down one cell below#5644alannadolny merged 2 commits intorelease/0.9.6-hotfix1from
Conversation
alannadolny
commented
Nov 27, 2025
alannadolny
commented
…ove edit functionality
alannadolny
requested review from
SebastianLib,
awalkowiak,
ewaterman and
kchelstowski
github-actions
bot
added
type: bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/0.9.6-hotfix1 #5644 +/- ##
=======================================================
Coverage ? 8.51%
Complexity ? 1127
=======================================================
Files ? 712
Lines ? 45648
Branches ? 10911
=======================================================
Hits ? 3887
Misses ? 41182
Partials ? 579 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| return formValues.containers.map((container, idx) => ({ | ||
| // If this is the edited line, replace quantity shipped, so we can avoid manipulating with | ||
| // fetched data, because the form data is not constantly consistent with the fetched data | ||
| // due to user changes |
There was a problem hiding this comment.
Can you explain this a bit more for me please? How does this fix the issue?
There was a problem hiding this comment.
Basically, there were more than one issue that is in the title. There were:
- issue with shifting qty down
- issue with disappearing lines after splitting line (after 2nd or 3rd try)
- issue with disappearing product info,
etc.
So I decided to write this function from scratch, because every time I fixed one issue, I saw another one...
Now it's mapping the form values, so that we are:
- cleaning receiving now cells for edited lines
- applying the new quantity shipped after using the split line modal
Basically, this workflow is badly designed, the split lines are not saved, nothing happens on the backend, the whole responsibility is on the frontend, so every calculation, data manipulation is here...
We should keep the frontend as a presentation layer, so the backend should calculate everything and return data that is ready to display, but that workflow is doing the opposite, since it was implemented
There was a problem hiding this comment.
Pull request overview
This PR refactors the rewriteQuantitiesAfterSave function in the receiving flow to fix a bug where splitting a line causes quantities to appear in the wrong cells. The refactoring simplifies the logic by removing the complex zipping of form values with fetched data and instead directly manipulating the form values.
Key Changes:
- Simplified the
rewriteQuantitiesAfterSavefunction to work directly with form values instead of merging with fetched container data - Removed the
fetchedContainersparameter from the function signature - Changed from processing all edited lines to using only the first edited line (
editLines[0])
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const rewriteQuantitiesAfterSave = ({ | ||
| formValues, | ||
| editLines, | ||
| fetchedContainers, | ||
| editLinesIndex, | ||
| parentIndex, | ||
| }) => { |
There was a problem hiding this comment.
The function parameter fetchedContainers was removed, but the call site at line 893 still passes this parameter. This means the fetched data from the server response will be ignored, potentially causing data inconsistencies. The parameter should either be kept in the function signature or removed from the call site.
| const flattenedFormValues = _.flatten( | ||
| formValues.containers.map((container) => container.shipmentItems), | ||
| ); | ||
| const editedLine = editLines[0]; |
There was a problem hiding this comment.
The code assumes that editLines[0] is the item that was saved to the API (the one with receiptItemId). However, the order of items in the editLines array is not guaranteed. When a line is split, the array could have the new items first and the original item last, or vice versa. Consider using editLines.find(line => line.receiptItemId) to explicitly find the saved item instead of assuming it's at index 0.
| const editedLine = editLines[0]; | |
| const editedLine = editLines.find(line => line.receiptItemId); |
| const groupedItems = _.reduce(containerSizes, (acc, size) => { | ||
| const start = _.sum(acc.map((x) => x.length)); | ||
| return [...acc, newFormLines.slice(start, start + size)]; | ||
| }, []); |
There was a problem hiding this comment.
The _.sum(acc.map((x) => x.length)) operation is recalculated for each container in the reduce function, resulting in O(n²) complexity where n is the number of containers. For better performance, consider maintaining a running sum variable. Example:
const groupedItems = _.reduce(containerSizes, (acc, size, index) => {
const start = index === 0 ? 0 : _.sum(containerSizes.slice(0, index));
return [...acc, newFormLines.slice(start, start + size)];
}, []);Or use a more efficient approach with a running index variable.
| const groupedItems = _.reduce(containerSizes, (acc, size) => { | |
| const start = _.sum(acc.map((x) => x.length)); | |
| return [...acc, newFormLines.slice(start, start + size)]; | |
| }, []); | |
| // Efficiently regroup edited flattened items back to containers using a running index | |
| const groupedItems = []; | |
| let start = 0; | |
| for (let i = 0; i < containerSizes.length; i++) { | |
| const size = containerSizes[i]; | |
| groupedItems.push(newFormLines.slice(start, start + size)); | |
| start += size; | |
| } |
…ceivingPage and clean up container fetching logic
