Skip to content

Ezra/309 unapprove distributions #312

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.

Sign up for GitHub

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

Merged
ND68 merged 4 commits into from
Apr 2, 2026
Merged

Ezra/309 unapprove distributions #312

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
69d6a0a
added unapprove backend logic and conditional button rendering for ap…
ezraexe Mar 17, 2026
ac36cf3
added unapprove backend logic and conditional button rendering for ap…
ezraexe Mar 17, 2026
7462a26
added greptile change
ezraexe Mar 17, 2026
587b6bd
adding more greptile suggestions to prevent unapproval of signed off …
ezraexe Mar 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 39 additions & 8 deletions src/components/DistributionTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { useCallback, useRef, useState } from "react";
import { useApiClient } from "@/hooks/useApiClient";
import Portal from "./baseTable/Portal";
import toast from "react-hot-toast";
import { CheckCircle, DotsThree } from "@phosphor-icons/react";
import { CheckCircle, DotsThree, XCircle } from "@phosphor-icons/react";
import DistributionsGeneralItemChipGroup from "./chips/DistributionsGeneralItemChipGroup";
import { TableDistribution } from "@/types/api/distribution.types";
import { useUser } from "@/components/context/UserContext";
Expand Down Expand Up @@ -173,6 +173,26 @@ function OptionsButton({
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.",
});

await promise;

setIsDropdownOpen(false);
fetchTableData();
}
Comment on lines +180 to +200

@greptile-apps greptile-apps Bot Mar 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Dropdown stays open on API error

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:

Suggested change
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).

@ezraexe ezraexe Mar 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commited these changes for both approval and unapproval

@ezraexe ezraexe Mar 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greptile review

@greptile-apps greptile-apps Bot Mar 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return (
<div className="relative">
Expand All @@ -190,13 +210,24 @@ function OptionsButton({
position="bottom-right"
className="w-48 rounded-md bg-white shadow-lg ring-1 ring-black/5 py-1"
>
<button
onClick={approveDistribution}
className="flex items-center w-full px-4 py-2 text-sm text-left transition-colors duration-150 hover:bg-gray-50 text-gray-900 cursor-pointer"
>
<CheckCircle size={16} className="mr-3 flex-shrink-0" />
<p>Approve</p>
</button>
{distribution.pending ? (
<button
onClick={approveDistribution}
className="flex items-center w-full px-4 py-2 text-sm text-left transition-colors duration-150 hover:bg-gray-50 text-gray-900 cursor-pointer"
>
<CheckCircle size={16} className="mr-3 flex-shrink-0" />
<p>Approve</p>
</button>
) : (
<button
onClick={unapproveDistribution}
className="flex items-center w-full px-4 py-2 text-sm text-left transition-colors duration-150 hover:bg-gray-50 text-gray-900 cursor-pointer"
>
<XCircle size={16} className="mr-3 flex-shrink-0" />
<p>Unapprove</p>
</button>

)}
</Portal>
</div>
);
Expand Down
5 changes: 5 additions & 0 deletions src/services/allocationService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,11 @@ export default class AllocationService {
"Cannot transfer items from an approved distribution. Approved distributions are locked."
);
}
if (allocation.signOffId !== null) {
throw new ArgumentError(
"Cannot transfer items that have been signed off."
);
}
Comment on lines +320 to +324

@greptile-apps greptile-apps Bot Mar 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Signed-off allocations in pending distributions become permanently stuck

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:

  1. Partners can no longer see those allocations (because getPartnerAllocations filters on distribution.pending === false).
  2. The allocations cannot be transferred elsewhere (blocked by this new check).
  3. 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."
    );
  }
}

@ezraexe ezraexe Mar 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commited these changes as well

@ezraexe ezraexe Mar 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greptile review

@greptile-apps greptile-apps Bot Mar 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

}

if (update.distributionId) {
Expand Down
Loading