Mobile App Sync api update#178
Conversation
…'s assigned projects.
| const getUserChapterAssignmentsRoute = createRoute({ | ||
| tags: ['Projects - Chapter Assignments'], | ||
| method: 'get', | ||
| path: '/project/member/{userId}/chapter-assignments', |
There was a problem hiding this comment.
This route deviates from the established patterns in the codebase. Since we are fetching the chapter assignments for a user, we should move this to the user domain and user a route like: /users/{userId}/chapter-assignments/all or /users/{userId}/chapter-assignments/sync or similar.
| ), | ||
| [HttpStatusCodes.BAD_REQUEST]: jsonContent( | ||
| createMessageObjectSchema('Bad Request'), | ||
| 'Invalid request body (chapters array empty or exceeds 200)' |
There was a problem hiding this comment.
| 'Invalid request body (chapters array empty or exceeds 1200)' |
| .where( | ||
| excludeProjectIds.length === 0 && updatedAfter | ||
| ? and( | ||
| inArray(project_units.projectId, projectIds), | ||
| gt(chapter_assignments.updatedAt, updatedAfter) | ||
| ) | ||
| : or( | ||
| inArray( | ||
| project_units.projectId, | ||
| projectIds.filter((id) => !excludeProjectIds.includes(id)) | ||
| ), | ||
| excludeProjectIds.length > 0 && updatedAfter | ||
| ? and( | ||
| inArray(project_units.projectId, excludeProjectIds), | ||
| gt(chapter_assignments.updatedAt, updatedAfter) | ||
| ) | ||
| : undefined | ||
| ) |
There was a problem hiding this comment.
Complex, hard-to-reason repository logic.
The ternary where clause in getByProjects tries to handle excludeProjectIds + updatedAfter in a single expression. It produces or(x, undefined) when excludeProjectIds.length > 0 && !updatedAfter, which is brittle. Extracting explicit conditions into a builder variable or using an array filter before passing to and()/or() would be much cleaner and safer.
| export const memberChapterAssignmentResponseSchema = z.object({ | ||
| chapterAssignmentId: z.number().int(), | ||
| projectId: z.number().int(), | ||
| projectUnitId: z.number().int(), | ||
| bibleId: z.number().int(), | ||
| bookId: z.number().int(), | ||
| chapterNumber: z.number().int(), | ||
| assignedUserId: z.number().int().nullable(), | ||
| peerCheckerId: z.number().int().nullable(), | ||
| status: z.string(), | ||
| submittedTime: z.string().nullable(), | ||
| createdAt: z.string().nullable(), | ||
| updatedAt: z.string().nullable(), | ||
| }); |
There was a problem hiding this comment.
Date schema inconsistency
This memberChapterAssignmentResponseSchema uses z.string().nullable() for timestamps, while the adjacent chapterAssignmentProgressResponseSchema (in the same file) uses z.date().nullable(). If the JSON response actually contains ISO strings, the z.date() schemas elsewhere may fail client-side validation depending on the OpenAPI generator. Align on one representation.
This also should be consistent even if we move this logic to the user domain. DB types are consistent and the code should reflect that.
| return ok({ | ||
| syncedAt: new Date().toISOString(), | ||
| data: toBulkChapterTextResponses(result.data), | ||
| }); |
There was a problem hiding this comment.
This is a minor deviation from the return signature that the codebase follows. While this is not a big deal, it prompts the question of whether we should consider keeping mobile-specific logic in a src/services/mobile-sync.service.ts and the data queries in their respective domains. This is not necessary for this PR but, if there are more sync endpoints that will be implemented, it is worth discussion.
2 participants
Things covered:
#issue 36