Conversation
kevinxiao27
left a comment
kevinxiao27
left a comment
There was a problem hiding this comment.
couple small changes requested, generally very well thought out!
| export const TEAMS_TABLE = "biztechTeams"; | ||
| export const QR_SCANS_RECORD = "biztechQRScans"; | ||
| export const PROFILES_TABLE = "biztechProfiles"; | ||
| export const AUDIT_TABLE = "biztechAudit"; |
There was a problem hiding this comment.
j make sure to set up these tables on cloud before we merge
There was a problem hiding this comment.
sgsg, I might have to modify the sort key though
timestamp#email#record_id, since if we batch delete with the same timestamp we have a non-unique PKs now so they might get overwritten
| return this.logChange(tableName, item.id, email, "DELETE"); | ||
| }); // generate the promises | ||
|
|
||
| await Promise.all(logDeletionPromises); // execute them all at once |
There was a problem hiding this comment.
not sure that this should actually be blocking, but logging isn't a core feature and presumably we won't be deleting 50 registrations at a time. You could try using batchwrite onto the audit table as well to reduce network calls.
There was a problem hiding this comment.
It shouldn't be blocking - but the way the BE is setup (serverless lambda instead of a long-running server) makes it a lot tricker to run "background" tasks -> either we can do something pub-sub or we need to create a manual lambda which we can "invoke and forget". Both options will add some layer of complexity to a non-user-facing feature though. What do you think?
I do agree with the batchwrite to the audit table for delete many, I'll put up a change for this
| const res = await docClient.send(command); | ||
|
|
||
| if (email) { | ||
| await this.logChange(params.TableName, params.Key.id, email, "UPDATE", this.calculateDelta(before, after)); |
There was a problem hiding this comment.
this seems fairly computationally expensive (or at least will take up more compute in a serverless function call. I'll comments below on certain things.
There was a problem hiding this comment.
It is indeed expensive, and will be 2 DB reads instead of 1, which is also my rationale of making it optional (if you purposely want to log the change, you must include authorizer email in the handler).
| } | ||
|
|
||
| if (a && b && typeof a === "object") { | ||
| const aKeys = Object.keys(a).sort(); |
There was a problem hiding this comment.
Objects don't need to be sorted here. We don't need to sort either of the keys. If all keys in a ==> maps to same value in b, and both objects have the same number of keys, they'll be equivalent.
| ENVIRONMENT: ${file(../../config.${self:provider.stage}.json):ENVIRONMENT} | ||
| NODE_ENV: ${env:NODE_ENV} | ||
| WS_API_ENDPOINT: https://bwiujsprij.execute-api.us-west-2.amazonaws.com/dev | ||
| WS_API_ENDPOINT: https://bn27jq3bal.execute-api.us-west-2.amazonaws.com/prod |
There was a problem hiding this comment.
lol we can just remove this diff i believe these are benny's changes for the bizwall (shouldn't be merged into dev)
|
Testing still required before re-review |
2 participants
POC video in FE PR: ubc-biztech/bt-web-v2#290
Also:
