Repository navigation
Ezra/309 unapprove distributions - #312
Conversation
✅ Deploy Preview for hope-for-haiti ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Greptile SummaryThis PR adds the ability to unapprove (revert to pending) a previously approved distribution, complementing the existing approve action. On the frontend a conditional Approve/Unapprove button is rendered based on Key observations:
Confidence Score: 2/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async function unapproveDistribution() { | ||
| const formData = new FormData(); | ||
| formData.append("pending", "true"); | ||
|
|
||
| const promise = apiClient.patch(`/api/distributions/${distribution.id}`, { | ||
| body: formData, | ||
| }); | ||
|
|
||
| toast.promise(promise, { | ||
| loading: "Unapproving distribution...", | ||
| success: "Distribution unapproved!", | ||
| error: "Failed to unapprove distribution.", | ||
| }); | ||
|
|
||
| await promise; | ||
|
|
||
| setIsDropdownOpen(false); | ||
| fetchTableData(); | ||
| } |
There was a problem hiding this comment.
If await promise rejects (e.g. the server returns an error), the lines after it — setIsDropdownOpen(false) and fetchTableData() — will never run. The dropdown will stay open even after the error toast is shown. The same pattern exists in approveDistribution. Using a try/finally block ensures cleanup always happens:
| async function unapproveDistribution() { | |
| const formData = new FormData(); | |
| formData.append("pending", "true"); | |
| const promise = apiClient.patch(`/api/distributions/${distribution.id}`, { | |
| body: formData, | |
| }); | |
| toast.promise(promise, { | |
| loading: "Unapproving distribution...", | |
| success: "Distribution unapproved!", | |
| error: "Failed to unapprove distribution.", | |
| }); | |
| await promise; | |
| setIsDropdownOpen(false); | |
| fetchTableData(); | |
| } | |
| async function unapproveDistribution() { | |
| const formData = new FormData(); | |
| formData.append("pending", "true"); | |
| const promise = apiClient.patch(`/api/distributions/${distribution.id}`, { | |
| body: formData, | |
| }); | |
| toast.promise(promise, { | |
| loading: "Unapproving distribution...", | |
| success: "Distribution unapproved!", | |
| error: "Failed to unapprove distribution.", | |
| }); | |
| try { | |
| await promise; | |
| fetchTableData(); | |
| } finally { | |
| setIsDropdownOpen(false); | |
| } | |
| } |
The same fix should be applied to approveDistribution (lines 157–175).
There was a problem hiding this comment.
commited these changes for both approval and unapproval
There was a problem hiding this comment.
The fix looks good! Both approveDistribution and unapproveDistribution now properly use try/finally blocks. The dropdown will close even if the API call fails, which resolves the issue.
The implementation is correct - fetchTableData() stays inside the try block so it only runs on success, which is the right behavior.
| if (allocation.signOffId !== null) { | ||
| throw new ArgumentError( | ||
| "Cannot transfer items that have been signed off." | ||
| ); | ||
| } |
There was a problem hiding this comment.
The new guard correctly prevents transferring signed-off allocations out of a distribution. However, updateDistribution in distributionService.ts has no corresponding check — an admin can successfully unapprove a distribution that already contains signed-off allocations.
After unapproving such a distribution:
- Partners can no longer see those allocations (because
getPartnerAllocationsfilters ondistribution.pending === false). - The allocations cannot be transferred elsewhere (blocked by this new check).
- There is no API path to clear the
signOffId, so the allocations are effectively orphaned.
To avoid this stuck state, updateDistribution should validate that no allocations have a non-null signOffId before allowing the transition back to pending:
// In distributionService.ts -> updateDistribution, before db.distribution.update()
if (data.pending === true && currentDistribution.pending === false) {
const signedOffCount = await db.allocation.count({
where: { distributionId, signOffId: { not: null } },
});
if (signedOffCount > 0) {
throw new ArgumentError(
"Cannot unapprove a distribution that has signed-off allocations."
);
}
}There was a problem hiding this comment.
commited these changes as well
There was a problem hiding this comment.
Perfect! The fix is implemented correctly in distributionService.ts. The validation check at lines 670-679 prevents unapproving distributions that contain signed-off allocations, which closes the stuck-state gap.
The implementation:
- ✅ Checks the state transition (approved → pending)
- ✅ Counts allocations with non-null
signOffId - ✅ Throws a clear error message if any exist
Combined with the transfer guard in allocationService.ts, signed-off allocations are now properly protected from both operations that would orphan them.
One minor style note: line 672 has inconsistent spacing in { not: null} - should be { not: null } for consistency, but this doesn't affect functionality.
The PR is solid!
ND68
left a comment
ND68
left a comment
There was a problem hiding this comment.
code looks good, thanks for handling the edge cases, nice work
2 participants
Description
Resolves ticket number: #309
Explain what your code changes:
DistributionTable.tsx - added unapprove function and conditional button rendering
allocationService.ts - signOffId validation in updateAllocationBatch
List the steps you took to test your code:
Tested the unapprove button and approve button within the distributions table, making sure the conditional button is correct, making sure the status changes, and the correct toast notification pops up
Also handling edge case where the shipments are already signed off, if they are signed off, transfers cannot occur. If this is something we should add, I can definitely look into that.
Verified within the prisma database that transferred allocation's are correctly updating(distributionId and partnerId columns)
Also logged into the partner account to confirm that the transferred items appear in their distributions view after approval
Checklist