# Phase 1 Design Specification — Mapper Split + IN_PROGRESS Auto-Transition

**Status**: APPROVED (2026-04-14)
**Overrides applied**: 2 (transition placement, failure handling)

---

## 1. Scope

| Sub-phase | Summary |
|---|---|
| **1.1 — Mapper Split** | Split `PaperMapper.toDetail()` into `toAdminDetail(Paper)` and `toAuthorDetail(Paper)`. Update `findByIdForCaller()` branching. Add `@JsonInclude(NON_NULL)` to `PaperDetailResponse`. |
| **1.2 — IN_PROGRESS Auto-Transition** | When a reviewer first previews or downloads a manuscript via `ReviewerPaperServiceImpl.getManuscriptForReview()`, auto-transition the review from `PENDING` → `IN_PROGRESS` and set `startedAt`. |

No database migration required. No new dependencies. No frontend changes.

---

## 2. Phase 1.1 — Mapper Split

### 2.1 Current State

`PaperMapper.toDetail(Paper)` (line 67) is called for **both** admin and author callers from `PaperServiceImpl.findByIdForCaller()` (line 86):

```java
if (isAdmin || isAuthor) {
    return paperMapper.toDetail(paper);
}
```

This means authors see admin-only fields: `adminNotes`, `plagiarismScore`, `plagiarismReport`, `submittedByName` (always their own name — harmless but unnecessary). Authors also see the raw `manuscriptName` which is fine for them but the DTO shape is identical regardless of role.

### 2.2 Target State

Two new methods replace `toDetail()`:

| Method | Caller | Purpose |
|---|---|---|
| `toAdminDetail(Paper)` | Admin | Full detail including `adminNotes`, `plagiarismScore`, `plagiarismReport`, `confidentialNotes` in reviews |
| `toAuthorDetail(Paper)` | Author | Everything except admin-only fields; reviews exclude `confidentialNotes` |

`toDetail(Paper)` is **deleted** to prevent accidental use.

`toReviewerSafeDetail(Paper)` (line 147) is **unchanged**.

### 2.3 Field Inclusion Matrix

| Field | Admin | Author | Reviewer |
|---|---|---|---|
| `id, referenceNo, titleHi/En, abstractHi/En, keywords, status` | Yes | Yes | Yes |
| `categoryNameHi/En` | Yes | Yes | Yes |
| `manuscriptName, manuscriptSize` | Yes | Yes | null / yes |
| `submittedByName` | Yes | null | null |
| `doi, volume, issue, pageStart, pageEnd` | Yes | Yes | null |
| `citationCount, downloadCount` | Yes | Yes | 0 |
| `plagiarismScore, plagiarismReport` | Yes | null | null |
| `adminNotes` | Yes | null | null |
| `requiredReviewers` | Yes | null (was exposed — **change**) | 0 |
| `coauthors` | Yes | Yes | empty |
| `reviews` (ReviewSummaryResponse) | Yes (with `confidentialNotes`) | Yes (without `confidentialNotes`) | empty |
| `statusHistory` | Yes | Yes | empty |
| `submittedAt, acceptedAt, publishedAt` | Yes | Yes | submittedAt only |

Key changes from current `toDetail()` (what author **loses**):
- `adminNotes` → null
- `plagiarismScore` → null
- `plagiarismReport` → null
- `submittedByName` → null (author already knows who they are)
- `requiredReviewers` → null (editorial workflow detail)
- `reviews[].confidentialNotes` → excluded (reviewer's private notes to editor)

### 2.4 DTO Changes — `PaperDetailResponse.java`

**Add class-level annotation:**
```java
@JsonInclude(JsonInclude.Include.NON_NULL)
```

This ensures null fields are omitted from JSON, keeping the wire payload clean for author/reviewer responses without needing separate DTO types.

**Add `confidentialNotes` to `ReviewSummaryResponse`:**
```java
@Data @Builder
public static class ReviewSummaryResponse {
    private Integer originalityScore;
    private Integer methodologyScore;
    private Integer clarityScore;
    private Integer relevanceScore;
    private Integer referencesScore;
    private BigDecimal overallScore;
    private String recommendation;
    private String commentsToAuthor;
    private String confidentialNotes;  // admin-only
    private String completedAt;
}
```

**Change primitive `int` fields to `Integer`** for fields that may be null in author/reviewer responses:
```java
// These three change from int → Integer:
private Integer citationCount;
private Integer downloadCount;
private Integer requiredReviewers;
```

This is required because `@JsonInclude(NON_NULL)` cannot suppress `int` (primitives are never null; `0` would always serialize). With `Integer`, null values are omitted.

**Note**: `CoauthorResponse.authorOrder` (int) and `CoauthorResponse.corresponding` (boolean) stay as primitives — they are never null when the list is populated, and the list itself is empty for reviewer responses.

### 2.5 PaperMapper Changes

**Delete** `toDetail(Paper)`.

**Add** `toAdminDetail(Paper)`:

```java
public PaperDetailResponse toAdminDetail(Paper paper) {
    return PaperDetailResponse.builder()
            .id(paper.getId())
            .referenceNo(paper.getReferenceNo())
            .titleHi(paper.getTitleHi())
            .titleEn(paper.getTitleEn())
            .abstractHi(paper.getAbstractHi())
            .abstractEn(paper.getAbstractEn())
            .keywords(paper.getKeywords() != null
                    ? Arrays.asList(paper.getKeywords())
                    : Collections.emptyList())
            .status(paper.getStatus())
            .categoryNameHi(paper.getCategory() != null ? paper.getCategory().getNameHi() : null)
            .categoryNameEn(paper.getCategory() != null ? paper.getCategory().getNameEn() : null)
            .manuscriptName(paper.getManuscriptName())
            .manuscriptSize(paper.getManuscriptSize())
            .submittedByName(paper.getSubmittedBy() != null ? paper.getSubmittedBy().getNameEn() : null)
            .doi(paper.getDoi())
            .volume(paper.getVolume())
            .issue(paper.getIssue())
            .pageStart(paper.getPageStart())
            .pageEnd(paper.getPageEnd())
            .citationCount(paper.getCitationCount())
            .downloadCount(paper.getDownloadCount())
            .plagiarismScore(paper.getPlagiarismScore())
            .plagiarismReport(paper.getPlagiarismReport())
            .adminNotes(paper.getAdminNotes())
            .requiredReviewers(paper.getRequiredReviewers())
            .coauthors(mapCoauthors(paper))
            .reviews(mapCompletedReviewsForAdmin(paper))  // includes confidentialNotes
            .statusHistory(mapStatusHistory(paper))
            .submittedAt(formatInstant(paper.getSubmittedAt()))
            .acceptedAt(paper.getAcceptedAt())
            .publishedAt(formatInstant(paper.getPublishedAt()))
            .build();
}
```

**Add** `toAuthorDetail(Paper)`:

```java
public PaperDetailResponse toAuthorDetail(Paper paper) {
    return PaperDetailResponse.builder()
            .id(paper.getId())
            .referenceNo(paper.getReferenceNo())
            .titleHi(paper.getTitleHi())
            .titleEn(paper.getTitleEn())
            .abstractHi(paper.getAbstractHi())
            .abstractEn(paper.getAbstractEn())
            .keywords(paper.getKeywords() != null
                    ? Arrays.asList(paper.getKeywords())
                    : Collections.emptyList())
            .status(paper.getStatus())
            .categoryNameHi(paper.getCategory() != null ? paper.getCategory().getNameHi() : null)
            .categoryNameEn(paper.getCategory() != null ? paper.getCategory().getNameEn() : null)
            .manuscriptName(paper.getManuscriptName())
            .manuscriptSize(paper.getManuscriptSize())
            // submittedByName: null — author knows who they are
            // plagiarismScore/plagiarismReport/adminNotes: null — admin-only
            // requiredReviewers: null — editorial workflow
            .doi(paper.getDoi())
            .volume(paper.getVolume())
            .issue(paper.getIssue())
            .pageStart(paper.getPageStart())
            .pageEnd(paper.getPageEnd())
            .citationCount(paper.getCitationCount())
            .downloadCount(paper.getDownloadCount())
            .coauthors(mapCoauthors(paper))
            .reviews(mapCompletedReviews(paper))  // excludes confidentialNotes
            .statusHistory(mapStatusHistory(paper))
            .submittedAt(formatInstant(paper.getSubmittedAt()))
            .acceptedAt(paper.getAcceptedAt())
            .publishedAt(formatInstant(paper.getPublishedAt()))
            .build();
}
```

**Extract** coauthor mapping into a shared helper (currently inlined in `toDetail`):

```java
private List<PaperDetailResponse.CoauthorResponse> mapCoauthors(Paper paper) {
    return paper.getCoauthors() != null
            ? paper.getCoauthors().stream()
                .map(c -> PaperDetailResponse.CoauthorResponse.builder()
                        .nameHi(c.getNameHi())
                        .nameEn(c.getNameEn())
                        .email(c.getEmail())
                        .institution(c.getInstitution())
                        .authorOrder(c.getAuthorOrder())
                        .corresponding(c.isCorresponding())
                        .build())
                .collect(Collectors.toList())
            : Collections.emptyList();
}
```

**Split** `mapCompletedReviews` into two variants:

```java
// Used by toAuthorDetail — excludes confidentialNotes
private List<PaperDetailResponse.ReviewSummaryResponse> mapCompletedReviews(Paper paper) {
    return reviewRepository.findByPaperId(paper.getId()).stream()
            .filter(r -> r.getStatus() == ReviewStatus.COMPLETED)
            .map(r -> PaperDetailResponse.ReviewSummaryResponse.builder()
                    .originalityScore(r.getOriginalityScore())
                    .methodologyScore(r.getMethodologyScore())
                    .clarityScore(r.getClarityScore())
                    .relevanceScore(r.getRelevanceScore())
                    .referencesScore(r.getReferencesScore())
                    .overallScore(r.getOverallScore())
                    .recommendation(r.getRecommendation() != null ? r.getRecommendation().name() : null)
                    .commentsToAuthor(r.getCommentsToAuthor())
                    // confidentialNotes: omitted
                    .completedAt(formatInstant(r.getCompletedAt()))
                    .build())
            .collect(Collectors.toList());
}

// Used by toAdminDetail — includes confidentialNotes
private List<PaperDetailResponse.ReviewSummaryResponse> mapCompletedReviewsForAdmin(Paper paper) {
    return reviewRepository.findByPaperId(paper.getId()).stream()
            .filter(r -> r.getStatus() == ReviewStatus.COMPLETED)
            .map(r -> PaperDetailResponse.ReviewSummaryResponse.builder()
                    .originalityScore(r.getOriginalityScore())
                    .methodologyScore(r.getMethodologyScore())
                    .clarityScore(r.getClarityScore())
                    .relevanceScore(r.getRelevanceScore())
                    .referencesScore(r.getReferencesScore())
                    .overallScore(r.getOverallScore())
                    .recommendation(r.getRecommendation() != null ? r.getRecommendation().name() : null)
                    .commentsToAuthor(r.getCommentsToAuthor())
                    .confidentialNotes(r.getConfidentialNotes())
                    .completedAt(formatInstant(r.getCompletedAt()))
                    .build())
            .collect(Collectors.toList());
}
```

### 2.6 `findByIdForCaller` Update

```java
// PaperServiceImpl.java, lines 85-86 change from:
if (isAdmin || isAuthor) {
    return paperMapper.toDetail(paper);
}

// to:
if (isAdmin) {
    return paperMapper.toAdminDetail(paper);
}
if (isAuthor) {
    return paperMapper.toAuthorDetail(paper);
}
```

All call sites that invoke `findByIdForCaller` (`submit`, `uploadRevision`, `updatePaper`) pass the author's ID, so they will correctly route through `toAuthorDetail`. No other call site changes needed.

### 2.7 Frontend Impact

**None.** The frontend `PaperDetail` TypeScript type already has all fields as optional (`?`). With `@JsonInclude(NON_NULL)`, fields that were previously serialized as `null` will simply be absent from the JSON. The existing UI already guards with optional chaining and conditional rendering.

Frontend `confidentialNotes` type update is **deferred** to Phase 4 (admin UI work). Not included in Phase 1.

---

### 2.8 Test Strategy for Mapper Split (Addendum)

**Status**: ADDED (2026-04-14) — this section was added post-approval as Phase 1A-ii (tests) discovered the original spec lacked test enumeration.

The mapper split introduces a security-critical separation between admin and author views. The test strategy verifies that `toAuthorDetail` correctly nulls out admin-only fields and `toAdminDetail` correctly populates them, with regression coverage for `toReviewerSafeDetail` and JSON-level verification that `@JsonInclude(NON_NULL)` suppresses the null fields from the wire format.

Tests live in `src/test/java/com/shodh/sanchayan/service/PaperMapperTest.java`. The test class uses `@ExtendWith(MockitoExtension.class)`, mocks `ReviewRepository` and `PaperStatusHistoryRepository` via `@Mock`, and instantiates `PaperMapper` via `@InjectMocks`. A `@BeforeEach` method constructs a fully-populated `Paper` fixture with `adminNotes`, `plagiarismScore`, `plagiarismReport`, completed reviews with `confidentialNotes` and `commentsToAuthor`, coauthors, and status history. Individual tests exercise one variant and assert specific field presence or absence.

#### Test 1 — toAdminDetail_populatesAllAdminFields
Given the fully-populated fixture, call `toAdminDetail(paper)`. Assert that `getAdminNotes()`, `getPlagiarismScore()`, `getPlagiarismReport()`, `getSubmittedByName()`, and `getRequiredReviewers()` all return the fixture's populated values. Verifies admins still see every admin-visible field.

#### Test 2 — toAuthorDetail_nullsAdminOnlyFields
Given the same fixture, call `toAuthorDetail(paper)`. Assert that `getAdminNotes()`, `getPlagiarismScore()`, `getPlagiarismReport()`, `getSubmittedByName()`, and `getRequiredReviewers()` all return null. Core security fix.

#### Test 3 — toAuthorDetail_populatesAuthorVisibleFields
Given the same fixture, call `toAuthorDetail(paper)`. Assert that `getId()`, `getReferenceNo()`, `getTitleEn()`, `getAbstractEn()`, `getStatus()`, `getDoi()`, `getVolume()`, `getIssue()`, `getPageStart()`, `getPageEnd()`, `getCitationCount()`, `getDownloadCount()`, `getCategoryNameEn()`, `getManuscriptName()`, and `getSubmittedAt()` all return the fixture's populated values. Verifies the author variant is not over-restrictive.

#### Test 4 — toAdminDetail_reviewsIncludeConfidentialNotes
Given a fixture with at least one completed review that has `confidentialNotes` populated, call `toAdminDetail(paper)`. Assert that `getReviews()` is not empty and the first review's `getConfidentialNotes()` returns the populated value.

#### Test 5 — toAuthorDetail_reviewsExcludeConfidentialNotes
Given the same fixture, call `toAuthorDetail(paper)`. Assert that `getReviews()` is not empty and every review has `getConfidentialNotes()` return null. Prevents reviewer's confidential notes from leaking to the author.

#### Test 6 — toAuthorDetail_reviewsIncludeCommentsToAuthor
Given the same fixture with `commentsToAuthor` populated, call `toAuthorDetail(paper)`. Assert that the first review's `getCommentsToAuthor()` returns the populated value. Inverse security check: the author MUST see reviewer feedback.

#### Test 7 — toAdminDetail_includesCoauthors
Given a fixture with coauthors, call `toAdminDetail(paper)`. Assert that `getCoauthors()` is not empty and the first coauthor's `getNameEn()` returns the populated value.

#### Test 8 — toAuthorDetail_includesCoauthors
Given the same fixture, call `toAuthorDetail(paper)`. Assert that `getCoauthors()` is not empty and the first coauthor's `getNameEn()` returns the populated value.

#### Test 9 — toAuthorDetail_jsonSerializationSuppressesNullFields
Given the fully-populated fixture, call `toAuthorDetail(paper)`. Serialize the result to JSON via `new ObjectMapper().writeValueAsString(response)`. Assert that the JSON string does NOT contain the substrings `"adminNotes"`, `"plagiarismScore"`, `"plagiarismReport"`, `"submittedByName"`, or `"requiredReviewers"`. Assert that the JSON DOES contain the substring `"commentsToAuthor"`. Verifies `@JsonInclude(NON_NULL)` suppresses null fields at the wire format level.

#### Test 10 — toReviewerSafeDetail_regressionAnchor
Given the fully-populated fixture, call `toReviewerSafeDetail(paper)`. Assert that `getAdminNotes()`, `getPlagiarismScore()`, `getPlagiarismReport()`, `getSubmittedByName()`, and `getDoi()` all return null, and `getReferenceNo()`, `getTitleEn()`, `getAbstractEn()`, `getStatus()`, `getKeywords()`, and `getCategoryNameEn()` all return populated values. Regression anchor: if a future change accidentally exposes sensitive data via `toReviewerSafeDetail`, this test fails.

---

## 3. Phase 1.2 — IN_PROGRESS Auto-Transition

### 3.1 Current State

- `Review.status` is set to `PENDING` when assigned by admin (PaperServiceImpl line 476)
- `ReviewStatus.IN_PROGRESS` exists in the enum but is **never written** anywhere in the codebase
- `Review.startedAt` column exists in the entity (line 30) but is **never set**
- `ReviewerPaperServiceImpl.getManuscriptForReview()` is `@Transactional(readOnly = true)` (line 60)
- `ReviewAuthorizationServiceImpl.assertReviewerCanAccess()` returns the `reviewId` UUID (line 34-36)
- `ACTIVE_STATUSES` already includes `IN_PROGRESS` (line 17-18) so the transition will not break authorization checks

### 3.2 Target State

When `getManuscriptForReview()` is called and the review is in `PENDING` status, auto-transition to `IN_PROGRESS` and record `startedAt = Instant.now()`. This signals to admins that the reviewer has started engaging with the paper.

### 3.3 Transition Placement (OVERRIDE 1)

The transition fires **after** all manuscript processing succeeds (authorization, agreement gate, paper load, byte fetch, metadata strip, watermark) and **before** the audit log write. This ensures IN_PROGRESS is only set when the reviewer has actually received the manuscript bytes.

Rationale: The spirit of IN_PROGRESS is "the reviewer has successfully started looking at the paper," not "the reviewer has been authorized." An authorized-but-failed access (S3 outage, corrupt PDF, metadata strip failure, blocked agreement gate for downloads) should not mark the review as IN_PROGRESS.

```java
// In ReviewerPaperServiceImpl.getManuscriptForReview():

// 1. Authorization
UUID reviewId = reviewAuthorizationService.assertReviewerCanAccess(reviewerId, paperId);

// 2. Agreement gate (unchanged)
// 3. Load paper (unchanged)
// 4. Fetch stored bytes (unchanged)
// 5. Detect content type, strip metadata, watermark (unchanged)

// 6. Auto-transition PENDING → IN_PROGRESS (after all processing succeeds)
transitionToInProgressIfPending(reviewId, reviewerId, paperId);

// 7. Audit log (unchanged)
paperAccessAuditService.logAccess(...);

return new ManuscriptContent(bytes, paper.getReferenceNo(), contentType);
```

### 3.4 Implementation — `transitionToInProgressIfPending`

Add to `ReviewerPaperServiceImpl`:

```java
/**
 * Auto-transitions a review from PENDING to IN_PROGRESS on first
 * successful manuscript access. Fail-open: exceptions are caught and
 * logged so the reviewer still receives their manuscript bytes.
 *
 * <p>Uses {@code REQUIRES_NEW} propagation to isolate the write from
 * the outer transaction, preventing a JPA rollback-only flag from
 * poisoning the manuscript-fetch transaction.
 */
@Transactional(propagation = Propagation.REQUIRES_NEW)
public void transitionToInProgressIfPending(UUID reviewId, UUID reviewerId, UUID paperId) {
    try {
        reviewRepository.findById(reviewId).ifPresent(review -> {
            if (review.getStatus() == ReviewStatus.PENDING) {
                review.setStatus(ReviewStatus.IN_PROGRESS);
                review.setStartedAt(Instant.now());
                reviewRepository.save(review);
                log.info("Review {} auto-transitioned PENDING → IN_PROGRESS", reviewId);
            }
        });
    } catch (Exception e) {
        log.warn("Review {} auto-transition failed (non-fatal) for reviewer {} on paper {}: {}",
                 reviewId, reviewerId, paperId, e.getMessage());
    }
}
```

**Note on `REQUIRES_NEW`**: Because the outer `getManuscriptForReview()` is `@Transactional` (not readOnly — see §3.5), a caught exception from `reviewRepository.save()` could set the outer transaction's rollback-only flag before the catch block executes. Using `REQUIRES_NEW` isolates the transition into its own transaction so a save failure cannot poison the outer transaction. This matches the fail-open contract: the reviewer always gets their bytes regardless of whether the status update succeeds.

**Self-invocation caveat**: Spring AOP proxies do not intercept self-calls within the same bean. Since `transitionToInProgressIfPending` is called from `getManuscriptForReview()` on the same `ReviewerPaperServiceImpl` instance, the `@Transactional(propagation = REQUIRES_NEW)` annotation would be ignored. To make the propagation work, either:
- (A) Extract `transitionToInProgressIfPending` into a separate `@Service` bean (e.g., `ReviewStatusTransitionService`), or
- (B) Inject `self` via `@Lazy` and call `self.transitionToInProgressIfPending(...)`.

**Recommended approach**: (A) — extract into a small `ReviewStatusTransitionService` with a single public method. This avoids the `@Lazy` self-injection pattern and keeps the class focused.

```java
@Service
@RequiredArgsConstructor
@Slf4j
public class ReviewStatusTransitionService {

    private final ReviewRepository reviewRepository;

    @Transactional(propagation = Propagation.REQUIRES_NEW)
    public void transitionToInProgressIfPending(UUID reviewId, UUID reviewerId, UUID paperId) {
        try {
            reviewRepository.findById(reviewId).ifPresent(review -> {
                if (review.getStatus() == ReviewStatus.PENDING) {
                    review.setStatus(ReviewStatus.IN_PROGRESS);
                    review.setStartedAt(Instant.now());
                    reviewRepository.save(review);
                    log.info("Review {} auto-transitioned PENDING → IN_PROGRESS", reviewId);
                }
            });
        } catch (Exception e) {
            log.warn("Review {} auto-transition failed (non-fatal) for reviewer {} on paper {}: {}",
                     reviewId, reviewerId, paperId, e.getMessage());
        }
    }
}
```

`ReviewerPaperServiceImpl` injects `ReviewStatusTransitionService` and calls it at the placement point defined in §3.3.

### 3.5 Transaction Semantics

**Current**: `ReviewerPaperServiceImpl` is `@Transactional(readOnly = true)` at class level (line 60).

**Change**: Override at method level on `getManuscriptForReview()`:

```java
@Override
@Transactional  // overrides class-level readOnly=true
public ManuscriptContent getManuscriptForReview(...) {
```

This is required because the method now participates in a transaction that may involve writes (via the `REQUIRES_NEW` sub-transaction). Only `getManuscriptForReview` becomes read-write; any future methods on the class remain read-only by default.

### 3.6 Failure Handling (OVERRIDE 2)

**Fail-open.** The `transitionToInProgressIfPending()` method catches and swallows all exceptions internally, logging at WARN level with full context (reviewId, reviewerId, paperId, exception message). The outer `getManuscriptForReview()` continues and returns the manuscript bytes to the reviewer.

Rationale:
1. **Consistency** — the existing reviewer audit log (`PaperAccessAuditService`) is already fail-open per the reviewer paper access feature. Having two fail modes in the same method is a design smell.
2. **Reviewer experience** — a reviewer trying to preview a paper should not get a 500 error because a secondary status-update failed. A stale PENDING status is a soft data-quality issue that self-corrects on the next successful access.

The `REQUIRES_NEW` propagation (§3.4) ensures that even if the inner transaction fails and rolls back, the outer transaction remains valid and uncommitted work is not affected.

| Scenario | Behavior |
|---|---|
| Review not found by ID (should never happen) | `ifPresent` no-op; manuscript fetch proceeds normally |
| Review already `IN_PROGRESS` or `COMPLETED` | `if (status == PENDING)` guard skips; no-op; idempotent |
| DB write fails | Caught and logged at WARN; outer transaction unaffected; reviewer gets manuscript bytes; transition retries on next access |
| `REQUIRES_NEW` transaction isolation failure | Caught by outer try/catch; logged; manuscript still returned |

### 3.7 Race Conditions

**Concurrent first-access by the same reviewer** (e.g., preview tab + download in parallel): Both transactions read `PENDING`, both try to set `IN_PROGRESS`. The second `save()` is a no-op update (same status value). `startedAt` may differ by milliseconds — acceptable. No unique constraint violation. No data corruption.

**No optimistic locking needed**: The transition is unidirectional (`PENDING` → `IN_PROGRESS`) and idempotent. The worst case is a slightly different `startedAt` timestamp, which has no business impact.

### 3.8 New Repository Method

**None needed.** `reviewRepository.findById(reviewId)` already exists via `JpaRepository`. The `reviewId` is already captured from `assertReviewerCanAccess`.

### 3.9 Observability

- Success: `"Review {} auto-transitioned PENDING → IN_PROGRESS"` at INFO level
- Failure: `"Review {} auto-transition failed (non-fatal) for reviewer {} on paper {}: {}"` at WARN level
- No audit trail entry (lightweight status signal; audit is reserved for business-significant actions like submission, completion, assignment)

### 3.10 Test Strategy for IN_PROGRESS Transition (Addendum)

**Status**: ADDED (2026-04-14) — this section was added post-approval as Phase 1B (implementation) discovered the original spec lacked test enumeration for Phase 1.2, matching the pattern resolved earlier for Section 2.8.

The IN_PROGRESS auto-transition is tested at two layers: (1) unit tests for `ReviewStatusTransitionService` covering the happy path, idempotency, not-found safety, and fail-open behavior; and (2) optional integration tests for `ReviewerPaperServiceImpl.getManuscriptForReview` covering placement and end-to-end fail-open. The service-level tests are mandatory. The integration tests are conditional on `ReviewerPaperServiceImplTest.java` already existing or being reasonable to create with the project's test conventions; if the fixture cost is too high, they are deferred to a follow-up and noted in the implementation report.

**File 1**: `src/test/java/com/shodh/sanchayan/service/ReviewStatusTransitionServiceTest.java` (new file). Uses JUnit 5 + Mockito + JUnit assertions (matching the project convention established by `CmsContentServiceTest.java` and `PaperMapperTest.java`). `@ExtendWith(MockitoExtension.class)`, `@Mock ReviewRepository reviewRepository`, `@InjectMocks ReviewStatusTransitionService service`. Builder-based `Review` entity fixtures constructed per test.

#### Test 1 — transitionToInProgressIfPending_pendingReview_updatesStatusAndStartedAt

Given a PENDING review returned by `reviewRepository.findById(reviewId)`, call `transitionToInProgressIfPending(reviewId, reviewerId, paperId)`. Verify that `reviewRepository.save` was called exactly once with a review whose status is now `IN_PROGRESS` and whose `startedAt` is within 5 seconds of `Instant.now()`. Verifies the happy path.

#### Test 2 — transitionToInProgressIfPending_inProgressReview_isNoOp

Given an `IN_PROGRESS` review returned by `findById`, call the method. Verify that `reviewRepository.save` was NEVER called. Verifies idempotency for the in-progress case — prevents the method from overwriting `startedAt` on subsequent accesses.

#### Test 3 — transitionToInProgressIfPending_completedReview_isNoOp

Given a `COMPLETED` review returned by `findById`, call the method. Verify that `reviewRepository.save` was NEVER called. Verifies idempotency for the completed case — prevents reverting a completed review.

#### Test 4 — transitionToInProgressIfPending_reviewNotFound_isNoOp

Given `reviewRepository.findById(reviewId)` returning `Optional.empty()`, call the method. Verify that `reviewRepository.save` was NEVER called and no exception is thrown. Verifies the `ifPresent` no-op path. This should never happen in production (the reviewId comes from `assertReviewerCanAccess` which already loaded the review), but the defensive check must not break.

#### Test 5 — transitionToInProgressIfPending_saveThrows_swallowsException

Given a PENDING review returned by `findById`, stub `reviewRepository.save` to throw `DataIntegrityViolationException` (or any `RuntimeException`). Call the method. Assert that no exception propagates out — the method must return normally. This is the **critical fail-open test**: if it fails, the guarantee that reviewers always receive their manuscript bytes is broken. The WARN log cannot be directly asserted without log capture infrastructure; verifying no-throw is sufficient.

**File 2**: `src/test/java/com/shodh/sanchayan/service/impl/ReviewerPaperServiceImplTest.java` (conditional). If this file already exists with the necessary mock dependencies (`ReviewAuthorizationService`, `PaperRepository`, `StorageService`, `ContentTypeDetector`, metadata strip/watermark services, `PaperAccessAuditService`), add three integration tests. If the test file does not exist and creating it requires mocking a large dependency surface beyond Phase 1B's scope, defer these tests to a follow-up and note the deferral in the implementation report.

#### Test 6 — getManuscriptForReview_onSuccess_callsTransitionService

Given a successful manuscript fetch pipeline (authorization, agreement, paper load, bytes, metadata strip, watermark all succeed), verify via Mockito that `reviewStatusTransitionService.transitionToInProgressIfPending` was called exactly once with the reviewId from `assertReviewerCanAccess`.

#### Test 7 — getManuscriptForReview_whenTransitionThrows_stillReturnsBytes

Given a `ReviewStatusTransitionService` mock that throws a `RuntimeException` from `transitionToInProgressIfPending`, verify that `getManuscriptForReview` still returns a `ManuscriptContent` with the correct bytes. This is the end-to-end fail-open verification. Note: the real `ReviewStatusTransitionService` already swallows exceptions internally per Section 3.6, so reaching this condition requires the mock to throw explicitly — but the test exists to catch a future refactor that accidentally removes the internal try/catch and lets the exception propagate up the call stack.

#### Test 8 — getManuscriptForReview_transitionCalledBetweenWatermarkAndAudit

Verify the call ordering using Mockito's `InOrder`: the watermark/metadata strip services are called BEFORE `reviewStatusTransitionService.transitionToInProgressIfPending`, and `paperAccessAuditService.logAccess` is called AFTER. This verifies the Override 1 placement from Section 3.3: the transition must happen after successful manuscript processing and before the audit log write, not at some other point in the pipeline.

---

## 4. Migration Requirements

**None.** No schema changes. `Review.startedAt` column and `ReviewStatus.IN_PROGRESS` enum value already exist.

---

## 5. Rollback Plan

| Sub-phase | Rollback |
|---|---|
| 1.1 Mapper Split | Revert `PaperMapper` methods back to single `toDetail()`. Revert `findByIdForCaller` branch. Remove `@JsonInclude(NON_NULL)` and `confidentialNotes` from DTO. Zero data impact. |
| 1.2 IN_PROGRESS | Remove `ReviewStatusTransitionService`. Remove call from `ReviewerPaperServiceImpl`. Revert `@Transactional` annotation. Reviews already transitioned to `IN_PROGRESS` remain valid — the status is already in the enum and in `ACTIVE_STATUSES`. No data cleanup needed. |

---

## 6. Implementation Sequence

```
Step 1: PaperDetailResponse.java
  - Add @JsonInclude(JsonInclude.Include.NON_NULL) + import
  - Change citationCount, downloadCount, requiredReviewers from int → Integer
  - Add confidentialNotes to ReviewSummaryResponse

Step 2: PaperMapper.java
  - Extract mapCoauthors() helper
  - Split mapCompletedReviews into two variants (with/without confidentialNotes)
  - Replace toDetail() with toAdminDetail() and toAuthorDetail()
  - Delete toDetail()

Step 3: PaperServiceImpl.java
  - Update findByIdForCaller() to branch admin → toAdminDetail, author → toAuthorDetail

Step 4: ReviewStatusTransitionService.java (NEW FILE)
  - Single public method: transitionToInProgressIfPending()
  - @Transactional(propagation = REQUIRES_NEW), fail-open

Step 5: ReviewerPaperServiceImpl.java
  - Inject ReviewStatusTransitionService
  - Add @Transactional on getManuscriptForReview() (overrides class-level readOnly)
  - Call reviewStatusTransitionService.transitionToInProgressIfPending()
    between watermark/strip completion and audit log

Step 6: Compile and verify
  - mvn compile (zero errors expected)
  - Manual smoke test: admin, author, reviewer each view a paper
```

Steps 1-3 form a single atomic commit (mapper split). Steps 4-5 form a second commit (IN_PROGRESS transition). Step 6 is verification.

---

## 7. Cross-Cutting Concerns

**Backward compatibility**: The `@JsonInclude(NON_NULL)` change means fields previously serialized as `null` will now be absent from JSON. The frontend `PaperDetail` type already marks these fields as optional (`?`), so no frontend breakage.

**Primitive → Integer boxing**: The three fields (`citationCount`, `downloadCount`, `requiredReviewers`) change from `int` to `Integer`. Lombok's `@Builder` will still accept `int` literals at call sites (autoboxing). Existing `toReviewerSafeDetail` sets `.citationCount(0)` and `.downloadCount(0)` which autobox to `Integer(0)`. These will serialize as `0` (not omitted by `NON_NULL`). If we want them omitted for reviewer, change to `null` — but keeping `0` is acceptable (reviewer sees zeros, not the real counts).

**Test impact**: No existing tests break. The mapper split produces the same output as before for admin callers. Author callers lose 4 fields they shouldn't have had. Reviewer output is unchanged.

---

## 8. Open Questions

**None.** All decisions from the Q1-Q8 round are resolved and incorporated above.

---

## 9. Deliverables Summary

| Deliverable | Type | Files Modified/Created |
|---|---|---|
| DTO annotation + field changes | Edit | `PaperDetailResponse.java` |
| Mapper split | Edit | `PaperMapper.java` |
| Branch update | Edit | `PaperServiceImpl.java` (1 line change) |
| Status transition service | **New** | `ReviewStatusTransitionService.java` |
| IN_PROGRESS transition call | Edit | `ReviewerPaperServiceImpl.java` |
| **Total** | | **4 edited + 1 new** |

No new dependencies. No migrations. No frontend changes.