MPDX-9838 Add HR Tools and partner reminders to prod#1905
Conversation
|
Preview branch generated at https://MPDX-9838-add-partner-reminders-to-prod.d3dytjb8adxkk5.amplifyapp.com |
Bundle sizes [mpdx-react]Compared against 06663d6
|
kegrimes
left a comment
There was a problem hiding this comment.
🤖 Multi-Agent Review — PR #1905
Verdict: ✅ APPROVED WITH SUGGESTIONS · 0 blockers · Risk 3/10 (LOW)
Agents: Security, Architecture, Testing, Standards, UX (standard mode)
The refactor is sound: moving the reportsDisabled gate from tab-level to item-level is a net tech-debt reduction (removes duplicated gating between useNavPages and useHrToolsNavItems), dependency arrays are correct, no consumer breaks, and server-side authorization (blockImpersonatingNonDevelopers + API account-list scoping) is unchanged and consistent with all 11 sibling HR Tools pages.
Medium-priority findings (see inline comments)
// TODOwithout a ticket ref — load-bearing: it's the only record thatpartnerRemindersis intentionally un-gated and must be re-gated post-HCM. (Standards, Architecture, Testing, UX)reportsDisabledfails open while the separateUserOptionquery loads → flash-of-extra-items when reports go live (dormant today underDISABLE_NEW_REPORTS=true); also makes the NavMenu unverified test timing-sensitive. (UX, Testing)- Prod kill-switch (
DISABLE_NEW_REPORTS=true) is untested, and the "only partner reminders" hook tests use an ineligible user, so deleting the newreportsDisabled ||prefixes would still pass them — the gate isn't isolated. (Testing)
Suggestions (non-blocking)
- No colocated tests for
useNavPages.tsxanduseReportsDisabled.ts(the latter covers finding #3 cheapest at the source). - Item-level
reportsDisabled ||diverges fromuseReportNavItems's conditional-spread — intentional and correct (one item must be exempted); noted so it isn't mistaken for inconsistency. hrToolsItems.length === 0inhideTabis now dead post-load — harmless safety net.
Recommended before merge: the two cheap wins that pay off at HCM launch — add a ticket to the TODO, and make reportsDisabled fail closed (expose loading, treat as disabled until known), which kills both the flash and the test flakiness in one change.
There was a problem hiding this comment.
AI Review Auto-Approval
Risk Level: LOW (3/10)
Verdict: APPROVED_WITH_SUGGESTIONS (suggestions posted, no blockers)
This PR was auto-approved because:
- The multi-agent AI review determined it is low risk
- No blocking issues were found
- All suggestions have been posted as review comments for the developer to consider
If you believe this PR needs human review, dismiss this approval and request a review manually.
|
@canac Can you glance over this for me? I think I have everything covered since there are certain things that will always be true in production:
I'm also wondering if we should add the UserTypeAccess gate back on without requiring the staff account id. This way only US staff can see the report which is what is happening in the nav bar anyway. |
canac
left a comment
There was a problem hiding this comment.
The logic looks great to me! I assume we're not merging this PR until Scott is ready for it to go out.
|
@canac Sorry, did you see this question?
|
Sorry, I did miss that question! Yes, I think you can add the |
b8f294b to
af253d6
Compare
@canac Actually I've made it where global and non-Cru staff won't see the HR tools tab. Ministry Partner Reminders will only be available to US and hybrid staff members! |
Description
Set to merge on Thursday, July 16th, 2026
Since Ministry Partner Reminders broke in staff web, we want to release this report before HCM go-live. We need to add HR Tools and partner reminders to production.
Production:
DISABLE_NEW_REPORTS=true,DEVELOPMENT_ENV=false,user_type_verified="false"(no modal), andus_staff_group=null(no HCM data)Testing
User type has NOT been verified in the modal (this is what will happen in production)
US staff production test: ✅
DISABLE_NEW_REPORTStotruein your local/hrTools/partnerRemindersNon-US staff production test: ✅
DISABLE_NEW_REPORTStotruein your localUS staff non-production test: ✅
DISABLE_NEW_REPORTStofalsein your localnulland user type verified is false - no HCM data and no modal)/hrTools/partnerRemindersUser type has been verified in the modal...
... AND HCM data exists (this is similar to what devs should see locally)
US staff non-production test: ✅
DISABLE_NEW_REPORTStofalsein your local/hrTools/partnerRemindersChecklist:
/pr-reviewcommand locally and fixed any relevant suggestions