Skip to content

Make sure users are signed in when clicking on managed cards#4255

Merged
joelbettner merged 7 commits intomainfrom
ionatan_linkecom_signedin
Jul 30, 2021
Merged

Make sure users are signed in when clicking on managed cards#4255
joelbettner merged 7 commits intomainfrom
ionatan_linkecom_signedin

Conversation

@iwiznia
Copy link
Contributor

@iwiznia iwiznia commented Jul 27, 2021

Fixed Issues

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

Tests

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

@iwiznia iwiznia requested review from a team and cead22 July 27, 2021 15:50
@iwiznia iwiznia self-assigned this Jul 27, 2021
@MelvinBot MelvinBot requested review from joelbettner and removed request for a team July 27, 2021 15:51
src/CONST.js Outdated
ADD_SECONDARY_LOGIN_URL: 'settings?param={%22section%22:%22account%22}',
MANAGE_CARDS_URL: 'domain_companycards',
ADD_SECONDARY_LOGIN_URL: '/settings?param={%22section%22:%22account%22}',
VALIDATE_CODE_URL: (accountID, validateCode, exitTo = '') => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be better in ROUTES, there are no functions in this file, and tons in ROUTES

src/ROUTES.js Outdated
import {Linking} from 'react-native';
import Onyx from 'react-native-onyx';
import {addTrailingForwardSlash} from './libs/Url';
import {GetAccountValidateCode} from './libs/API';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nab but it's a bit weird for ROUTES to depend on API. Is there a better place for openSignedInLink?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know... tried putting it in URL but got the same shitty circular dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to suggestions...

Copy link
Contributor

@cead22 cead22 Jul 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/libs/actions/App.js

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok moved

src/ROUTES.js Outdated
* This links to a page in e.com ensuring the user is logged in.
* It does so by getting a validate code and redirecting to the validate URL with exitTo set to the URL
* we want to visit
* @param {string} url relative URL to open in expensify.com
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does "relative" mean "with a leading slash", if so, can you add that explicitly?

@iwiznia iwiznia requested a review from a team as a code owner July 27, 2021 16:39
@MelvinBot MelvinBot requested review from nkuoch and removed request for a team July 27, 2021 16:40
@iwiznia iwiznia removed the request for review from nkuoch July 27, 2021 16:40
cead22
cead22 previously approved these changes Jul 27, 2021
@iwiznia
Copy link
Contributor Author

iwiznia commented Jul 28, 2021

Updated and retested (had not done it before the last push 😱 and it was broken)

cead22
cead22 previously approved these changes Jul 28, 2021
@iwiznia
Copy link
Contributor Author

iwiznia commented Jul 29, 2021

Had conflicts, updated this. @cead22 please reapprove, @joelbettner please do your first review.

@joelbettner joelbettner merged commit f9531a0 into main Jul 30, 2021
@joelbettner joelbettner deleted the ionatan_linkecom_signedin branch July 30, 2021 20:13
@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.81-6🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Aug 6, 2021

🚀 Deployed to production in version: 1.0.82-7🚀

platform result
🤖 android 🤖 success ✅
🖥 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