Explicit video url #128
Explicit video url #128
Conversation
|
|
||
| if (!videoToken) { | ||
| throw new MissingParamsError(["videoToken"]); | ||
| if (!videoReference) { |
There was a problem hiding this comment.
think it might be worth having some validation of videoReferences.
|
|
||
| return publicVideoRef; | ||
| } | ||
| // async function movePrivateVideoToPublicVideo( |
There was a problem hiding this comment.
also we'll need this stuff until all clients have updated o.o + create videoReferences server side for older clients :/
…to allow for backwards compatibility
packages/client/src/me/sendInvite.ts
Outdated
| videoToken: string, | ||
| isJointVideo: boolean | ||
| ) { | ||
| export function sendInvite(args: { |
There was a problem hiding this comment.
What's the args: { } change for? Don't see anything different in the call-sites
There was a problem hiding this comment.
Just would rather be using named arguments than positional ones, less likely to mess them up, less brittle to stuff like mistaken orderings, better automatic code suggestions
There was a problem hiding this comment.
but i can save that for a separate pr
osdiab
force-pushed
the
osdiab/feature/explicit-video-url
branch
from
74a4dc8 to
d670ec7
Compare
osdiab
force-pushed
the
osdiab/feature/explicit-video-url
branch
from
d670ec7 to
49e17e7
Compare
|
|
||
| import { callApi } from "../callApi"; | ||
|
|
||
| /** | ||
| * API call for a non-member to join Raha and create a new member. | ||
| * @param fullName Full name of new member | ||
| * @param username New username for the new member. Must be unique | ||
| * @param videoToken Video token of the invite, of the new member either identifying themselves or | ||
| * being invited by the existing member. | ||
| * @param videoReference Video of the new member either identifying themselves. |
There was a problem hiding this comment.
"either identifying themselves or being invited by the existing member""
| @@ -21,7 +21,7 @@ export const listMembers = (membersCollection: CollectionReference) => | |||
| id: member.id, | |||
| created_at: member.get("created_at"), | |||
| full_name: member.get("full_name"), | |||
| identity_video_url: member.get("identity_video_url"), | |||
| identityVideoReference: member.get("identityVideoReference"), | |||
There was a problem hiding this comment.
why switch to camel-case? should we be consistent?
There was a problem hiding this comment.
this PR is closed since it's not backwards compatible, but i think it's lame we even have any inconsistency about using snake_case here at all so I wanted to incrementally move new stuff to camel case. But given that we'll have to support old versions indefinitely, i'll just make it be snake_case and embrace the pointless inconsistency
There was a problem hiding this comment.
I thought in general we did underlines for field names
There was a problem hiding this comment.
the only place we ever do it is operation field names, probably bc @rahulgi defined the schema when he was more in python mode than javascript, so every time we go from operations to code we have to keep renaming things from snake case to camel case; it's pointless but we've committed
There was a problem hiding this comment.
can we just migrate things? i'm ok with doing camelcase moving forward
| @@ -46,15 +46,15 @@ export const sendInvite = ( | |||
| loggedInMember | |||
| ); | |||
|
|
|||
| const { inviteEmail, videoToken, isJointVideo } = call.body; | |||
| const { inviteEmail, videoReference, isJointVideo } = call.body; | |||
|
|
|||
| if (!loggedInMember.exists) { | |||
| throw new InviterMustBeInvitedError(); | |||
| } | |||
|
|
|||
| const requiredParams = { | |||
There was a problem hiding this comment.
Would this break if an older client tried to invite someone?
There was a problem hiding this comment.
this PR doesn't support backwards compatibility and is now closed
| errors.push({ | ||
| ...errorDetailBase, | ||
| message: `Field 'kind' expected to be '${ | ||
| MediaReferenceKind.VIDEO |
|
There's no need to PR this anymore because we can't rely on codepush to invalidate old clients, so closing. |
pass video urls explicitly to the api, rather than inferring them.
Cannot be deployed until associated migration is written/ran, and mobile app is using new client.