Skip to content
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

Open modal to join screen share requests from GuidesPlus agents #6654

Merged
merged 8 commits into from
Dec 14, 2021

Conversation

francoisl
Copy link
Contributor

@francoisl francoisl commented Dec 8, 2021

cc @tjferriss

Details

  • Listen to screen share invites sent by GuidesPlus agents. When joined, open OldDot in a new tab, where all the code to join screen shares already exists.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/184039

Tests

Needs the Web-E PR https://github.com/Expensify/Web-Expensify/pull/32573. Tests steps

QA Steps

Regular staging QA: make sure there are no regressions in NewDot potentially introduced by this PR.

Internal QA (since it requires EW access - I can do this part myself): follow QA steps from the Web-E PR.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

See the recording in https://github.com/Expensify/Web-Expensify/pull/32573 for the full flow

@francoisl francoisl self-assigned this Dec 8, 2021
@francoisl francoisl requested a review from a team as a code owner December 8, 2021 23:14
@MelvinBot MelvinBot requested review from marcochavezf and removed request for a team December 8, 2021 23:15
src/Expensify.js Outdated Show resolved Hide resolved
src/languages/es.js Outdated Show resolved Hide resolved
src/languages/es.js Outdated Show resolved Hide resolved
@francoisl
Copy link
Contributor Author

Updated, thanks.

I'm seeing a warning for the propTypes in the console that makes 0 sense when I receive the push notification:

Failed prop type: Invalid prop `screenShareRequest.accessToken` of type `string` supplied to `Expensify`, expected `object`.

image

screenShareRequest.accessToken is expected to be a string according to the code, not an object. Also, a string is supplied. Do you happen to have any idea? Kinda stumped on this one.

@marcochavezf
Copy link
Contributor

Updated, thanks.

I'm seeing a warning for the propTypes in the console that makes 0 sense when I receive the push notification:

Failed prop type: Invalid prop `screenShareRequest.accessToken` of type `string` supplied to `Expensify`, expected `object`.

image

screenShareRequest.accessToken is expected to be a string according to the code, not an object. Also, a string is supplied. Do you happen to have any idea? Kinda stumped on this one.

Ah we'll need to use only PropTypes.shape, something like:

    screenShareRequest: PropTypes.shape({

        /** Access token required to join a screen share room, generated by the backend */
        accessToken: PropTypes.string,

        /** Name of the screen share room to join */
        roomName: PropTypes.string,
    }),

because PropTypes.objectOf is used when we don't know the name of the properties ahead of time, but in this case we already know screenShareRequest has only the properties accessToken and roomName. https://stackoverflow.com/questions/45764746/react-proptypes-objectof-vs-shape

@francoisl
Copy link
Contributor Author

Oooh got it, thanks, that makes sense. The PR is updated, but still on hold.

@francoisl francoisl changed the title [HOLD Web-E #32573] Open modal to join screen share requests from GuidesPlus agents Open modal to join screen share requests from GuidesPlus agents Dec 14, 2021
@francoisl
Copy link
Contributor Author

Ok this is ready to go, the PR it depended on is on production on Web-E now. I also fixed the conflict in this PR.

@marcochavezf marcochavezf merged commit 7b0752d into main Dec 14, 2021
@marcochavezf marcochavezf deleted the francois-guidesPlusScreenshareRequest branch December 14, 2021 20:54
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @marcochavezf in version: 1.1.20-3 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@mvtglobally
Copy link

@francoisl, @marcochavezf Can you please validate this internally as well and check off

@francoisl
Copy link
Contributor Author

Tested just now with @leminhphong, worked like a charm!

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Julesssss in version: 1.1.21-1 🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants