hide agreements page - #975
Conversation
👎 Total coverage is lower than the default branch
Detailed report8 files with a coverage regression
|
nathanstitt
left a comment
nathanstitt
left a comment
There was a problem hiding this comment.
looks good Jennifer, nothing major, mainly a helper and think we can drop that test
| // inheriting the destination agreements' own Proceed computed: once code is submitted, the | ||
| // read-only code step — NOT plain /view, which would jump an advanced study to results. | ||
| const codeSubmitted = studyHasJobStatus(study, 'CODE-SUBMITTED') | ||
| const proceedHref = codeSubmitted |
There was a problem hiding this comment.
The codeSubmitted ? studyViewCode : studyCode destination (plus its near-identical why-comment) now lives both here and in agreements/researcher/page.tsx. Since both are meant to inherit exactly what the old Agreements Proceed computed, we could extract a tiny helper (e.g. a researcherCodeStepHref(study, { orgSlug, returnTo }) next to studyHasJobStatus in @/lib/studies) so the two can't drift if the code-step routing changes again. It would also be a natural home for documenting the small asymmetry on these lines: the code-submitted branch threads returnTo while the upload branch drops it (Routes.studyCode takes none).
| if (await proceed.isVisible().catch(() => false)) { | ||
| await proceed.click() | ||
| } | ||
| await expect(page.getByTestId('code-review-section')).toBeVisible() |
There was a problem hiding this comment.
Nice — dropping the isVisible().catch() conditional here removes exactly the kind of branch that can silently mask a regression. If the gate ever reappeared, this now fails loudly on the missing code-review-section instead of quietly clicking through it.
| } | ||
| } | ||
| }) | ||
| }) |
There was a problem hiding this comment.
this is kinda of a crazy test. I don't think we need to test a negative, we just remove it
2 participants
Summary
Jira: OTTER-727
Approach
Agreements existed in three forms — a reviewer screen (
reviewer-agreements, resolved by the rule table), a reviewer route, and a researcher route — all rendering the sharedagreements-page.tsx.There's no central next/prev map: ordering lives in rule-table precedence plus hardcoded
previousHref/proceedHrefprops on each leaf view. So this is a "splice one node out of a linked list" change — delete the reviewer rule entry, then re-point every inbound edge at whatever Agreements' own Previous/Proceed pointed at. Doing it that way preserves the OTTER-643 loop fix (reviewer Previous →/review/proposal, not/review) instead of reintroducing an agreements ⇄ code-review cycle.Reviewer (Data Partner)
reviewer-agreementsentry fromREVIEWER_SCREEN_RULES. Its successor['reviewer-code-review', { when: (s) => s.codeAwaitingDecision }]has the identical predicate minus the ack clause, so it absorbs that state exactly — no other rule moved./reviewnow resolves straight to the code-review editor.REVIEWER_CODE_SCREENSinresolve.ts, so the read-only/review/codewalk-back resolves to code review too.Research Lab (researcher)
/submitted's Proceed button now computes the destination Agreements' own Proceed did:/view/codeonce code is submitted, else/code. (Not plain/view— that would jump an advanced study straight to results.)Inbound edges re-pointed
review/code-review.tsxPrevious/review/proposalreviewer-code-feedback-screen.tsxPrevious (read-only step)/review/proposalcode/page.tsxPrevious (APPROVED)/submittedcode-post-submission-view.tsxBack/submittedcode-post-decision-view.tsxPrevious step/submittedproposal-submitted.tsxProceed/codeor/view/codeBoth
/agreements/*routes now redirectRather than render, so a stale bookmark, history entry or old email link can't reach the placeholder or write an ack:
/agreements/reviewer→/review(which runs the shared reviewer guard)./agreements/researcher→ the code step. Keeps its study lookup and access check, since the destination depends on code stage and the guard stops the redirect being used to probe studies the user can't see.Deliberately retained
Per the card's "don't erase it from our code base":
reviewer-agreements-screen.tsx,agreements-page.tsx, thereviewer-agreementsScreenId+ itsSCREEN_COMPONENTSentry, bothRoutes.*Agreementsdefinitions,ackAgreementsAction, and the two*_agreements_acked_atcolumns. All commented as intentionally unreachable. Restoring the gate is re-adding one rule entry (plus the back-edges above).dashboard-rules.tsis untouched — its rule only readsresearcherAgreementsAckedand nothing in the AC requires changing it.Copy change worth a look
The
/submittedapproved banner read "proceed to Step 3 - Agreements to sign the required legal documents" — it named the hidden page and told users to do something now impossible. Changed to "…then proceed to provide your code."Step numbers and button labels are untouched per the card, so the button still reads "Proceed to step 3" and lands on a page headed "STEP 4". That gap will be handled in this card.
Testing
reviewer-screen-rules.test.ts: no rule maps toreviewer-agreements, and no state across the status × decision × ack × code-stage matrix resolves to it. The screen is retained on purpose, so absence-of-a-rule is the only thing hiding it — worth asserting directly./agreements/*route tests to assert the redirect target instead of the rendered placeholder.navigateToCodeUploadloses the/agreements/researcherhop;openCodeReviewEditorloses its conditional "Proceed to Step 3" branch (removing a conditional-visibility check that could otherwise mask a regression). Noted the now-inertagreementsAckedseed flag.Docs
docs/study-screens-logic.md: reviewer rule table renumbered (10 rows → 9), precedence notes corrected, and an OTTER-727 section recording what was removed, what was retained, and how to restore the gate.