feat: added Pending review decision#2436
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! |
Kanishkavijay39
left a comment
There was a problem hiding this comment.
Hey @anshulk-public,
Strengths
src/controllers/plg/plg-onboarding.js: The PENDING path correctly reuses the full pre-existing validation stack: admin-only access control, WAITLISTED status guard, justification required, and the review entry append logic. No duplication.src/controllers/plg/plg-onboarding.js:1771-1776: The PENDING handler correctly does NOT callsetStatus,setWaitlistReason, orpostPlgOnboardingNotification. The status stays WAITLISTED and no external notification fires - exactly right for a "marking as in-progress" action.package-lock.json:@adobe/spacecat-shared-data-accessbumped from 3.63.0 to 3.65.0, picking up the PENDING addition toREVIEW_DECISIONSfrom the companion spacecat-shared change. Deployment sequencing is correct.test/controllers/plg/plg-onboarding.test.js:6049-6113: Both new tests are well-structured - the first verifiessetStatusandsetWaitlistReasonare not called, the second verifies the existing review is preserved at index 0 and the new one appended at index 1.
Issues
Important (Should Fix)
src/controllers/plg/plg-onboarding.js around line 1762: PENDING early return is placed after the checkKey guard that PENDING doesn't need
The current flow runs:
checkKey = deriveCheckKey(onboarding)
if (!checkKey) return badRequest(...) // runs for ALL decisions including PENDING
setUpdatedBy(...)
if (decision === PENDING) { save(); return ok(...) } // PENDING early return
checkKey is only needed by the BYPASSED switch-case. PENDING doesn't use it. If a WAITLISTED record lacks a waitlistReason (malformed data, manual DB write, or a future code path that creates WAITLISTED without a reason), PENDING fails with 'Unable to determine the review reason from the onboarding record' when it should succeed. Move the PENDING early return to before the checkKey check:
onboarding.setUpdatedBy(reviewedBy);
// PENDING: record ESE action without changing status
if (decision === REVIEW_DECISIONS.PENDING) {
await onboarding.save();
return ok(PlgOnboardingDto.toAdminJSON(onboarding));
}
const checkKey = deriveCheckKey(onboarding);
if (!checkKey) {
return badRequest('Unable to determine the review reason from the onboarding record');
}(See inline comment.)
Minor (Nice to Have)
test/controllers/plg/plg-onboarding.test.js: Missing test for PENDING when checkKey would be absent
There is no test covering the edge case where getWaitlistReason() returns null on a WAITLISTED record and decision is PENDING. Once the early return is moved, add a test with a record that has no waitlistReason and confirm PENDING still returns 200. This pins the "PENDING does not need checkKey" contract.
package-lock.json: 12+ unrelated packages bumped alongside the feature change
The lockfile refresh bundles unrelated version bumps (helix-shared-body-data, mysticat-shared-seo-client, spacecat-shared-http-utils, etc.) with this feature PR. Not a blocker, but consider a separate lockfile-refresh PR next time or a note in the description so reviewers know what to focus on.
Recommendations
- The JSDoc for
update()now lists five decision types across two different endpoints. As this grows, consider a brief table or a link to the API spec to keep the inline docs from drifting.
Assessment
Ready to merge? With fixes
Reasoning: The logic is correct in the happy path, but the checkKey guard creates an unnecessary failure mode for PENDING that can be eliminated with a 3-line reorder.
Next Steps
- Move the PENDING early return to before the
checkKeyguard. - Add a test covering PENDING on a record with no
waitlistReason. - Minor lockfile and test items are optional.
Kanishkavijay39
left a comment
There was a problem hiding this comment.
Hey @anshulk-public,
Strengths
Previously flagged issues now addressed:
- The PENDING early return is now correctly placed before the
checkKeyguard (src/controllers/plg/plg-onboarding.js:1769-1773). PENDING no longer fails on records with nowaitlistReason. - New test at
test/controllers/plg/plg-onboarding.test.js:6112covers PENDING on a record with nullwaitlistReasonand pins the "PENDING does not need checkKey" contract.
CI is green. This is good to go.
Assessment
Ready to merge? Yes
Reasoning: Both flagged issues from the first review have been addressed cleanly. The reorder is minimal and correct, and the test case covers the edge case precisely.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
# [1.507.0](v1.506.0...v1.507.0) (2026-05-21) ### Features * added Pending review decision ([#2436](#2436)) ([fab83c4](fab83c4))
|
🎉 This PR is included in version 1.507.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
Adds a
PENDINGreview decision to the PLG onboarding waitlist review flow.When an ESE takes action on a
WAITLISTEDrecord (e.g. emails the customer),they can submit
PENDINGvia "Mark as Pending" to signal to other ESEsthat someone is actively handling it. The status stays
WAITLISTEDandreviewedBycaptures who is handling it.jira
Changes
src/controllers/plg/plg-onboarding.js— addedPENDINGtoallowedDecisionsand a handler that saves the review entry without changing status or re-running
the PLG flow
Please ensure your pull request adheres to the following guidelines:
describe here the problem you're solving.
If the PR is changing the API specification:
yet. Ideally, return a 501 status code with a message explaining the feature is not implemented yet.
If the PR is changing the API implementation or an entity exposed through the API:
If the PR is introducing a new audit type:
Related Issues
Thanks for contributing!