-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
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.
This looks great! The only other thing I'd add is that I noticed in the code coverage output it's not exercising deleting the "user studies" user/{uid}/studies
subcollection:
------------|---------|----------|---------|---------|-------------------
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
------------|---------|----------|---------|---------|-------------------
All files | 93.84 | 87.5 | 100 | 93.84 |
index.ts | 93.75 | 87.5 | 100 | 93.75 | 129-134
studies.ts | 100 | 100 | 100 | 100 |
------------|---------|----------|---------|---------|-------------------
Which is this bit:
rally-web-platform/functions/src/index.ts
Lines 124 to 142 in 35279a3
// There will be one document per study here, use batching in case it ever goes over the limit. | |
// Work in batches of 5: https://firebase.google.com/docs/firestore/manage-data/transactions#security_rules_limits | |
let batch = admin.firestore().batch(); | |
const userStudyDocs = await collectionRef.get(); | |
for (const [count, userStudyDoc] of userStudyDocs.docs.entries()) { | |
batch.delete(userStudyDoc.ref); | |
// Count is 0-based, so commit on multiples of 4. | |
if (count % 4 === 0) { | |
await batch.commit(); | |
batch = admin.firestore().batch(); | |
} | |
} | |
// Do a final commit in case we ended on a partial batch. | |
await batch.commit(); | |
// Finally, delete the user document. | |
await admin.firestore().collection("users").doc(user.uid).delete(); |
This is a subcollection of /users/{uid}
and is used to store the list of studies that the user has consented to, for instance if I join the facebookPixelHunt
study then I would have a document in /users/{uid}/studies/facebookPixelHunt
. Both the website and the extension watch this document to see if consent has been recorded for this study.
You can see where the website modifies this:
rally-web-platform/src/lib/stores/api.js
Lines 70 to 73 in 35279a3
async function updateUserStudiesCollection(studyId, updates, merge = true) { | |
const userStudyref = doc(db, "users", firebaseUid, "studies", studyId); | |
await setDoc(userStudyref, updates, { merge }); | |
} |
Unfortunately, Firebase cannot recursively delete subcollections, and it must be done in batches in case the number of documents gets large in the future, so having test coverage here would be especially helpful :)
Would you mind adding a test for this? Ideal would be to add a number of documents over the batch limit (it works in batches of 4) to ensure that there isn't a bug introduced here.
The consequence of getting this wrong isn't too dire, but it does mean useless garbage builds up in Firestore which we'd like to avoid, and we also want to honor the user's request to delete all of their data.
Rename index to studyKey in loadFirestore / addRallyStudyToFirestore. (#254) * Rename index to studyKey in loadFirestore / addRallyStudyToFirestore. * Corrected import in add-studies-to-firebase and renamed index -> studyId Merge branch 'master' into functions-test Resolving comments
npm run dev
OR
npm run test:functions
(this automatically starts the emulator).