Mobile App Sync api update#178
Conversation
…'s assigned projects.
| .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.
There was a problem hiding this comment.
Agreed. For reference, we currently have two sync-oriented endpoints: one for chapter assignments and one for bulk Bible texts. While they were introduced to support the mobile sync workflow, they are not limited to mobile clients.
The intent is to use the same endpoints for both full and incremental syncs. An initial sync retrieves the complete dataset, while subsequent syncs use the updatedAfter parameter to fetch only records that have changed since the last sync.
2 participants
Things covered:
#issue 36