Journal API adjustments #1010
Merged
Journal API adjustments #1010
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## Walkthrough
This update refactors and enhances the journal submission, referee management, and email notification systems. It introduces new, strongly-typed email payloads, restructures email templates to use consolidated submission objects, updates notification payloads, and improves error handling and logging. Several controller and service methods are extended for richer metadata, improved role permissions, and robust communication workflows.
## Changes
| File(s) / Path(s) | Change Summary |
|-----------------------------------------------------------------------------------|---------------|
| `src/services/email.ts` | Deleted legacy email service module. |
| `src/services/email/email.ts`, `src/services/email/journalEmailTypes.ts` | Introduced new email service and strongly-typed journal email payloads. |
| `src/controllers/communities/submissions.ts`, `src/controllers/doi/mint.ts`,<br>`src/services/journals/JournalInviteService.ts`, `src/services/journals/JournalRefereeManagementService.ts`, `src/workers/doiSubmissionQueue.ts` | Updated import paths for email utilities and ensured email sending is awaited; improved error handling. |
| `src/controllers/journals/invites/editorInviteDecision.ts` | Standardized response payload for editor invite acceptance. |
| `src/controllers/journals/referees/inviteReferee.ts`,<br>`src/services/journals/JournalRefereeManagementService.ts`,<br>`test/integration/journals/journalRefereeManagement.test.ts` | Renamed `managerId` to `managerUserId` for clarity; updated related test cases. |
| `src/controllers/journals/submissions/index.ts`,<br>`src/services/journals/JournalSubmissionService.ts`,<br>`src/services/journals/JournalRevisionService.ts` | Enhanced revision and rejection workflows with richer metadata, notifications, and emails; added helper methods for submission metadata. |
| `src/routes/v1/journals/index.ts`,<br>`test/integration/journals/journalSubmission.test.ts` | Expanded editor role permissions for submission actions; removed test preventing chief editor from accepting submissions. |
| `src/services/Notifications/NotificationService.ts`,<br>`src/services/Notifications/notificationPayloadTypes.ts` | Updated referee invite notification payload to use `inviteToken` and `dueDateHrs`. |
| `src/templates/emails/journals/*.tsx` | Refactored all journal-related email templates to use consolidated `SubmissionExtended` and structured editor/assigner objects; removed individual submission props and direct submission links. |
| `.env.example` | Added `SENDGRID_TEMPLATE_ID_MAP` variable for email templates. |
| `src/utils.ts` | Added `prependIndefiniteArticle` utility function. |
| `src/utils/clock.ts` | Added `getRelativeTime` utility for human-readable time differences. |
| `package.json` | Removed alias for `date-fn-latest`, relying directly on `date-fns`. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant Controller
participant JournalService
participant EmailService
participant NotificationService
User->>Controller: Action (e.g., accept invite, submit revision)
Controller->>JournalService: Process action
JournalService->>JournalService: Fetch extended submission data
JournalService->>EmailService: sendEmail(payload)
EmailService-->>JournalService: Email sent/logged
JournalService->>NotificationService: emitNotification(payload)
NotificationService-->>JournalService: Notification emitted
JournalService-->>Controller: Result
Controller-->>User: ResponsePossibly related PRs
Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
desci-server/src/routes/v1/journals/index.ts (1)
155-160: Avoid magic-array repetition – extract a shared constant for editor‐action routesExactly the same
[EditorRole.ASSOCIATE_EDITOR, EditorRole.CHIEF_EDITOR]literal now appears four times. Repeating this array:
- bloats the diff every time the list changes,
- invites accidental divergence between routes,
- and slightly hurts readability.
Consider defining it once – e.g.
const EDITOR_ACTION_ROLES: EditorRole[] = [EditorRole.ASSOCIATE_EDITOR, EditorRole.CHIEF_EDITOR];– then passing it toensureJournalRole(...)everywhere.-import { EditorRole } from '@prisma/client'; +import { EditorRole } from '@prisma/client'; +// Roles allowed to perform submission-level editorial actions +const EDITOR_ACTION_ROLES: EditorRole[] = [ + EditorRole.ASSOCIATE_EDITOR, + EditorRole.CHIEF_EDITOR, +]; ... - ensureJournalRole([EditorRole.ASSOCIATE_EDITOR, EditorRole.CHIEF_EDITOR]), + ensureJournalRole(EDITOR_ACTION_ROLES),A micro-refactor like this is low-risk, keeps the file DRY, and makes future permission tweaks a one-liner.
Also applies to: 166-169, 176-179, 221-224
desci-server/src/controllers/journals/invites/editorInviteDecision.ts (1)
34-38: DRY up the picked invite fieldsBoth the accept and decline branches call
_.pick(invite, ['id', 'role', 'inviterId', 'journalId', 'decisionAt', 'accepted', 'declined']).Extract the field list into a constant to avoid duplication and the risk that one branch drifts from the other:
const logger = parentLogger.child({ module: 'Journals::EditorInviteDecisionController' }); +const INVITE_RESPONSE_FIELDS: Array<keyof Invite> = [ + 'id', + 'role', + 'inviterId', + 'journalId', + 'decisionAt', + 'accepted', + 'declined', +]; + ... - return sendSuccess( - res, - { invite: _.pick(invite, ['id', 'role', 'inviterId', 'journalId', 'decisionAt', 'accepted', 'declined']) }, - 'Editor invitation accepted successfully.', - ); + return sendSuccess( + res, + { invite: _.pick(invite, INVITE_RESPONSE_FIELDS) }, + 'Editor invitation accepted successfully.', + ); ... - invite: _.pick(invite, ['id', 'role', 'inviterId', 'journalId', 'decisionAt', 'accepted', 'declined']), + invite: _.pick(invite, INVITE_RESPONSE_FIELDS),Makes both paths symmetrical, keeps the literal list in one place, and eases future field changes.
Also applies to: 44-48
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
desci-server/src/controllers/journals/invites/editorInviteDecision.ts(1 hunks)desci-server/src/routes/v1/journals/index.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
desci-server/src/controllers/journals/invites/editorInviteDecision.ts (1)
desci-server/src/core/api.ts (1)
sendSuccess(5-8)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Cursor BugBot
- GitHub Check: build-and-test
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
🔭 Outside diff range comments (2)
desci-server/src/services/journals/JournalInviteService.ts (1)
68-69: Missingcryptoimport – compile-time failure
crypto.randomUUID()is used to generate the invite token, butcryptois not imported in this file.
Add:import crypto from 'node:crypto';(or
import * as crypto from 'crypto') near the top.desci-server/src/services/journals/JournalSubmissionService.ts (1)
186-206: Check that the target editor exists before dereferencingeditor.user
editorcan benullwhen an invalideditorIdis supplied for the journal.
Subsequent lines accesseditor.user.*, which will throw and skip the intended error handling.- const editor = await prisma.journalEditor.findFirst({...}); - // ... - await NotificationService.emitOnJournalSubmissionAssignedToEditor({ editor }); + const editor = await prisma.journalEditor.findFirst({...}); + if (!editor) { + throw new NotFoundError('Editor not found for this journal'); + }
🧹 Nitpick comments (25)
.env.example (2)
67-69: Clarify expected format forSENDGRID_TEMPLATE_ID_MAP.Storing a “map” in a single env-var is brittle unless the consumers parse a well-defined format (e.g. JSON or
key1=id1,key2=id2). Add a short comment describing the exact syntax and required escaping rules, otherwise onboarding & CI configuration will be error-prone.
201-202: Remove superfluous blank lines to silence dotenv-linter.- -Only one trailing newline is sufficient.
desci-server/src/utils.ts (1)
309-319: Edge-cases missing inprependIndefiniteArticle.The simple vowel-check fails for words whose pronunciation starts with a vowel sound but a consonant letter (“hour”, “MBA”, “honest”, etc.). If email copy may include such cases, consider switching to a small phonetic helper (e.g.
an-array) or at least add a regex for common exceptions.Implementation is otherwise fine.
desci-server/src/services/Notifications/NotificationService.ts (1)
608-616: Clarify and document the newdueDateHrssemanticSwitching from an absolute
Dateto a relativedueDateHrs: numberhelps avoid TZ problems, but leaves room for mis-interpretation (is it “hours from creation”, “working hours”, etc.).
Add a short JSDoc or comment here (and in the corresponding type) that states precisely:• the reference point (invite creation vs. now)
• that it is UTC hours, not local-time hours.This will save downstream consumers from off-by-X-hours bugs.
Also applies to: 622-640
desci-server/src/services/journals/JournalInviteService.ts (1)
95-106: Consider propagating email-send failures
sendEmailis correctly awaited, but any rejection is swallowed by the outertry/catch, logged, and the function still returns the invite as “successful”.
If email delivery is business-critical, re-throw or surface a 5xx so the caller can retry, otherwise note explicitly that silent failures are acceptable.desci-server/src/controllers/journals/submissions/index.ts (1)
329-330: Misleading log contextThe log tag uses
'acceptSubmission', but we are inrejectSubmissionController, which will confuse log searches.- logger.error({ fn: 'acceptSubmission', error: e, submissionId }, 'Notification push failed'); + logger.error({ fn: 'rejectSubmission', error: e, submissionId }, 'Notification push failed');desci-server/src/templates/emails/journals/FinalRejectionDecision.tsx (1)
37-40: Empty link breaks CTAButton
href={''}renders as an anchor with empty URL → clicking reloads inbox page.
Either supply a meaningful link (e.g. author guidelines / submission criteria) or remove the button altogether to avoid user confusion.desci-server/src/templates/emails/journals/RefereeDeclinedEmail.tsx (2)
1-1: Remove unused imports to prevent dead-code + TS lint noise
EditorRoleandJournalSubmissionare not referenced anywhere in this file after the refactor.
Leaving them in will trigger eslint/ts-server “unused import” diagnostics and bloats the bundle.-import { EditorRole, Journal, JournalSubmission } from '@prisma/client'; +import { Journal } from '@prisma/client';
44-46: Guard against missing / malformed author arrays
submission.authors.join(', ')will throw ifauthorsisnull,undefined, or not an array (possible when data comes from Prisma raw JSON fields).
Consider a safe fallback:-<Text className="text-md text-center">Authors: {submission.authors.join(', ')}</Text> +<Text className="text-md text-center"> + Authors: {(submission.authors ?? []).join(', ') || 'N/A'} +</Text>desci-server/src/templates/emails/journals/RefereeReviewReminder.tsx (1)
29-31: Null-safety forauthorsarrayHandle potential
null/undefinedas suggested previously to avoid runtime crashes in production emails.desci-server/src/templates/emails/journals/OverdueAlertEditor.tsx (1)
32-35: Duplicate authors join() safetyAs in other templates, wrap
submission.authorswith a fallback to prevent crashes if data is malformed.desci-server/src/templates/emails/journals/ExternalRefereeInvite.tsx (2)
10-13:refereeNameprop is declared but never usedThe value is passed in from the email service yet ignored inside the template. Either include the name in the copy (e.g., greeting) or remove the prop to keep the payload minimal.
33-36: Author list null-safetyApply the same guarded join pattern to avoid runtime errors if
authorsis not an array.desci-server/src/templates/emails/journals/RefereeAccepted.tsx (1)
1-3: UnusedEditorRoleimport
EditorRoleis imported but never referenced after the refactor. Remove it to avoid a TypeScriptno-unused-varserror and keep bundles lean.desci-server/src/templates/emails/journals/RevisionSubmittedConfirmation.tsx (1)
1-3: Remove orphanedEditorRoleimport
EditorRoleis no longer used in this file.desci-server/src/templates/emails/journals/RefereeReassigned.tsx (1)
12-13:refereeEmailprop is declared but never usedEither utilise this value in the email body (e.g., “({refereeEmail})”) or drop the prop entirely to avoid dead code.
desci-server/src/templates/emails/journals/SubmissionReassigned.tsx (2)
10-17: Unusededitorprop
editoris defined inSubmissionReassignedEmailPropsbut never referenced, leading to an unused variable warning and unnecessary payload bloat. Remove it or incorporate it into the email copy.- editor: { - name: string; - userId: number; - };
1-3: ExtraneousEditorRoleimport
EditorRoleisn’t used after the refactor—safe to delete.desci-server/src/templates/emails/journals/MajorRevisionRequest.tsx (1)
5-5: Import extension may break TS buildsUsing
.jsin a TSX file can confuse TypeScript path resolution in some tsconfig setups. Prefer importing the.tssource (compiler will rewrite) or omit the extension.desci-server/src/templates/emails/journals/SubmissionAssigned.tsx (2)
1-1: Unused import
EditorRoleis not referenced; remove to keep the bundle lean and silence linters.
10-14: Dead prop:editorThe
editorprop is declared but never used in the component. Drop it (and its callers) or render relevant info to avoid stale API surface.desci-server/src/templates/emails/journals/RefereeInvite.tsx (1)
1-1: Remove unusedEditorRoleimportIt isn’t referenced.
desci-server/src/templates/emails/journals/SubmissionAcceped.tsx (1)
10-14: UnusededitorpropDeclared but unused; prune or surface editor info.
desci-server/src/services/email/email.ts (1)
32-38: Nested ternary hurts readabilityConsider extracting
deploymentEnvironmentStringinto a small helper with a switch for clarity and easier extension.desci-server/src/services/journals/JournalSubmissionService.ts (1)
368-378: Variable shadowing makes the code harder to follow
const submission = …is declared twice (outer scope and inside the try-block).
Rename the inner one (e.g.fullSubmission) to avoid accidental misuse.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
.env.example(2 hunks)desci-server/src/controllers/communities/submissions.ts(1 hunks)desci-server/src/controllers/doi/mint.ts(1 hunks)desci-server/src/controllers/journals/referees/inviteReferee.ts(1 hunks)desci-server/src/controllers/journals/submissions/index.ts(3 hunks)desci-server/src/services/Notifications/NotificationService.ts(2 hunks)desci-server/src/services/Notifications/notificationPayloadTypes.ts(1 hunks)desci-server/src/services/email.ts(0 hunks)desci-server/src/services/email/email.ts(1 hunks)desci-server/src/services/email/journalEmailTypes.ts(1 hunks)desci-server/src/services/journals/JournalInviteService.ts(2 hunks)desci-server/src/services/journals/JournalRefereeManagementService.ts(11 hunks)desci-server/src/services/journals/JournalRevisionService.ts(3 hunks)desci-server/src/services/journals/JournalSubmissionService.ts(12 hunks)desci-server/src/templates/emails/journals/DeskRejection.tsx(1 hunks)desci-server/src/templates/emails/journals/ExternalRefereeInvite.tsx(2 hunks)desci-server/src/templates/emails/journals/FinalRejectionDecision.tsx(1 hunks)desci-server/src/templates/emails/journals/MajorRevisionRequest.tsx(1 hunks)desci-server/src/templates/emails/journals/MinorRevisionRequest.tsx(1 hunks)desci-server/src/templates/emails/journals/OverdueAlertEditor.tsx(1 hunks)desci-server/src/templates/emails/journals/RefereeAccepted.tsx(1 hunks)desci-server/src/templates/emails/journals/RefereeDeclinedEmail.tsx(3 hunks)desci-server/src/templates/emails/journals/RefereeInvite.tsx(2 hunks)desci-server/src/templates/emails/journals/RefereeReassigned.tsx(2 hunks)desci-server/src/templates/emails/journals/RefereeReviewReminder.tsx(2 hunks)desci-server/src/templates/emails/journals/RevisionSubmittedConfirmation.tsx(1 hunks)desci-server/src/templates/emails/journals/SubmissionAcceped.tsx(1 hunks)desci-server/src/templates/emails/journals/SubmissionAssigned.tsx(1 hunks)desci-server/src/templates/emails/journals/SubmissionReassigned.tsx(2 hunks)desci-server/src/utils.ts(1 hunks)desci-server/src/utils/clock.ts(2 hunks)desci-server/src/workers/doiSubmissionQueue.ts(1 hunks)desci-server/test/integration/journals/journalSubmission.test.ts(0 hunks)
💤 Files with no reviewable changes (2)
- desci-server/test/integration/journals/journalSubmission.test.ts
- desci-server/src/services/email.ts
✅ Files skipped from review due to trivial changes (4)
- desci-server/src/workers/doiSubmissionQueue.ts
- desci-server/src/controllers/doi/mint.ts
- desci-server/src/controllers/journals/referees/inviteReferee.ts
- desci-server/src/services/email/journalEmailTypes.ts
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 202-202: [ExtraBlankLine] Extra blank line detected
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (5)
desci-server/src/controllers/communities/submissions.ts (1)
20-20: Path change looks correct – verify runtime resolution.The project imports transpiled
.jsfiles; ensuredesci-server/src/services/email/email.tsis emitted toemail.jsin the same folder so Node can resolve it in production.desci-server/src/services/Notifications/notificationPayloadTypes.ts (1)
117-119: Renamed fields require cascading updates.
dueDate→dueDateHrsand newinviteTokenwill break existing code & templates that still expect aDate. Please grep fordueDatein referee-invite contexts and update all usages.#!/bin/bash # find stale usages outside this file rg -n "dueDate[^A-Za-z]" | grep -v notificationPayloadTypes.tsdesci-server/src/services/email/email.ts (1)
162-173: Environment check uses non-standard value
NODE_ENVis usually"development", not"dev". Ensure the condition matches your deployment conventions or make it inclusive (['dev', 'development'].includes(...)).desci-server/src/services/journals/JournalSubmissionService.ts (1)
441-447: Potential runtime whenassignedEditor.useris undefinedThe service trusts that every
journalEditorhas a linkeduser.
Insert a guard or add a NOT NULL join in the query to avoidCannot read property 'name'….desci-server/src/services/journals/JournalRefereeManagementService.ts (1)
34-40:crypto.randomUUID()used without import – may fail in older Node runtimesAdd
import crypto from 'crypto';(or userandomUUIDfrom"node:crypto") to satisfy TS type-checking and non-global environments.
Comment on lines
+77
to
+83
| type: EmailTypes.REVISION_SUBMITTED, | ||
| payload: { | ||
| email: submission.assignedEditor.email, | ||
| journal: submission.journal, | ||
| submission: submissionExtended, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
assignedEditor.email is undefined
assignedEditor is a JournalEditor, which has no email field.
Fetch the linked User or reuse the editor object already queried:
const editorUser = await prisma.user.findUnique({ where: { id: submission.assignedEditorId } });
await sendEmail({
type: EmailTypes.REVISION_SUBMITTED,
payload: {
email: editorUser.email,
journal: submission.journal,
submission: submissionExtended,
},
});🤖 Prompt for AI Agents
In desci-server/src/services/journals/JournalRevisionService.ts around lines 77
to 83, the code incorrectly accesses assignedEditor.email, but assignedEditor
lacks an email field. To fix this, fetch the User linked to assignedEditor by
querying prisma.user with assignedEditorId, then use the retrieved user's email
in the payload for sendEmail instead of assignedEditor.email.
Comment on lines
43
to
86
| try { | ||
| // Notification logic | ||
| const submission = await prisma.journalSubmission.findUnique({ | ||
| where: { id: submissionId }, | ||
| include: { | ||
| journal: true, | ||
| author: true, | ||
| assignedEditor: true, | ||
| }, | ||
| }); | ||
|
|
||
| const editor = await prisma.journalEditor.findUnique({ | ||
| where: { | ||
| userId_journalId: { | ||
| userId: submission.assignedEditorId, | ||
| journalId, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const submissionExtendedResult = await journalSubmissionService.getSubmissionExtendedData(submissionId); | ||
| if (submissionExtendedResult.isErr()) { | ||
| throw new Error('Failed to get submission extended data'); | ||
| } | ||
| const submissionExtended = submissionExtendedResult.value; | ||
|
|
||
| await NotificationService.emitOnRevisionSubmittedToEditor({ | ||
| journal: submission.journal, | ||
| editor, | ||
| submission: submission, | ||
| submissionTitle: submissionExtended.title, | ||
| author: submission.author, | ||
| }); | ||
| sendEmail({ | ||
| type: EmailTypes.REVISION_SUBMITTED, | ||
| payload: { | ||
| email: submission.assignedEditor.email, | ||
| journal: submission.journal, | ||
| submission: submissionExtended, | ||
| }, | ||
| }); | ||
| } catch (e) { | ||
| logger.error({ fn: 'createRevision', error: e, submissionId }, 'Notification push failed'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against unassigned submissions
If submission.assignedEditorId is null, the subsequent queries will throw.
Add an early check and return an err(...) so the caller sees a sensible error instead of a 500.
if (!submission?.assignedEditorId) {
throw new Error('Submission has no assigned editor');
}🤖 Prompt for AI Agents
In desci-server/src/services/journals/JournalRevisionService.ts between lines 43
and 86, the code assumes submission.assignedEditorId is always set, but if it is
null, subsequent queries will fail and cause a 500 error. Add an early check
after fetching the submission to verify submission.assignedEditorId is not null
or undefined. If it is missing, return an error or throw a new Error with a
clear message like 'Submission has no assigned editor' to provide a sensible
error response to the caller.
Comment on lines
+272
to
+284
| include: { | ||
| journal: true, | ||
| author: true, | ||
| assignedEditor: true, | ||
| }, | ||
| }); | ||
|
|
||
| const submissionExtendedResult = await journalSubmissionService.getSubmissionExtendedData(submissionId); | ||
| if (submissionExtendedResult.isErr()) { | ||
| throw new Error('Failed to get submission extended data'); | ||
| } | ||
| const submissionExtended = submissionExtendedResult.value; | ||
|
|
There was a problem hiding this comment.
Missing null-guard for assignedEditor in acceptance path
submission.assignedEditor is assumed present, yet the DB relation is optional.
Add an explicit check similar to:
if (!submission.assignedEditor) {
throw new Error('Assigned editor record missing');
}🤖 Prompt for AI Agents
In desci-server/src/services/journals/JournalSubmissionService.ts around lines
272 to 284, the code assumes submission.assignedEditor is always present, but
since the database relation is optional, you need to add a null check for
assignedEditor. Insert an explicit check after fetching submission to verify if
assignedEditor exists, and if not, throw an error with a clear message like
'Assigned editor record missing' to handle this case safely.
desci-server/src/services/journals/JournalRefereeManagementService.ts
Outdated
Show resolved
Hide resolved
desci-server/src/services/journals/JournalRefereeManagementService.ts
Outdated
Show resolved
Hide resolved
desci-server/src/services/journals/JournalRefereeManagementService.ts
Outdated
Show resolved
Hide resolved
… compatible with past dates
…crash if no author arr
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
desci-server/src/controllers/admin/metrics/publishMetrics.ts (1)
56-62: Shadowed variable hides outer scope value
guestSignUpSuccessRateis redeclared inside theifblock, shadowing the variable defined earlier (lines 42-47). This can confuse readers and static–analysis tools. Rename the inner variable to make its intent explicit.- const [prevTotalUsers, prevPublishers, prevPublishersInCommunity, guestSignUpSuccessRate] = await Promise.all([ + const [ + prevTotalUsers, + prevPublishers, + prevPublishersInCommunity, + prevGuestSignUpSuccessRate, + ] = await Promise.all([ @@ - guestSignUpSuccessRate, + guestSignUpSuccessRate: prevGuestSignUpSuccessRate,
♻️ Duplicate comments (3)
desci-server/src/services/journals/JournalSubmissionService.ts (3)
259-264: Error masking persists – propagate or surface partial failure
Same issue flagged in previous review: the catch block still only logs the error and returns success to the caller, silently hiding notification/email failures.
Please propagate the error (or return an object that indicates partial success) so API clients can retry or alert.
324-326: Null-guard forassignedEditorstill missing
submission.assignedEditoris queried but never null-checked. When the relation is optional, this will throw in production for submissions created pre-assignment.- name: submission.assignedEditor.name, - userId: submission.assignedEditor.id, + name: submission.assignedEditor?.name ?? 'Unknown editor', + userId: submission.assignedEditor?.id ?? 0,Better: throw a domain error earlier if
assignedEditoris absent.
535-538:targetVersionIndexcan be negative – add bounds check
submission.versionis treated as 1-based while array indices are 0-based. For the first version this yieldslength - 1, but for any higher version the index can become negative and crash.- const targetVersionIndex = researchObject.versions.length - submission.version; - const targetVersion = researchObject.versions[targetVersionIndex]; + const targetVersionIndex = + researchObject.versions.length - submission.version; + if (targetVersionIndex < 0 || targetVersionIndex >= researchObject.versions.length) { + return err(new Error('Requested version out of bounds')); + } + const targetVersion = researchObject.versions[targetVersionIndex];
🧹 Nitpick comments (2)
desci-server/src/controllers/admin/metrics/publishMetrics.ts (1)
32-35: Minor typo in comment
previouse→previous– helps keep inline docs tidy.-// hard check the from and to dates are valid, to prevent previouse period from being bogus ... +// Hard-check the from and to dates are valid to prevent previous period from being bogus ...desci-server/src/services/journals/JournalSubmissionService.ts (1)
406-414: Variable shadowing & redundant queryInside the
requestRevisiontry-block you re-querysubmissionand shadow the outer const.
This increases query load and makes debugging harder.- // Notification logic - const submission = await prisma.journalSubmission.findUnique({ + // Refresh with relations needed for notifications + const freshSubmission = await prisma.journalSubmission.findUnique({ where: { id: submissionId }, include: { journal: true, author: true, }, });Use
freshSubmission(or rename the original) to avoid shadowing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
desci-server/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
desci-server/package.json(0 hunks)desci-server/src/controllers/admin/analytics.ts(1 hunks)desci-server/src/controllers/admin/metrics/featureAdoptionMetrics.ts(1 hunks)desci-server/src/controllers/admin/metrics/publishMetrics.ts(1 hunks)desci-server/src/controllers/admin/metrics/userEngagements.ts(1 hunks)desci-server/src/controllers/doi/mint.ts(2 hunks)desci-server/src/controllers/journals/submissions/index.ts(3 hunks)desci-server/src/services/interactionLog.ts(1 hunks)desci-server/src/services/journals/JournalRefereeManagementService.ts(9 hunks)desci-server/src/services/journals/JournalRevisionService.ts(3 hunks)desci-server/src/services/journals/JournalSubmissionService.ts(12 hunks)desci-server/src/utils/clock.ts(2 hunks)desci-server/src/workers/doiSubmissionQueue.ts(2 hunks)desci-server/test/integration/analytics.test.ts(1 hunks)desci-server/test/integration/util.test.ts(1 hunks)desci-server/test/util.ts(1 hunks)
💤 Files with no reviewable changes (1)
- desci-server/package.json
✅ Files skipped from review due to trivial changes (7)
- desci-server/src/controllers/admin/metrics/featureAdoptionMetrics.ts
- desci-server/src/controllers/admin/analytics.ts
- desci-server/test/integration/util.test.ts
- desci-server/test/util.ts
- desci-server/src/services/interactionLog.ts
- desci-server/src/controllers/admin/metrics/userEngagements.ts
- desci-server/test/integration/analytics.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- desci-server/src/workers/doiSubmissionQueue.ts
- desci-server/src/controllers/doi/mint.ts
- desci-server/src/utils/clock.ts
- desci-server/src/controllers/journals/submissions/index.ts
- desci-server/src/services/journals/JournalRevisionService.ts
- desci-server/src/services/journals/JournalRefereeManagementService.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (1)
desci-server/src/controllers/admin/metrics/publishMetrics.ts (1)
1-1: Correct import – LGTM
Switching to the canonicaldate-fnspackage resolves the broken import path.
Comment on lines
+215
to
+221
| const editor = await prisma.journalEditor.findFirst({ | ||
| where: { userId: payload.editorId, journalId: submission.journalId }, | ||
| include: { | ||
| user: true, | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Guard against a missing editor record before continuing
editor can be null if the supplied editorId does not map to an editor of the target journal.
All subsequent notification / email logic relies on editor.user. Add an explicit check and fail fast.
const editor = await prisma.journalEditor.findFirst({
where: { userId: payload.editorId, journalId: submission.journalId },
include: {
user: true,
},
});
+ if (!editor) {
+ throw new NotFoundError('Editor record not found for this journal');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const editor = await prisma.journalEditor.findFirst({ | |
| where: { userId: payload.editorId, journalId: submission.journalId }, | |
| include: { | |
| user: true, | |
| }, | |
| }); | |
| const editor = await prisma.journalEditor.findFirst({ | |
| where: { userId: payload.editorId, journalId: submission.journalId }, | |
| include: { | |
| user: true, | |
| }, | |
| }); | |
| if (!editor) { | |
| throw new NotFoundError('Editor record not found for this journal'); | |
| } |
🤖 Prompt for AI Agents
In desci-server/src/services/journals/JournalSubmissionService.ts around lines
215 to 221, the code fetches an editor record that may be null if the editorId
does not correspond to an editor of the journal. Add an explicit check after the
prisma query to verify if editor is null, and if so, throw an error or return
early to fail fast before any notification or email logic that uses editor.user
is executed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
desci-server/src/services/journals/JournalManagementService.ts (1)
146-161: Consider using a cleaner type definition approach.The intersection type approach works but could be simplified for better maintainability.
Consider this cleaner approach:
-}> & { - editors: Array< - Prisma.JournalEditorGetPayload<{ - include: { - user: { - select: { - id: true; - name: true; - email: true; - orcid: true; - }; - }; - }; - }> & { currentWorkload: number } - >; -}; +}>; +export type JournalDetailsWithWorkload = JournalDetails & { + editors: Array<JournalDetails['editors'][0] & { currentWorkload: number }>; +};Then update the function return type accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
desci-server/src/services/journals/JournalManagementService.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (1)
desci-server/src/services/journals/JournalManagementService.ts (1)
8-8: LGTM! Import addition is appropriate.The
SubmissionStatusimport is correctly added and used in the workload calculation logic.
…rnal-api-adjustments
… journal-api-adjustments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
Tests