-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Save reports #16
base: main
Are you sure you want to change the base?
Save reports #16
Conversation
changed package to allow for running backend and frontend in parallel
Added pre commit prettier hook
Co-authored-by: Rachel Ma <ma.ra@northeastern.edu>
Implemented submit card hookup and global user context
…into save-reports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good! Just one question about why we are changing the report into comment schema, and if we can treat them as their own field.
@@ -29,6 +35,19 @@ export const CommentSchema = z.object({ | |||
|
|||
export type CommentSchema = z.infer<typeof CommentSchema>; | |||
|
|||
export const ReportSchema = z.object({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also have a UUID field in the report schema as well - it's an easy way to have a unique identifier for each comment/report. Otherwise, looks good!
getAllActiveCommentsOfType(CommentType.Report, comments).map((comment) => ({ | ||
AuthorID: comment.AuthorID, | ||
Type: comment.Type, | ||
Content: comment.Content.split(",")[0], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be careful using ',' as a delimiter here, since it wouldn't surprise me for the Explanation
to include commas. Could you use something less frequently use to join and split the content, even just a semi-colon?
AuthorID: report.AuthorID, | ||
Type: report.Type, | ||
Timestamp: report.Timestamp, | ||
Content: `${report.Content},${report.Notified},${report.Explanation}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason why we need to try and turn reports into comments? Can we just fully separate out the reports and comments in the fields?
Files changed:
apps/backend/src/db/timesheets/EntryOperations.ts
apps/frontend/src/components/TimeCardPage/CellTypes/CommentCell.tsx
apps/frontend/src/components/TimeCardPage/CellTypes/CommentModals/ShowReportModal.tsx
apps/frontend/src/components/TimeCardPage/TimeTableRow.tsx