-
Notifications
You must be signed in to change notification settings - Fork 662
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
web-api(feat): add support for conversations.requestShared
approve
, deny
and list
APIs
#1843
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1843 +/- ##
==========================================
+ Coverage 91.61% 91.63% +0.01%
==========================================
Files 37 37
Lines 9924 9946 +22
Branches 633 633
==========================================
+ Hits 9092 9114 +22
Misses 820 820
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. |
02dfdaf
to
cd4a8f5
Compare
conversations.requestShared
approve
and deny
APIsconversations.requestShared
approve
, deny
and list
APIs
…` and `deny` APIs
…API request arguments.
cd4a8f5
to
22511ac
Compare
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.
Left one question about an optional property, but all looks in order to me.
/** @description Unique identifier of message. */ | ||
ts: string; | ||
} | ||
interface Message { | ||
/** @description A message to send to the user who requested the invite. */ | ||
message?: string; |
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 surprised this is optional, given it's the only property?
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 could easily fold this into the deny
method's arguments directly; the reason it is its own interface right now is that I factored it out when, earlier in these APIs' pilot, there were other methods (that the team decided not to ship) that used the same property, so instead of repeating it across different API argument interfaces, I factored it out and reused them. But, I just removed all those other APIs since they are not public yet.
I can fold that back into just the Deny arguments - let me know!
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 need: I trust your judgement! At first glance, it's just surprising that a Message
interface wouldn't feature the message
prop. 😂
Carry on!
This PR adds support for two new APIs: