Add Interactions Middleware #163
Conversation
…els for interactions
…pe compatibility issues
sthuray
left a comment
sthuray
left a comment
There was a problem hiding this comment.
Great job with this! A few requests:
-
Update the table schemas to match the ones in this ticket (this one is more consistent with our other tables, and more thorough in the type declaration): https://github.com/uwblueprint/humane-society/pull/107/files#diff-415d2f577bee729ff37cde4ecd68acc5177abd43148e5f79823839ee741ecdd7
-
the logInteraction file could use some error handling if it doesn't receive all expected values. Example: if "TASK_ASSIGNEE_CHANGED" is the interaction type, you expect to receive both "oldUserName" and "newUserName", but "newUserName" is null, then throw an error. (instead of using defaults with ?? in some places)
| @@ -1,5 +1,3 @@ | |||
| version: "3.7" | |||
| break; | ||
|
|
||
| case InteractionTypeEnum.CHANGED_PET_NAME: | ||
| short_description = `Changed ${oldUserName}'s name to ${newUserName}`; |
There was a problem hiding this comment.
Might be a good idea to add some verification here just so if newUserName is undefined, we won't get
Changed ${oldUserName}'s name to undefined
| const targetUserId = | ||
| interactionType.includes("USER") && !interactionType.includes("TASK") ? targetId : null; | ||
| const targetPetId = interactionType.includes("PET") ? targetId : null; | ||
| const targetTaskId = interactionType.includes("TASK_") ? targetId : null; |
There was a problem hiding this comment.
Aren't there cases where this give targetTaskId the petId?
he-is-harry
left a comment
he-is-harry
left a comment
There was a problem hiding this comment.
Leaving some comments from my pass of this PR, looks good overall, just some suggestions to make.
As a more general note, when I was testing the change I noticed that the interaction_types table requires these InteractionTypeEnum values to be inserted before the logging will work. I think it might be beneficial to create a migration that inputs the values in, especially since these enum values are from our development side.
|
|
||
| // defineRelationships(); | ||
|
|
||
| const InteractionType = InteractionTypeModel(sequelize, DataTypes); |
There was a problem hiding this comment.
I wonder if we can define these interaction and interaction type models in a more similar way to what is done for the other types, it might remove the need for these lines here?
There was a problem hiding this comment.
I believe these files here were just added by mistake, I think they can be removed as there are no dependencies on them.
| }; | ||
| }; | ||
|
|
||
| export enum InteractionTypeEnum { |
There was a problem hiding this comment.
There seems to be a bit of duplication between the different enum types. For example, ASSIGNED_TASK is not used and TASK_ASSIGNED is used instead, but then STARTED_TASK is used below while TASK_STARTED is not used. We should probably remove the duplicates here that are not used and consolidate the logic, I would probably remove the ones above in all caps since they don't match the pattern
There was a problem hiding this comment.
Same as this the package.json comment above, I believe this one can be removed
|
@he-is-harry Thanks for your review. Just fix merge conflicts when you get the chance. |
6 participants
Notion ticket link
Interactions Middleware Ticket
Implementation description
Steps to test
What should reviewers focus on?
Checklist