hide agreements page

docs/study-screens-logic.md

@@ -167,11 +167,10 @@ cascade with the `?from=` cases removed (those became routing, not screen-select
 | 3   | `hasResults`                                                             | `reviewer-outputs-decided`   |
 | 4   | `isExecuting`                                                            | `reviewer-outputs-pending`   |
 | 5   | `codeDecision !== null`                                                  | `reviewer-code-feedback`     |
-| 6   | `codeAwaitingDecision && !reviewerAgreementsAcked`                       | `reviewer-agreements`        |
-| 7   | `codeAwaitingDecision`                                                   | `reviewer-code-review`       |
-| 8   | `!hasSubmittedCode && status` ∈ `APPROVED`/`REJECTED`/`CHANGE-REQUESTED` | `reviewer-proposal-feedback` |
-| 9   | `status === 'PENDING-REVIEW'`                                            | `reviewer-proposal-review`   |
-| 10  | fallback                                                                 | `study-overview`             |
+| 6   | `codeAwaitingDecision`                                                   | `reviewer-code-review`       |
+| 7   | `!hasSubmittedCode && status` ∈ `APPROVED`/`REJECTED`/`CHANGE-REQUESTED` | `reviewer-proposal-feedback` |
+| 8   | `status === 'PENDING-REVIEW'`                                            | `reviewer-proposal-review`   |
+| 9   | fallback                                                                 | `study-overview`             |
 
 `reviewer-outputs-errored` (#1) has two shapes, decided by whether the job carries an encrypted
 artifact describing the run's own outcome: an `ENCRYPTED-RESULT`, or an encrypted error log
@@ -220,10 +219,18 @@ run's outputs and a submission-time scan log is not one.
 Precedence notes: errored/available/decided form a priority chain (#1–#3) — an errored run with no
 decision is claimed first, then an undecided completed run, then any remaining `hasResults` state
 (which, by exclusion, is always a decided result — OTTER-677); `isExecuting` (#4) out-ranks a
-present code decision (#5 — `CODE-APPROVED` is always present once execution starts); the agreements
-gate sits **above** active review (#6 > #7 — a reviewer must ack before the review page renders);
-and the proposal-feedback rule is gated on `!hasSubmittedCode` so the code rules own the screen once
-code exists.
+present code decision (#5 — `CODE-APPROVED` is always present once execution starts); and the
+proposal-feedback rule is gated on `!hasSubmittedCode` so the code rules own the screen once code
+exists.
+
+**OTTER-727 — the hidden agreements gate.** A `reviewer-agreements` rule used to sit between #5 and
+#6, claiming `codeAwaitingDecision && !reviewerAgreementsAcked` so a Data Partner had to ack before
+the review page rendered. It was removed when the Agreements page was hidden: #6 now owns the whole
+`codeAwaitingDecision` state (its predicate is the same one minus the ack clause). The `ScreenId`,
+its `SCREEN_COMPONENTS` entry and `_screens/reviewer-agreements-screen.tsx` are deliberately
+**retained but unreachable** — restoring the gate means re-adding that one rule entry (plus the
+back-edges that were re-pointed). The gate was always intended to give way to `legal_document` SLA
+acknowledgements (SHRMP-273); OTTER-727 hides the placeholder ahead of that ack frontend shipping.
 
 Each rule decides only **which** screen renders; the leaf view owns its own back/forward
 buttons. No query param feeds into screen selection — `resolveScreen` is a pure `state → screen`
@@ -318,9 +325,9 @@ highlights on `PENDING-REVIEW` or `codeAwaitingDecision`.
 
 Both roles are implemented. The resolvers take a `role` (`'researcher' | 'reviewer'`); `resolveScreen`
 picks the matching rule table, and the pill/highlight resolvers already branch on role. The
-**projection is shared and role-agnostic** — the reviewer flow reads the same `StudyState` facts
-(notably `reviewerAgreementsAcked`, previously unused) and inherits all the order-independence
-guarantees for free. Adding the reviewer flow was adding a rule table + adapters, not
+**projection is shared and role-agnostic** — the reviewer flow reads the same `StudyState` facts and
+inherits all the order-independence guarantees for free (`reviewerAgreementsAcked` is the exception:
+it is still projected but, since OTTER-727, read by no rule). Adding the reviewer flow was adding a rule table + adapters, not
 re-architecting — exactly as the design intended.
 
 ### Reviewer routing (`/review`)
@@ -332,11 +339,17 @@ re-architecting — exactly as the design intended.
 - **Shared guard** (`review/reviewer-page-guard.tsx`): both reviewer entry points run the same
   access preamble (session/org → study → lab-org redirect to `/view` → `isSubmittedStudy` →
   enclave-only), so a non-reviewer hitting either URL directly is handled identically.
-- **Agreements gate as a screen**: the old redirect-to-`/agreements` is now the `reviewer-agreements`
-  screen (rule #3). The reviewer branch of `agreements/page.tsx` became a plain revisitable step
-  (no `?from=`), like the researcher branch. This gate reads
-  `study.reviewerAgreementsAckedAt`; once study-level-agreement acknowledgement ships on
-  `legal_document`, rule #6 goes with it — two agreement gates on one study would disagree.
+- **Agreements gate — hidden (OTTER-727)**: the old redirect-to-`/agreements` became the
+  `reviewer-agreements` screen, and is now hidden entirely. Its rule is gone from the reviewer table
+  (see the OTTER-727 note above), and both `/agreements/reviewer` and `/agreements/researcher`
+  redirect onward (to `/review` and the code step respectively) so stale bookmarks and history entries
+  can't reach the placeholder or write an ack. Nothing links to either route. The screen component,
+  the shared `agreements-page.tsx`, both `Routes.*Agreements` definitions, `ackAgreementsAction` and
+  the two `*_agreements_acked_at` columns are all retained for a future revival; the ack facts are
+  still projected onto `StudyState` but no screen rule reads them. This gate read
+  `study.reviewerAgreementsAckedAt`, and was already slated to go when study-level-agreement
+  acknowledgement ships on `legal_document` (SHRMP-273) — two agreement gates on one study would
+  disagree. Hiding it now means that landing does not have to remove a live gate.
 - **Dedicated proposal route** (`/review/proposal`, `studyReviewProposal`): backs the "View approved
   initial request" link. It always shows the **decided** initial request regardless of code stage,
   and **falls through** to the canonical `/review` screen (e.g. editable proposal review) when the

src/app/[orgSlug]/study/[studyId]/_screens/reviewer-agreements-screen.tsx

@@ -4,6 +4,11 @@ import { Routes } from '@/lib/routes'
 import { AgreementsPage } from '../agreements/agreements-page'
 import type { ScreenComponentProps } from './types'
 
+// OTTER-727: HIDDEN — nothing renders this. The `reviewer-agreements` rule was removed from
+// REVIEWER_SCREEN_RULES, so reviewer-code-review now claims the whole codeAwaitingDecision state.
+// Retained rather than deleted because the product direction for agreements is undecided; restoring
+// the gate means re-adding that one rule entry (and the back-edges listed in the OTTER-727 plan).
+//
 // Reviewer agreements gate, modelled as a screen (not a redirect). Acking proceeds into code review
 // (the bare /review re-resolves to reviewer-code-review once reviewerAgreementsAckedAt is set).
 // OTTER-643: Previous walks back to the decided proposal (/review/proposal), the loop-free analog of

src/app/[orgSlug]/study/[studyId]/_screens/reviewer-code-feedback-screen.tsx

@@ -15,10 +15,11 @@ export async function ReviewerCodeFeedbackScreen({ study, raw, orgSlug, descript
     }
 
     // Only the read-only /review/code walk-back (descriptor.readOnlyCodeStep) shows "Previous" → it
-    // continues back through agreements → proposal (OTTER-643). The live code-decision screen leaves it
-    // unset, matching the live DO design that hides Previous.
+    // continues back to the decided proposal (OTTER-643; one hop since OTTER-727 hid the intervening
+    // agreements step). The live code-decision screen leaves it unset, matching the live DO design
+    // that hides Previous.
     const previousHref = descriptor.readOnlyCodeStep
-        ? Routes.studyReviewerAgreements({ orgSlug, studyId: study.id })
+        ? Routes.studyReviewProposal({ orgSlug, studyId: study.id })
         : undefined
 
     // OTTER-687: forward to the DP outputs screen, which lives at bare /review. Suppressed while

src/app/[orgSlug]/study/[studyId]/agreements/researcher/page.test.tsx

@@ -1,17 +1,12 @@
 import { beforeEach, describe, it, expect, vi } from 'vitest'
 import { redirect } from 'next/navigation'
-import * as RouterMock from 'next-router-mock'
 import {
     db,
     insertTestStudyJobData,
     insertTestStudyOnly,
     mockDualRoleSessionWithTestData,
     mockSessionWithTestData,
-    renderWithProviders,
-    screen,
     setTestStudyStatus,
-    userEvent,
-    waitFor,
 } from '@/tests/unit.helpers'
 import ResearcherAgreementsRoute from './page'
 
@@ -29,135 +24,66 @@ const renderRoute = (orgSlug: string, studyId: string, searchParams: Record<stri
         searchParams: Promise.resolve(searchParams),
     })
 
-describe('ResearcherAgreementsRoute', () => {
-    it('renders researcher agreements for APPROVED study not yet acknowledged', async () => {
+// OTTER-727 hid the Agreements step. This route now exists only to catch stale bookmarks/history: it
+// must always redirect onward to the code step and never render the placeholder.
+describe('ResearcherAgreementsRoute (hidden — redirects)', () => {
+    it('redirects an APPROVED study with no code to the code upload page', async () => {
         const { org, user } = await mockSessionWithTestData({ orgType: 'lab' })
         const { study } = await insertTestStudyOnly({ org, researcherId: user.id })
 
-        const page = await renderRoute(org.slug, study.id)
-        renderWithProviders(page!)
+        await expect(renderRoute(org.slug, study.id)).rejects.toThrow('NEXT_REDIRECT')
 
-        expect(screen.getByText('STEP 3A')).toBeInTheDocument()
-        expect(screen.getByText('STEP 3B')).toBeInTheDocument()
-        expect(screen.getByText('STEP 3C')).toBeInTheDocument()
-        expect(screen.getByRole('button', { name: /Proceed to Step 4/ })).toBeInTheDocument()
+        expect(mockRedirect).toHaveBeenCalledWith(`/${org.slug}/study/${study.id}/code`)
     })
 
-    // Revisitable researcher step: renders for an authorized researcher regardless of ack state or
-    // study status, and does not self-redirect. resolveScreen (on /view) is the screen authority.
-    it('renders researcher agreements even after acknowledging (revisitable, no redirect)', async () => {
-        const { org, user } = await mockSessionWithTestData({ orgType: 'lab' })
-        const { study } = await insertTestStudyOnly({ org, researcherId: user.id })
-        await db
-            .updateTable('study')
-            .set({ researcherAgreementsAckedAt: new Date() })
-            .where('id', '=', study.id)
-            .execute()
-
-        const page = await renderRoute(org.slug, study.id)
-        renderWithProviders(page!)
-
-        expect(mockRedirect).not.toHaveBeenCalled()
-        expect(screen.getByText('STEP 3A')).toBeInTheDocument()
-    })
-
-    it('renders researcher agreements even when study is not APPROVED (revisitable, no redirect)', async () => {
-        const { org, user } = await mockSessionWithTestData({ orgType: 'lab' })
-        const { study } = await insertTestStudyOnly({ org, researcherId: user.id })
-        await setTestStudyStatus(study.id, 'DRAFT')
-
-        const page = await renderRoute(org.slug, study.id)
-        renderWithProviders(page!)
-
-        expect(mockRedirect).not.toHaveBeenCalled()
-        expect(screen.getByText('STEP 3A')).toBeInTheDocument()
-    })
-
-    // Previous → /submitted (the approved-proposal page with a working "Proceed to step 3"), NOT
-    // /view — /view resolves to proposal-feedback, which has no forward path and would dead-end an
-    // approved-no-code researcher.
-    it('Previous button targets /submitted (not /view), no ?from=', async () => {
+    // Inherits what the page's own Proceed used to compute: the read-only code step, NOT plain /view
+    // (which would jump an advanced study straight to results).
+    it('redirects to /view/code once code has been submitted', async () => {
         const { org, user } = await mockSessionWithTestData({ orgType: 'lab' })
         const { study } = await insertTestStudyJobData({ org, researcherId: user.id, jobStatus: 'CODE-SUBMITTED' })
 
-        const page = await renderRoute(org.slug, study.id)
-        renderWithProviders(page!)
-
-        const interact = userEvent.setup()
-        await interact.click(screen.getByRole('button', { name: /Previous/ }))
+        await expect(renderRoute(org.slug, study.id)).rejects.toThrow('NEXT_REDIRECT')
 
-        // eslint-disable-next-line @typescript-eslint/no-explicit-any
-        const { asPath } = (RouterMock as any).memoryRouter
-        expect(asPath).toBe(`/${org.slug}/study/${study.id}/submitted`)
-        expect(asPath).not.toContain('from=')
+        expect(mockRedirect).toHaveBeenCalledWith(`/${org.slug}/study/${study.id}/view/code`)
     })
 
-    it('Previous preserves returnTo=org on the /submitted link', async () => {
+    it('preserves returnTo=org on the redirect so org scope survives the hop', async () => {
         const { org, user } = await mockSessionWithTestData({ orgType: 'lab' })
         const { study } = await insertTestStudyJobData({ org, researcherId: user.id, jobStatus: 'CODE-SUBMITTED' })
 
-        const page = await renderRoute(org.slug, study.id, { returnTo: 'org' })
-        renderWithProviders(page!)
-
-        const interact = userEvent.setup()
-        await interact.click(screen.getByRole('button', { name: /Previous/ }))
+        await expect(renderRoute(org.slug, study.id, { returnTo: 'org' })).rejects.toThrow('NEXT_REDIRECT')
 
-        // eslint-disable-next-line @typescript-eslint/no-explicit-any
-        const { asPath } = (RouterMock as any).memoryRouter
-        expect(asPath).toBe(`/${org.slug}/study/${study.id}/submitted?returnTo=org`)
+        expect(mockRedirect).toHaveBeenCalledWith(`/${org.slug}/study/${study.id}/view/code?returnTo=org`)
     })
 
-    // "Proceed to Step 4" → the code step (/view/code), not plain /view (which would jump an advanced
-    // study straight to results).
-    it('Proceed targets /view/code when code is already submitted', async () => {
-        const { org, user } = await mockSessionWithTestData({ orgType: 'lab' })
-        const { study } = await insertTestStudyJobData({ org, researcherId: user.id, jobStatus: 'CODE-SUBMITTED' })
-
-        const page = await renderRoute(org.slug, study.id)
-        renderWithProviders(page!)
-
-        const interact = userEvent.setup()
-        await interact.click(screen.getByRole('button', { name: /Proceed to Step 4/ }))
-
-        await waitFor(() => {
-            // eslint-disable-next-line @typescript-eslint/no-explicit-any
-            const { asPath } = (RouterMock as any).memoryRouter
-            expect(asPath).toBe(`/${org.slug}/study/${study.id}/view/code`)
-        })
-    })
-
-    it('Proceed targets code upload when no code has been submitted yet', async () => {
+    it('redirects regardless of ack state (the ack no longer gates anything)', async () => {
         const { org, user } = await mockSessionWithTestData({ orgType: 'lab' })
         const { study } = await insertTestStudyOnly({ org, researcherId: user.id })
+        await db
+            .updateTable('study')
+            .set({ researcherAgreementsAckedAt: new Date() })
+            .where('id', '=', study.id)
+            .execute()
 
-        const page = await renderRoute(org.slug, study.id)
-        renderWithProviders(page!)
-
-        const interact = userEvent.setup()
-        await interact.click(screen.getByRole('button', { name: /Proceed to Step 4/ }))
+        await expect(renderRoute(org.slug, study.id)).rejects.toThrow('NEXT_REDIRECT')
 
-        await waitFor(() => {
-            // eslint-disable-next-line @typescript-eslint/no-explicit-any
-            const { asPath } = (RouterMock as any).memoryRouter
-            expect(asPath).toBe(`/${org.slug}/study/${study.id}/code`)
-        })
+        expect(mockRedirect).toHaveBeenCalledWith(`/${org.slug}/study/${study.id}/code`)
     })
 
-    it('renders Previous button for researcher', async () => {
+    it('redirects a non-APPROVED study too', async () => {
         const { org, user } = await mockSessionWithTestData({ orgType: 'lab' })
         const { study } = await insertTestStudyOnly({ org, researcherId: user.id })
+        await setTestStudyStatus(study.id, 'DRAFT')
 
-        const page = await renderRoute(org.slug, study.id)
-        renderWithProviders(page!)
+        await expect(renderRoute(org.slug, study.id)).rejects.toThrow('NEXT_REDIRECT')
 
-        expect(screen.getByRole('button', { name: 'Previous' })).toBeInTheDocument()
+        expect(mockRedirect).toHaveBeenCalledWith(`/${org.slug}/study/${study.id}/code`)
     })
 
-    // Dual-role regression: a user who is both reviewer (enclave) and researcher (their own lab)
-    // reaches this researcher route via their lab's slug after clicking "Proceed to step 3" on the
-    // approved proposal. Even though they CAN review, this route keeps them in the researcher flow —
-    // it must NOT bounce them into the reviewer agreement → /review loop.
-    it('keeps a dual-role user in the researcher flow (does not treat them as a reviewer)', async () => {
+    // Dual-role regression (kept from the pre-OTTER-727 suite): a user who is both reviewer (enclave)
+    // and researcher (their own lab) must stay in the researcher flow — the redirect goes to the lab's
+    // code step, never into the reviewer's /review flow.
+    it('keeps a dual-role user in the researcher flow', async () => {
         const { user, labOrg, enclaveOrg } = await mockDualRoleSessionWithTestData()
         const study = await db
             .insertInto('study')
@@ -177,11 +103,8 @@ describe('ResearcherAgreementsRoute', () => {
             .returning('id')
             .executeTakeFirstOrThrow()
 
-        const page = await renderRoute(labOrg.slug, study.id)
-        renderWithProviders(page!)
+        await expect(renderRoute(labOrg.slug, study.id)).rejects.toThrow('NEXT_REDIRECT')
 
-        expect(mockRedirect).not.toHaveBeenCalled()
-        expect(screen.getByText('STEP 3A')).toBeInTheDocument()
-        expect(screen.getByRole('button', { name: /Proceed to Step 4/ })).toBeInTheDocument()
+        expect(mockRedirect).toHaveBeenCalledWith(`/${labOrg.slug}/study/${study.id}/code`)
     })
 })