room name and user name as GET parameters #95

leesalminen · 2020-03-18T17:13:14Z

This feature allows users to pass in the room name and user name as GET parameters to the application. This enables <iframe> embeds as well as easier link sharing to get multiple participants into the same room.

Example URL: https://video-app-XXXX-dev.twil.io/?passcode=NNNNNNNNN&room=my-room&user=admin

I know that the room name can be passed in via URL route, however this does not work when deployed to Twilio Functions API infrastructure.

Contributing to Twilio

All third party contributors acknowledge that any contributions they provide will be made under the same open source license that the open source project is provided under.

I acknowledge that all my contributions will be made under the project's license.

Burndown

Before review

Updated CHANGELOG.md if necessary
Added unit tests if necessary
Updated affected documentation
Verified locally with npm test
Manually sanity tested running locally
Included screenshot as PR comment (if needed)
Ready for review

Before merge

Got one or more +1s
Re-tested if necessary

…r to the passcode

charliesantos

Great addition @leesalminen ! Thank you for submitting. I left a few comments/questions.
Can you also please write tests for your changes? Thanks!

charliesantos · 2020-03-19T00:34:27Z

package.json

@@ -23,6 +23,7 @@
    "lint-staged": "^9.5.0",
    "lodash.throttle": "^4.1.1",
    "prettier": "^1.19.1",
+    "query-string": "^6.11.1",


This should've updated the package-lock.json. Can you please push up the changes?

charliesantos · 2020-03-19T00:35:15Z

src/state/usePasscodeAuth/usePasscodeAuth.ts

@@ -1,9 +1,22 @@
 import { useCallback, useEffect, useState } from 'react';
 import { useHistory } from 'react-router-dom';
+const queryString = require('query-string');


Can we use import here?

charliesantos · 2020-03-19T00:38:54Z

src/state/usePasscodeAuth/usePasscodeAuth.ts

+  const parsed = queryString.parse(window.location.search);
+  const user = parsed.user ? parsed.user : window.sessionStorage.getItem('user');
+  return user;
+}


If we are sharing this url, I would expect we want the user to enter their own username. Why are we setting it in the url? Can you please provide the use case for this?

@charliesantos there are a few use-cases where prefilling a user's name could be useful.

For example, I click a button to send an SMS link to the video room. The button trigger's a controller that queries by DB based on the phone entered and pulls the first name based on the phone number. Then, the controller adds that param to the link and sends an SMS -> user has a one-click join experience.

That's a good use case @cweems. In that case, I think we can keep this feature in this PR.

leesalminen · 2020-03-20T21:47:26Z

Our use case is similar. We wanted to add a button to our application that when clicked provided an instant join experience to a room. Our application already knows what room to join and the user’s name. I see your other notes on the code. I should have some time this weekend to review.

On Fri, Mar 20, 2020 at 3:45 PM Charlemagne Santos ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/state/usePasscodeAuth/usePasscodeAuth.ts <#95 (comment)> : > @@ -1,9 +1,22 @@ import { useCallback, useEffect, useState } from 'react'; import { useHistory } from 'react-router-dom'; +const queryString = require('query-string'); + +export function getRoomName() { + const parsed = queryString.parse(window.location.search); + const room = parsed.room ? parsed.room : window.sessionStorage.getItem('room'); + return room; +} + +export function getUserName() { + const parsed = queryString.parse(window.location.search); + const user = parsed.user ? parsed.user : window.sessionStorage.getItem('user'); + return user; +} That's a good use case @cweems <https://github.com/cweems>. In that case, I think we can keep this feature in this PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#95 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXA4572Z5LGIFVWFGWXA4DRIPPYDANCNFSM4LOWNIOQ> .

-- Lee Salminen “A ship is always safe on the shore, but that’s not what ships are for.” -Albert Einstein

saulm · 2020-03-28T23:12:18Z

This feature (passing user/room) as coded in this PR doesn't work if you don't use passcode auth correct?

saulm · 2020-03-30T12:43:04Z

Heads up, based on my testing locally of this feature, using auto connect in Safari will break the Audio (No audio track)

cweems · 2020-04-14T20:40:49Z

@leesalminen @saulm I made a PR on top of Lee's that adds the ability to accept URL params without passcode auth: leesalminen#1

I was also able to reproduce the Safari issue, but I don't understand what's causing it. Is there some Safari specific behavior that prevents audio and video access without a form submit?

timmydoza · 2020-04-15T21:00:23Z

Thanks for the PR @leesalminen

Some thoughts:

It looks like this feature only works when the app is set to use passcode authentication. I think it could be useful if this feature was always available, even when the app is set to use a different (or no) auth method.
This feature has the user join a room instantly when the room and user parameters are present. As some other users have pointed out, this can cause some problems with audio. Browsers have an autoplay policy which requires a user interaction (such as clicking on the page) before audio elements can start to play sound. When a users joins a room immediately, audio elements are added to the DOM before there has been any user interaction, and thus no sound can be heard.
An additional problem is that the user joins a room before they have granted permissions to the app to use their microphone and camera. This means that they join a room without publishing any audio or video to the room. Once they are in the room and grant access to their mic and camera, the app doesn't know to then publish those tracks to the room. This app assumes that it will have access to the user's mic and camera before the user clicks the 'Join Room' button, so we'd have to update some logic in the app to handle this auto-join scenario.

To summarize, I think it's fine to populate the user's name and room name via query parameters, but I don't think it's a good idea to add the auto-join feature. @leesalminen, would this PR still meet your use case if the auto-join feature is removed?

RusseII · 2020-04-17T18:43:24Z

To summarize, I think it's fine to populate the user's name and room name via query parameters, but I don't think it's a good idea to add the auto-join feature. @leesalminen, would this PR still meet your use case if the auto-join feature is removed?

I'm not sure about for @leesalminen but I can comment on my use case. We have a similar situation where we are creating a shareable link for job-seekers to join a two way interview. We already have the job seekers name, so it's nice being able to prepopulate that for them.

I actually prefer still having some sort of preview room so they can make sure everything looks nice and is working before joining the interview.

imjared · 2020-04-17T19:43:56Z

In our use case, we found it was sufficient enough to have users join the "waiting room" with standard GET parameters:

This saves us from the issues @timmydoza mentioned above, namely the browser's requirement for user interaction before <audio> is triggered

timmydoza · 2020-04-22T23:35:39Z

I think the uses cases that @RusseII and @imjared explain are good ones. I agree with having a 'waiting room' or a 'preview room'.

I can go ahead and wrap up this PR to implement the feature. One question that I have: If the user's identity and room name are supplied with query parameters, then should the 'Name' and 'Room' text fields still be editable by the user?

imjared · 2020-04-23T00:37:11Z

I can go ahead and wrap up this PR to implement the feature. One question that I have: If the user's identity and room name are supplied with query parameters, then should the 'Name' and 'Room' text fields still be editable by the user?

Good question. For our use case, I left them editable though I don't know why a user would want to edit them and I could certainly see a use case where the provider might want to obscure that functionality with something like simply having a "ready to join?" button.

RusseII · 2020-04-23T02:21:34Z

For our use case - We actually hide the room name. The user-name being editable doesn't really matter one way or the other to us. We will probably do what @imjared mentioned and obscure the name box with something like a "ready to join" button.

…

On Wed, Apr 22, 2020 at 6:35 PM timmydoza ***@***.***> wrote: I think the uses cases that @RusseII <https://github.com/RusseII> and @imjared <https://github.com/imjared> explain are good ones. I agree with having a 'waiting room' or a 'preview room'. I can go ahead and wrap up this PR to implement the feature. One question that I have: If the user's identity and room name are supplied with query parameters, then should the 'Name' and 'Room' text fields still be editable by the user? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#95 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADSXBSX4ODZEVN3DMYPA7K3RN55NRANCNFSM4LOWNIOQ> .

imjared · 2020-04-23T02:24:58Z

src/components/MenuBar/MenuBar.tsx

+    URLRoomName = window.sessionStorage.getItem('room') || '';
+  }
+
+  const URLUserName = window.sessionStorage.getItem('user') || '';


should this come from useParams() the same way that URLRoomName does?

timmydoza · 2020-04-29T05:23:04Z

Thanks for the discussion @RusseII, @imjared!

Given that each use case may vary a little bit, is seems that it may be tricky to implement a one-size-fits-all solution. It also sounds like a simple feature to implement, and that you both have already done so. Given all this, (in addition to the fact that we haven't heard from the original author @leesalminen in a while) I'm leaning towards closing this PR. Any objections?

imjared · 2020-04-29T17:16:12Z

@timmydoza honestly you all have made this thing so easy to customize that i think it's fine if it's left to the user to add the ?room_name= functionality.

took me all of like 10 mins after cloning to figure out how to run it and start making changes

timmydoza · 2020-05-01T19:11:51Z

Sounds good @imjared! I appreciate the feedback! I'll close this PR then.

RusseII · 2020-05-01T20:21:55Z

@timmydoza I'm a bit late to the party but I agree with everything @imjared said

ability to pass in room name and user name via GET parameters, simila…

0bc286a

…r to the passcode

aaalaniz requested review from timmydoza and charliesantos and removed request for timmydoza March 18, 2020 18:21

charliesantos requested changes Mar 19, 2020

View reviewed changes

imjared reviewed Apr 23, 2020

View reviewed changes

timmydoza closed this May 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

room name and user name as GET parameters #95

room name and user name as GET parameters #95

leesalminen commented Mar 18, 2020 •

edited

charliesantos left a comment

charliesantos Mar 19, 2020

charliesantos Mar 19, 2020

charliesantos Mar 19, 2020

cweems Mar 20, 2020

charliesantos Mar 20, 2020

leesalminen commented Mar 20, 2020 via email

saulm commented Mar 28, 2020

saulm commented Mar 30, 2020

cweems commented Apr 14, 2020

timmydoza commented Apr 15, 2020

RusseII commented Apr 17, 2020

imjared commented Apr 17, 2020

timmydoza commented Apr 22, 2020

imjared commented Apr 23, 2020

RusseII commented Apr 23, 2020 via email

imjared Apr 23, 2020

timmydoza commented Apr 29, 2020

imjared commented Apr 29, 2020

timmydoza commented May 1, 2020

RusseII commented May 1, 2020

room name and user name as GET parameters #95

room name and user name as GET parameters #95

Conversation

leesalminen commented Mar 18, 2020 • edited

Burndown

Before review

Before merge

charliesantos left a comment

Choose a reason for hiding this comment

charliesantos Mar 19, 2020

Choose a reason for hiding this comment

charliesantos Mar 19, 2020

Choose a reason for hiding this comment

charliesantos Mar 19, 2020

Choose a reason for hiding this comment

cweems Mar 20, 2020

Choose a reason for hiding this comment

charliesantos Mar 20, 2020

Choose a reason for hiding this comment

leesalminen commented Mar 20, 2020 via email

saulm commented Mar 28, 2020

saulm commented Mar 30, 2020

cweems commented Apr 14, 2020

timmydoza commented Apr 15, 2020

RusseII commented Apr 17, 2020

imjared commented Apr 17, 2020

timmydoza commented Apr 22, 2020

imjared commented Apr 23, 2020

RusseII commented Apr 23, 2020 via email

imjared Apr 23, 2020

Choose a reason for hiding this comment

timmydoza commented Apr 29, 2020

imjared commented Apr 29, 2020

timmydoza commented May 1, 2020

RusseII commented May 1, 2020

leesalminen commented Mar 18, 2020 •

edited