Details

Reviewers

atul
rohan
tomek
ashoat

Commits

rCOMM7a552df95c20: [keyserver] introduced reactionMessageCreationResponder function to message…

Summary

introduced reactionMessageCreationResponder function to message responders. reactionMessageCreationResponder validates the input from the client, does the permission checks, and if everything is copacetic creates the reaction message data object, and sends it to createMessages where it gets pushed into the db

Depends on D5747 and D5752

Linear Task: ENG-2250

Test Plan

This will be tested further in subsequent diffs. But ran this locally on web/mobile and nothing crashed. Also helped me get to the point where I could push a reaction message into the DB (see screenshot below)

Screenshot 2022-11-28 at 7.16.55 PM.png (2×3 px, 1 MB)

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Nov 28 2022, 4:31 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptNov 28 2022, 4:31 PM

ginsu edited the summary of this revision. (Show Details)Nov 28 2022, 4:38 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu added reviewers: atul, rohan, tomek.

ginsu added parent revisions: D5748: [keyserver] introduce sendReactionMessageRequestInputValidator to message responders, D5747: [keyserver] modified createMessages function to handle inserting targetMessageID into the DB.

ginsu attached a referenced file: F263988: Screenshot 2022-11-28 at 7.16.55 PM.png. (Show Details)

Harbormaster completed remote builds in B13812: Diff 18904.Nov 28 2022, 4:45 PM

ginsu requested review of this revision.Nov 28 2022, 4:45 PM

ashoat requested changes to this revision.Nov 28 2022, 7:02 PM

ashoat added inline comments.

keyserver/src/responders/message-responders.js
12 ↗	(On Diff #18904)	Can't find where this type is in `master` or any of the diffs in the stack
202 ↗	(On Diff #18904)	Great work checking this

This revision now requires changes to proceed.Nov 28 2022, 7:02 PM

ginsu requested review of this revision.Nov 29 2022, 7:01 AM

ginsu added inline comments.

keyserver/src/responders/message-responders.js
12 ↗	(On Diff #18904)	My bad thought I included the diff where I introduced `SendReactionMessageRequest` for review yesterday but here it is: D5752

added sendReactionMessageRequestInputValidator to this diff

ginsu added inline comments.Nov 29 2022, 7:53 AM

keyserver/src/responders/message-responders.js
12 ↗	(On Diff #18962)	D5752

ginsu edited the summary of this revision. (Show Details)Nov 29 2022, 7:53 AM

ginsu mentioned this in D5748: [keyserver] introduce sendReactionMessageRequestInputValidator to message responders.

Harbormaster completed remote builds in B13844: Diff 18962.Nov 29 2022, 8:06 AM

ginsu edited the summary of this revision. (Show Details)Nov 29 2022, 11:47 AM

ginsu added a parent revision: D5752: [lib] introduce SendReactionMessageRequest type to message types.

ashoat accepted this revision.Nov 30 2022, 7:00 AM

tomek accepted this revision.Nov 30 2022, 10:14 AM

tomek added inline comments.

keyserver/src/responders/message-responders.js
187 ↗	(On Diff #18962)	Should we check if this is actually the only supported reaction?
187–198 ↗	(On Diff #18962)	Can we make `reaction` mandatory in `sendReactionMessageRequestInputValidator` instead of checking if it's `undefined` later?
217 ↗	(On Diff #18962)	Are there other constraints on `reaction` that can be verified?

Looks good!

(make sure to respond to @tomek's questions before landing)

This revision is now accepted and ready to land.Nov 30 2022, 2:21 PM

ginsu added a child revision: D5750: [lib/keyserver] included reactionMessageCreationResponder to the JSON endpoints.Nov 30 2022, 3:50 PM

rebase with unsettingReaction field

updated parameter check

ginsu added inline comments.Dec 2 2022, 2:43 PM

keyserver/src/responders/message-responders.js
187 ↗	(On Diff #18962)	@atul and I talked about making a set of supported reactions and using that set as an extra check. I was thinking of doing that in a subsequent diff, but if you prefer me to do that in this diff let me know!
187–198 ↗	(On Diff #18962)	The new `unsettingReaction` field should address this, but please let me know if I missed something!
217 ↗	(On Diff #18962)	@atul and I talked about making a set of supported reactions and using that set as an extra check. I was thinking of doing that in a subsequent diff, but if you prefer me to do that in this diff let me know!

Harbormaster completed remote builds in B13951: Diff 19117.Dec 2 2022, 2:44 PM

Harbormaster completed remote builds in B13952: Diff 19118.Dec 2 2022, 2:50 PM

fix small bug

Harbormaster completed remote builds in B13957: Diff 19123.Dec 2 2022, 4:25 PM

rebase with action field

Harbormaster completed remote builds in B13971: Diff 19144.Dec 5 2022, 1:39 PM

fixed inconsitency in privacy/blocking logic

Rerequesting review because made some major changes with the creatorRelationshipHasBlock check in this responder

Harbormaster completed remote builds in B14493: Diff 19749.Dec 19 2022, 5:12 PM

ashoat accepted this revision.Dec 20 2022, 5:43 AM

ashoat added 1 blocking reviewer(s): atul.

ashoat changed 2 blocking reviewer(s), added 1: tomek; removed 1: atul.

Looks good! A couple of small issues inline - feel free to request a review if some of them require significant changes.

keyserver/src/responders/message-responders.js
190
202–204		Wouldn't the validator catch `!action`? We're setting `t.enums.of(['add_reaction', 'remove_reaction'])` so it shouldn't be empty here.
212–216		It is confusing to see a result of `fetchKnownUserInfos` being assigned to a singular variable and then accessed as an object.
222–226		It seems like this and `fetchKnownUserInfos` could be awaited at the same time.
187 ↗	(On Diff #18962)	I think at this point it is a matter of only a couple of chars changed, so it's worth doing it now.