fix: adds revoke and regrant logic for cases when suggestions get OUTDATED#2035
fix: adds revoke and regrant logic for cases when suggestions get OUTDATED#2035
Conversation
|
This PR will trigger a minor release when merged. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Replace N+1 Suggestion.findById calls with single batchGetByKeys batch lookup. Remove redundant freshNewIds variable reusing newSuggestionIds. Add clarifying comment on grantIds uniqueness contract. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Hi @sandsinh , they are few comments on the approach Existing token path: revokes ALL grants when ANY is stale — grant-suggestions-handler.js:~222-241 When one suggestion is OUTDATED/REJECTED, the code revokes all grants for that token (including APPROVED ones), then relies on the subsequent splitSuggestionsByGrantStatus + getTopSuggestions to re-grant the non-stale ones. But the re-grant block only considers suggestions from newSuggestions (fetched via allByOpportunityIdAndStatus(opptyId, STATUSES.NEW)). If a previously granted suggestion has been APPROVED, it won't be in the NEW list, so it won't be re-granted — its slot is permanently lost until the next cycle. Is this intentional? If not, the "existing token" path should either: Only revoke the stale grants (not all grants), or Same issue: grantedEntities is filtered from newSuggestions (status=NEW), but previously granted suggestions may have moved to APPROVED status. Those won't appear in newSuggestions, so grantedEntities will be empty for them, and their re-grant is silently skipped. No error handling on parallel revokes — grant-suggestions-handler.js:~163 revokeGrants fires all revocations via Promise.all. If one fails, the entire operation rejects and remaining revokes are lost. Consider Promise.allSettled or sequential processing, since a partial revoke leaves the system in an inconsistent state (some grants revoked, token count partially decremented). Double splitSuggestionsByGrantStatus call on every path — grant-suggestions-handler.js:~253-254 After the revoke/re-grant logic, the function always calls splitSuggestionsByGrantStatus again. On the happy path (no stale grants, existing token), this is an unnecessary extra DB call. Consider only re-fetching when a revoke actually occurred. Minor |
- Only revoke grants for suggestions in revocable states (OUTDATED, REJECTED, PENDING_VALIDATION, NEW), preserving APPROVED grant slots - Use Promise.allSettled for revocations to avoid partial revoke leaving token counts inconsistent - Skip redundant splitSuggestionsByGrantStatus DB call on happy path - Break monolithic grantSuggestionsForOpportunity into focused functions: handleNewTokenCycle, handleExistingTokenCycle, fillRemainingCapacity Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
## [1.389.1](v1.389.0...v1.389.1) (2026-03-31) ### Bug Fixes * adds revoke and regrant logic for cases when suggestions get OUTDATED ([#2035](#2035)) ([a052952](a052952))
|
🎉 This PR is included in version 1.389.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
…DATED (#2035) ## Summary - When a granted suggestion becomes stale (OUTDATED, REJECTED, PENDING_VALIDATION), its grant is revoked and the token `used` count is decremented - Freed grant slots are filled with the next eligible NEW suggestion on the subsequent GET suggestions call - **Only revocable states are revoked** — suggestions in permanent states (FIXED, APPROVED, etc.) keep their grants untouched, preserving their token slots - Uses `Promise.allSettled` for revocations so partial failures don't leave token counts inconsistent - Skips redundant `splitSuggestionsByGrantStatus` DB call on the happy path (no revokes) - Refactors `grantSuggestionsForOpportunity` into focused functions: `handleNewTokenCycle`, `handleExistingTokenCycle`, `fillRemainingCapacity` - Bumps `@adobe/spacecat-shared-data-access` to `3.34.0` - 100% test coverage on `grant-suggestions-handler.js` ### Grant/Revoke Logic **Existing token (same cycle):** 1. Fetches all grants for the current token 2. Checks each granted suggestion's status — if in a revocable state, revokes only that grant 3. Fills remaining capacity from NEW ungranted suggestions **New token (new cycle):** 1. Creates a new token with default `tokensPerCycle` total 2. Revokes old grants from previous cycle, re-grants them under new token 3. Fills remaining capacity from NEW ungranted suggestions ### Monthly Cycle Scenarios | Scenario | Behavior | |---|---| | **NONE are FIXED** | All stale grants revoked, new grants issued | | **SOME are FIXED** | Only non-FIXED revoked, FIXED preserved, freed slots filled | | **ALL are FIXED** | No revokes, only new grants for remaining capacity | ## Related Issues - [SITES-40623](https://jira.corp.adobe.com/browse/SITES-40623) ## Test plan - [x] Unit tests: 33 passing with 100% coverage on grant-suggestions-handler.js - [x] Revoke error path covered (Promise.allSettled + throw) - [x] Manual E2E tests via `test/scripts/test-grant-monthly-cycles.mjs` — all 13 assertions passing - [x] Verified FIXED grants are never revoked - [x] Verified OUTDATED/REJECTED grants are revoked and replaced - [x] Verified token state (used count) stays consistent after revoke/re-grant 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
## [1.389.1](v1.389.0...v1.389.1) (2026-03-31) ### Bug Fixes * adds revoke and regrant logic for cases when suggestions get OUTDATED ([#2035](#2035)) ([a052952](a052952))
Summary
usedcount is decrementedPromise.allSettledfor revocations so partial failures don't leave token counts inconsistentsplitSuggestionsByGrantStatusDB call on the happy path (no revokes)grantSuggestionsForOpportunityinto focused functions:handleNewTokenCycle,handleExistingTokenCycle,fillRemainingCapacity@adobe/spacecat-shared-data-accessto3.34.0grant-suggestions-handler.jsGrant/Revoke Logic
Existing token (same cycle):
New token (new cycle):
tokensPerCycletotalMonthly Cycle Scenarios
Related Issues
Test plan
test/scripts/test-grant-monthly-cycles.mjs— all 13 assertions passing🤖 Generated with Claude Code