Repository navigation
Finalized Donor Offer Allocation - #189
Conversation
❌ Deploy Preview for hope-for-haiti failed. Why did it fail? →
|
| } | ||
|
|
||
| if (session.user.type === "PARTNER") { | ||
| return <p />; |
There was a problem hiding this comment.
| return <p />; | |
| return </>; |
There was a problem hiding this comment.
not sure why, but during interaction mode it does not let me change allocations. this should not be the case and the user should have the ability to change them during that mode too.
There was a problem hiding this comment.
This is not what was asked. When I hover over a chip during interaction mode, it does not let me open the popup and change my allocation. It works normally.
There was a problem hiding this comment.
if the name is too long it separates into two lines, which makes it look really wonky. I think a fix to this is making the partner name truncate into ellipsis when it is longer than the width of the outer chip
There was a problem hiding this comment.
| className="w-80 bg-white border border-gray-primary/20 rounded shadow-lg p-2 text-sm font-bold" |
| throw new ArgumentError(parsed.error.message); | ||
| } | ||
|
|
||
| const result = await getDonorOfferAllocationItems( |
There was a problem hiding this comment.
This function should be in the donorOfferService class
| <span className="flex items-center gap-1 px-1 py-[1px] whitespace-nowrap"> | ||
| {hasSuggestedChange && ( | ||
| <span className="flex items-center gap-1 px-1 py-[1px] rounded bg-gray-primary/10 text-gray-primary/60 line-through"> | ||
| {previousPartnerLabel} | ||
| </span> | ||
| )} | ||
| <span | ||
| className={`flex items-center gap-1 px-1 py-[1px] rounded whitespace-nowrap ${ | ||
| hasSuggestedChange | ||
| ? "bg-blue-primary/20 text-blue-primary font-semibold" | ||
| : item.allocation | ||
| ? "bg-blue-primary/10 text-blue-primary" | ||
| : "bg-gray-primary/10 text-gray-primary/50" | ||
| }`} | ||
| > | ||
| {hasSuggestedRemoval ? "None" : currentPartnerLabel} | ||
| </span> |
There was a problem hiding this comment.
this change also fundamentally changes the styling of the chips which was not asked for. revert it back to the previous styling. The class name of the span directly on the partner name should have the class truncate with a max with parameter of 100 pixels.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Disclaimer: This comment was AI-generated and is not necessarily completely accurate. Please take code comments with a grain of salt.
Greptile Overview
Greptile Summary
This PR implements a finalized donor offer allocation workflow that enables staff to allocate line items from completed donor offers to partner distributions using AI-suggested allocations. The implementation adds a new allocation screen at /donorOffers/[donorOfferId]/allocate with an interaction mode where users can preview, manually adjust, cancel, undo, or keep AI-generated allocation suggestions before batch-saving them to the database. The changes introduce new API endpoints for fetching allocation-ready items, creating batch allocations, retrieving distributions by donor offer, and submitting (archiving) finalized donor offers. A reusable AllocationTable component was created to generalize allocation workflows across both the new donor offer allocation screen and the existing unallocated items screen. The routing logic in AdminDonorOffersScreen now directs users to the allocation interface when clicking finalized donor offers. The allocation suggestion algorithm was refactored to accept a donor offer ID instead of raw data structures, centralizing data loading in the backend. Distribution management ensures allocations are properly linked to partner-specific distributions, with state tracking to prevent duplicates. The implementation includes visual indicators for suggested allocations (strikethrough previous partners, highlighted new partners) and supports both streaming preview mode and immediate persistence mode for line item allocations.
PR Description Notes:
- The PR description does not provide details about the specific changes or implementation approach, only references issue #184 and a checklist
Important Files Changed
Changed Files
| Filename | Score | Overview |
|---|---|---|
| src/screens/DonorOffersScreens/PartnerAllocateDonorOfferScreen.tsx | 1/5 | File has been completely emptied (only contains blank line), breaking any imports or routes that depend on this screen component |
| src/screens/DonorOffersScreens/AdminAllocateDonorOfferScreen.tsx | 4/5 | Implements new allocation screen with AI suggestions, interaction mode, batch operations, and complex state management for rollback/diff tracking |
| src/components/baseTable/AllocationTable.tsx | 4/5 | Creates reusable allocation table component wrapping AdvancedBaseTable with enrichment, ref forwarding, and interaction mode support |
| src/components/LineItemChipGroup.tsx | 3/5 | Adds interaction mode to line item chips with preview, manual allocation, distribution management, and visual indicators for suggestions |
| src/screens/AdminUnallocatedItemsScreen.tsx | 2/5 | Adds suggestedAllocation field but introduces critical pagination bug (returns items.length instead of res.total) |
| src/services/distributionService.ts | 2/5 | Adds getDistributionsForDonorOffer but reduce operation overwrites multiple distributions per partner, potentially losing data |
| src/services/allocationService.ts | 2/5 | Adds createBatchAllocations with potential transaction atomicity issues and missing duplicate lineItemId validation |
| src/services/donorOfferService.ts | 3/5 | Adds getAllocationItems without pagination, may load all allocations indiscriminately mixing unrelated data |
| src/app/api/suggest/finalized/route.ts | 3.5/5 | Refactors suggestion endpoint to accept donor offer ID, centralizes data loading, but silently swallows JSON parsing errors |
| src/screens/DonorOffersScreens/AdminDonorOffersScreen.tsx | 3/5 | Modifies click routing to direct finalized offers to /allocate page, minimal change with unclear ARCHIVED offer handling |
| src/app/donorOffers/[donorOfferId]/allocate/page.tsx | 3/5 | Creates new allocation route with role-based rendering but doesn't handle session loading state and returns empty fragment for partners |
| src/app/api/donorOffers/[donorOfferId]/allocationItems/route.ts | 4/5 | Adds GET endpoint for allocation items with proper validation and staff-only authorization |
| src/app/api/donorOffers/[donorOfferId]/distributions/route.ts | 4/5 | Adds GET endpoint to retrieve distributions for donor offer with proper validation and authorization |
| src/app/api/distributions/[distributionId]/allocations/batch/route.ts | 3/5 | Creates batch allocation endpoint with redundant distribution fetch and potential partnerId constraint issues |
| src/app/api/donorOffers/[donorOfferId]/submit/route.ts | 3/5 | Adds submission endpoint to archive donor offers without validating current state or allocation completion |
Confidence score: 1/5
- This PR has a critical blocking issue that will break production code and must be fixed before merging
- Score reflects the completely emptied PartnerAllocateDonorOfferScreen.tsx file (line 1 is blank), which will cause immediate import failures and break the partner allocation route. Additionally, the pagination bug in AdminUnallocatedItemsScreen.tsx (returning items.length instead of res.total) will break multi-page navigation, the distribution reducer overwrites multiple distributions per partner potentially losing data, and the batch allocation transaction implementation may not provide atomicity guarantees. The button styling in AdminAllocateDonorOfferScreen.tsx violates the custom color rule by mixing config colors with hardcoded hover states.
- The PartnerAllocateDonorOfferScreen.tsx file requires immediate attention and restoration, AdminUnallocatedItemsScreen.tsx pagination logic needs correction (line 107), DistributionService.getDistributionsForDonorOffer reducer logic should handle multiple distributions per partner (lines 59-67), AllocationService.createBatchAllocations transaction pattern should be reviewed for atomicity (lines 106-124), and AdminAllocateDonorOfferScreen.tsx button styling should use tailwind config colors consistently (lines 813-830)
PR Checklist Feedback
Score: 72/100
Areas that need improvement:
General
- Console.logs or commented code may be present in AdminAllocateDonorOfferScreen.tsx (extensive logging for debugging interaction mode)
Frontend
- Hardcoded colors/sizes detected: AdminAllocateDonorOfferScreen.tsx uses hover:bg-blue-600 and hover:bg-green-600 instead of tailwind config hover states (lines 813-830)
- Error handling for API calls needs improvement: AllocationTable.tsx has no error handling for failed fetches or distribution creation callbacks
- Component modularization: LineItemChipGroup.tsx contains redundant
AllocationResponsetype definition that should be in a shared types file
Backend
- Schemas need to be more strict: finalizedAutomatedSuggestionSchema silently catches JSON parsing errors with
.catch(() => ({}))on line 34 - Edge case handling gaps: submit endpoint doesn't validate donor offer state before archiving; batch allocation endpoint doesn't validate distribution ownership consistency
- Operations not optimized: getAllocationItems lacks pagination; sequential batching in AdminAllocateDonorOfferScreen could be parallelized
Sequence Diagram
sequenceDiagram
participant User
participant AdminAllocateDonorOfferScreen
participant API
participant DonorOfferService
participant AllocationService
participant DistributionService
participant Database
User->>AdminAllocateDonorOfferScreen: Navigate to /donorOffers/{id}/allocate
AdminAllocateDonorOfferScreen->>API: GET /api/donorOffers/{id}/allocationItems
API->>DonorOfferService: getAllocationItems(donorOfferId)
DonorOfferService->>Database: Query generalItems with line items, allocations, requests
Database-->>DonorOfferService: Return items data
DonorOfferService-->>API: Return allocation items
API-->>AdminAllocateDonorOfferScreen: Display items with line item chips
alt User clicks "Suggest Allocations"
User->>AdminAllocateDonorOfferScreen: Click "Suggest Allocations" button
AdminAllocateDonorOfferScreen->>API: POST /api/suggest/finalized
API->>DonorOfferService: getAdminDonorOfferDetails(donorOfferId)
DonorOfferService->>Database: Query donor offer items and requests
Database-->>DonorOfferService: Return items with requests
DonorOfferService-->>API: Return donor offer details
API->>API: Run HiGHS ILP solver to optimize allocations
API-->>AdminAllocateDonorOfferScreen: Return suggested allocations
AdminAllocateDonorOfferScreen->>AdminAllocateDonorOfferScreen: Display suggestions with preview (interaction mode)
end
alt User manually adjusts allocations
User->>AdminAllocateDonorOfferScreen: Click line item chip to change allocation
AdminAllocateDonorOfferScreen->>AdminAllocateDonorOfferScreen: Update suggestedAllocation state
AdminAllocateDonorOfferScreen->>AdminAllocateDonorOfferScreen: Track manual changes in manualChanges map
end
alt User clicks "Keep" to apply suggestions
User->>AdminAllocateDonorOfferScreen: Click "Keep" button
loop For each pending change
alt Removal needed
AdminAllocateDonorOfferScreen->>API: DELETE /api/allocations/{allocationId}
API->>AllocationService: deleteAllocation(id)
AllocationService->>Database: Delete allocation
Database-->>AllocationService: Confirm deletion
AllocationService-->>API: Return deletion result
API-->>AdminAllocateDonorOfferScreen: Return deletedDistribution flag
end
alt Addition needed
AdminAllocateDonorOfferScreen->>API: POST /api/distributions (if needed)
API->>DistributionService: createDistribution(data)
DistributionService->>Database: Create distribution
Database-->>DistributionService: Return distribution
DistributionService-->>API: Return distribution
API-->>AdminAllocateDonorOfferScreen: Return distribution ID
end
end
AdminAllocateDonorOfferScreen->>API: POST /api/distributions/{id}/allocations/batch
API->>AllocationService: createBatchAllocations(distributionId, allocations)
AllocationService->>Database: Create multiple allocations in transaction
Database-->>AllocationService: Return created allocations
AllocationService-->>API: Return allocations
API-->>AdminAllocateDonorOfferScreen: Confirm batch creation
AdminAllocateDonorOfferScreen->>AdminAllocateDonorOfferScreen: Exit interaction mode, reload table
end
alt User clicks "Submit Donor Offer"
User->>AdminAllocateDonorOfferScreen: Click "Submit Donor Offer" button
AdminAllocateDonorOfferScreen->>API: POST /api/donorOffers/{id}/submit
API->>DonorOfferService: updateDonorOffer(donorOfferId, state: ARCHIVED)
DonorOfferService->>Database: Update donor offer state
Database-->>DonorOfferService: Confirm update
DonorOfferService-->>API: Return success
API-->>AdminAllocateDonorOfferScreen: Return confirmation
AdminAllocateDonorOfferScreen->>AdminAllocateDonorOfferScreen: Redirect to /donorOffers
end
Additional Comments (2)
-
src/screens/AdminUnallocatedItemsScreen.tsx, line 105-108 (link)logic: Returning
items.lengthas total instead ofres.totalwill break pagination. The API returns the correct total count for all items matching filters, but this returns only the current page's item count. -
src/app/api/suggest/finalized/route.ts, line 192-194 (link)style: When the solver doesn't find an optimal solution, an InternalError is thrown. This doesn't distinguish between infeasible problems (e.g., impossible request constraints) and solver failures. Consider providing more specific error messages.
Context used:
- Rule from
dashboard- Use colors from the tailwind config for toolbar buttons and text instead of custom colors. The tailw... (source)
15 files reviewed, 27 comments
| const { data: session } = useSession(); | ||
|
|
||
| if (!session?.user.type) { | ||
| redirect("/signIn"); | ||
| } | ||
|
|
||
| if (isStaff(session.user.type)) { | ||
| return <AdminAllocateDonorOfferScreen />; | ||
| } | ||
|
|
||
| if (session.user.type === "PARTNER") { | ||
| return <></>; | ||
| } | ||
|
|
||
| redirect("/signIn"); |
There was a problem hiding this comment.
logic: Missing loading state handling while session data is being fetched. The redirect calls may execute before session is fully loaded, potentially causing flashing or incorrect redirects.
| </div>, | ||
| ], | ||
| onClick: () => router.push(`/donorOffers/${offer.donorOfferId}`), | ||
| onClick: () => router.push(`/donorOffers/${offer.donorOfferId}${offer.state === DonorOfferState.FINALIZED ? "/allocate" : ""}`) |
There was a problem hiding this comment.
logic: The conditional routing appends /allocate when clicking on FINALIZED offers but does nothing for ARCHIVED offers. Based on the PR requirements, archived offers should also route to /allocate (or be non-clickable), since finalized offers become archived after submission. Should ARCHIVED offers route to /allocate as well, or should archived offers be non-clickable?
| export async function GET( | ||
| _: NextRequest, | ||
| { params }: { params: Promise<{ donorOfferId: string }> } | ||
| ) { |
There was a problem hiding this comment.
style: The first parameter is destructured but unused. Consider using a more explicit name or removing it entirely:
| export async function GET( | |
| _: NextRequest, | |
| { params }: { params: Promise<{ donorOfferId: string }> } | |
| ) { | |
| export async function GET( | |
| _req: NextRequest, | |
| { params }: { params: Promise<{ donorOfferId: string }> } | |
| ) { |
| const { donorOfferId } = await params; | ||
| const parsed = paramSchema.safeParse({ donorOfferId }); | ||
| if (!parsed.success) { | ||
| throw new ArgumentError(parsed.error.message); | ||
| } |
There was a problem hiding this comment.
style: Validation occurs after awaiting params. Consider validating before the await to fail fast:
| const { donorOfferId } = await params; | |
| const parsed = paramSchema.safeParse({ donorOfferId }); | |
| if (!parsed.success) { | |
| throw new ArgumentError(parsed.error.message); | |
| } | |
| const rawParams = await params; | |
| const parsed = paramSchema.safeParse({ donorOfferId: rawParams.donorOfferId }); | |
| if (!parsed.success) { | |
| throw new ArgumentError(parsed.error.message); | |
| } |
| const distribution = await DistributionService.getDistribution( | ||
| parsedParams.data.distributionId | ||
| ); |
There was a problem hiding this comment.
style: The distribution is fetched but not used except for passing its ID to createBatchAllocations, which already receives the ID from route params. This fetch adds unnecessary latency and a potential failure point. Is this fetch intended to validate that the distribution exists before creating allocations?
| for (const suggestion of pendingChanges) { | ||
| const baselineLine = baselineLineMap.get(suggestion.lineItemId); | ||
| const baselinePartnerId = | ||
| baselineLine?.allocation?.partner?.id ?? null; | ||
| const nextPartnerId = suggestion.partnerId; | ||
|
|
||
| if (baselinePartnerId === nextPartnerId) { | ||
| continue; | ||
| } | ||
|
|
||
| if (baselineLine?.allocation?.id && baselinePartnerId !== null) { | ||
| removals.push({ | ||
| lineItemId: suggestion.lineItemId, | ||
| partnerId: baselinePartnerId, | ||
| }); | ||
| } | ||
|
|
||
| if (nextPartnerId === null) { | ||
| appliedCount += 1; | ||
| continue; | ||
| } | ||
|
|
||
| const partnerName = | ||
| suggestion.partnerName ?? `Partner ${nextPartnerId}`; | ||
|
|
||
| const distribution = await ensureDistributionForPartner( | ||
| nextPartnerId, | ||
| partnerName | ||
| ); | ||
|
|
||
| const entry = additionsByDistribution.get(distribution.id) ?? []; | ||
| entry.push({ | ||
| partnerId: nextPartnerId, | ||
| lineItemId: suggestion.lineItemId, | ||
| }); | ||
| additionsByDistribution.set(distribution.id, entry); | ||
| } |
There was a problem hiding this comment.
style: batching allocations by distribution is sequential and may be slow. Consider using Promise.all on independent distribution batch calls to parallelize API requests.
| for (const removal of removals) { | ||
| const baselineLine = baselineLineMap.get(removal.lineItemId); | ||
| const allocationId = baselineLine?.allocation?.id; | ||
| if (!allocationId) { | ||
| continue; | ||
| } | ||
|
|
||
| const response = await apiClient.delete<{ | ||
| deletedDistribution: boolean; | ||
| }>(`/api/allocations/${allocationId}`); | ||
|
|
||
| if (response.deletedDistribution && baselineLine?.allocation?.partner) { | ||
| handleDistributionRemoved(baselineLine.allocation.partner.id); | ||
| } | ||
|
|
||
| appliedCount += 1; | ||
| } |
There was a problem hiding this comment.
style: deleting allocations sequentially may be slow when there are many removals. Consider batching delete requests or using parallel execution with Promise.all.
| for (const [distributionId, allocations] of additionsByDistribution) { | ||
| if (!allocations.length) { | ||
| continue; | ||
| } | ||
|
|
||
| await apiClient.post( | ||
| `/api/distributions/${distributionId}/allocations/batch`, | ||
| { | ||
| body: JSON.stringify({ allocations }), | ||
| } | ||
| ); | ||
|
|
||
| appliedCount += allocations.length; | ||
| } |
There was a problem hiding this comment.
style: sequential batch post for each distribution may be slow. Consider parallelizing with Promise.all since distributions are independent.
| className="px-4 py-2 bg-blue-primary text-white rounded hover:bg-blue-600 disabled:opacity-60 disabled:cursor-not-allowed" | ||
| disabled={isProcessingSuggestions} | ||
| > | ||
| {isProcessingSuggestions | ||
| ? "Preparing suggestions..." | ||
| : "Suggest Allocations"} | ||
| </button> |
There was a problem hiding this comment.
style: button uses bg-blue-primary and hover:bg-blue-600 which mixes custom and hardcoded colors
Context Used: Rule from dashboard - Use colors from the tailwind config for toolbar buttons and text instead of custom colors. The tailw... (source)
| className="px-3 py-2 bg-green-primary text-white rounded hover:bg-green-600 disabled:opacity-60 disabled:cursor-not-allowed" | ||
| disabled={ | ||
| isProcessingSuggestions || | ||
| manualChanges.size > 0 || | ||
| isSubmitting | ||
| } | ||
| > | ||
| {isSubmitting ? "Submitting..." : "Submit Donor Offer"} | ||
| </button> |
There was a problem hiding this comment.
style: button uses bg-green-primary and hover:bg-green-600 which mixes custom and hardcoded colors
Context Used: Rule from dashboard - Use colors from the tailwind config for toolbar buttons and text instead of custom colors. The tailw... (source)
2 participants
Description
Resolves #184
Checklist