-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: ✨ Catfishes know about other catfishes #264
base: development
Are you sure you want to change the base?
Changes from all commits
5757927
6299f0f
187db5a
f212b40
bc3ee58
e9dab4b
ce43550
8d3b19a
349a320
bbad02f
765dc3d
b77ac4a
a56d13d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,17 @@ service cloud.firestore { | |
} | ||
|
||
match /privatePlayers/{uid} { | ||
|
||
function isCatfish(){ | ||
return get(/databases/$(database)/documents/lobbies/$(code)/privatePlayers/$(request.auth.uid)).data.role == "CATFISH"; | ||
} | ||
|
||
function isCatfishResource(){ | ||
return resource.data.role == "CATFISH" | ||
} | ||
|
||
allow get: if request.auth.uid == uid; | ||
allow list: if isCatfish() && isCatfishResource(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we really want catfish to be able to read the entire privatePlayer document? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They're only allowed to read other catfish private player docs in which only holds the their role(which we want them to know), stalker(wouldn't matter if they know because they're working together), and prompt in which they would have the same prompt. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm I guess so but it feels wrong There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you have a better idea lmk. We'd be only duplicating a little bit of data if you do have one |
||
} | ||
|
||
match /promptAnswers/{uid} { | ||
|
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.
shouldn't we call unsubscribeCatfishes here?
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 I understand but explain why?
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'm pretty sure the way the code is currently written we resubscribe every time the private player changes (each time there is a new prompt for example) but we never unsubscribe so that will put a lot of unwanted stress on the database.
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.
why is
unsubscribeCatfishes
not called if the state is ROLE?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.
So you saying subscribe for that moment then unsubscribe? And a side note I think the getDocs function might be better since we're not using the newly updated documents to display to the user or anything
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.
No I'm saying unsubscribe before you subscribe
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.
Yeah getDocs makes sense to me
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.