-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Update signInWithShortLivedToken to use API.Write instead of DeprecateAPI commands #13456
Changes from 15 commits
2aa3759
c47a091
f49f859
84d1ed7
e564601
47b67b3
f0c7dfe
957c07a
f023893
3b8c4ba
ae16c2d
77e74de
799ad8d
cb42542
9708d54
92c0649
83f661d
eec41c4
45b35b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,8 @@ | ||
import Onyx from 'react-native-onyx'; | ||
import Str from 'expensify-common/lib/str'; | ||
import _ from 'underscore'; | ||
import lodashGet from 'lodash/get'; | ||
import ONYXKEYS from '../../../ONYXKEYS'; | ||
import redirectToSignIn from '../SignInRedirect'; | ||
import * as DeprecatedAPI from '../../deprecatedAPI'; | ||
import CONFIG from '../../../CONFIG'; | ||
import Log from '../../Log'; | ||
import PushNotification from '../../Notification/PushNotification'; | ||
|
@@ -14,7 +12,6 @@ import * as Localize from '../../Localize'; | |
import UnreadIndicatorUpdater from '../../UnreadIndicatorUpdater'; | ||
import Timers from '../../Timers'; | ||
import * as Pusher from '../../Pusher/pusher'; | ||
import * as User from '../User'; | ||
import * as Authentication from '../../Authentication'; | ||
import * as Welcome from '../Welcome'; | ||
import * as API from '../../API'; | ||
|
@@ -27,23 +24,6 @@ Onyx.connect({ | |
callback: val => credentials = val, | ||
}); | ||
|
||
/** | ||
* Sets API data in the store when we make a successful "Authenticate"/"CreateLogin" request | ||
* | ||
* @param {Object} data | ||
* @param {String} data.accountID | ||
* @param {String} data.authToken | ||
* @param {String} data.email | ||
*/ | ||
function setSuccessfulSignInData(data) { | ||
PushNotification.register(data.accountID); | ||
Onyx.merge(ONYXKEYS.SESSION, { | ||
errors: null, | ||
..._.pick(data, 'authToken', 'accountID', 'email', 'encryptedAuthToken'), | ||
}); | ||
Onyx.set(ONYXKEYS.SHOULD_SHOW_COMPOSE_INPUT, true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this safe to remove? Need to confirm it doesn't cause any regression regarding compose input display There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, good thought. We should be fine though because the internal PR created before this one sets the value for that onyx key. |
||
} | ||
|
||
/** | ||
* Clears the Onyx store and redirects user to the sign in page | ||
*/ | ||
|
@@ -59,7 +39,7 @@ function signOut() { | |
{ | ||
onyxMethod: CONST.ONYX.METHOD.SET, | ||
key: ONYXKEYS.CREDENTIALS, | ||
value: null, | ||
value: {}, | ||
}, | ||
]; | ||
API.write('LogOut', { | ||
|
@@ -159,64 +139,43 @@ function beginSignIn(login) { | |
} | ||
|
||
/** | ||
* | ||
* Will create a temporary login for the user in the passed authenticate response which is used when | ||
* re-authenticating after an authToken expires. | ||
NikkiWines marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* | ||
* @param {String} authToken | ||
* @param {String} email | ||
* @return {Promise} | ||
*/ | ||
function createTemporaryLogin(authToken, email) { | ||
const autoGeneratedLogin = Str.guid('expensify.cash-'); | ||
const autoGeneratedPassword = Str.guid(); | ||
function signInWithShortLivedAuthToken(authToken) { | ||
const optimisticData = [ | ||
{ | ||
onyxMethod: CONST.ONYX.METHOD.MERGE, | ||
key: ONYXKEYS.ACCOUNT, | ||
value: { | ||
...CONST.DEFAULT_ACCOUNT_DATA, | ||
isLoading: true, | ||
}, | ||
}, | ||
]; | ||
|
||
return DeprecatedAPI.CreateLogin({ | ||
authToken, | ||
partnerName: CONFIG.EXPENSIFY.PARTNER_NAME, | ||
partnerPassword: CONFIG.EXPENSIFY.PARTNER_PASSWORD, | ||
partnerUserID: autoGeneratedLogin, | ||
partnerUserSecret: autoGeneratedPassword, | ||
shouldRetry: false, | ||
forceNetworkRequest: true, | ||
email, | ||
includeEncryptedAuthToken: true, | ||
}) | ||
.then((createLoginResponse) => { | ||
if (createLoginResponse.jsonCode !== 200) { | ||
Onyx.merge(ONYXKEYS.ACCOUNT, {errors: {[DateUtils.getMicroseconds()]: Localize.translate('createLoginResponse.message')}}); | ||
return createLoginResponse; | ||
} | ||
|
||
setSuccessfulSignInData(createLoginResponse); | ||
|
||
// If we have an old generated login for some reason | ||
// we should delete it before storing the new details | ||
if (credentials && credentials.autoGeneratedLogin) { | ||
DeprecatedAPI.DeleteLogin({ | ||
partnerUserID: credentials.autoGeneratedLogin, | ||
partnerName: CONFIG.EXPENSIFY.PARTNER_NAME, | ||
partnerPassword: CONFIG.EXPENSIFY.PARTNER_PASSWORD, | ||
shouldRetry: false, | ||
}) | ||
.then((response) => { | ||
if (response.jsonCode === CONST.JSON_CODE.SUCCESS) { | ||
return; | ||
} | ||
|
||
Log.hmmm('[Session] Unable to delete login', false, {message: response.message, jsonCode: response.jsonCode}); | ||
}); | ||
} | ||
|
||
Onyx.merge(ONYXKEYS.CREDENTIALS, { | ||
autoGeneratedLogin, | ||
autoGeneratedPassword, | ||
}); | ||
return createLoginResponse; | ||
}) | ||
.finally(() => { | ||
Onyx.merge(ONYXKEYS.ACCOUNT, {isLoading: false}); | ||
}); | ||
const successData = [ | ||
{ | ||
onyxMethod: CONST.ONYX.METHOD.MERGE, | ||
key: ONYXKEYS.ACCOUNT, | ||
value: { | ||
isLoading: false, | ||
}, | ||
}, | ||
]; | ||
|
||
const failureData = [ | ||
{ | ||
onyxMethod: CONST.ONYX.METHOD.MERGE, | ||
key: ONYXKEYS.ACCOUNT, | ||
value: { | ||
isLoading: false, | ||
}, | ||
}, | ||
]; | ||
API.write('SignInWithShortLivedAuthToken', {authToken, oldPartnerUserID: credentials.autoGeneratedLogin || ''}, {optimisticData, successData, failureData}); | ||
} | ||
|
||
/** | ||
|
@@ -262,29 +221,6 @@ function signIn(password, twoFactorAuthCode) { | |
API.write('SigninUser', {email: credentials.login, password, twoFactorAuthCode}, {optimisticData, successData, failureData}); | ||
} | ||
|
||
/** | ||
* Uses a short lived authToken to continue a user's session from OldDot | ||
* | ||
* @param {String} email | ||
* @param {String} shortLivedAuthToken | ||
* @param {String} exitTo | ||
*/ | ||
function signInWithShortLivedAuthToken(email, shortLivedAuthToken) { | ||
Onyx.merge(ONYXKEYS.ACCOUNT, {...CONST.DEFAULT_ACCOUNT_DATA, isLoading: true}); | ||
|
||
createTemporaryLogin(shortLivedAuthToken, email) | ||
.then((response) => { | ||
if (response.jsonCode !== CONST.JSON_CODE.SUCCESS) { | ||
return; | ||
} | ||
|
||
User.getUserDetails(); | ||
Onyx.merge(ONYXKEYS.ACCOUNT, {success: true}); | ||
}).finally(() => { | ||
Onyx.merge(ONYXKEYS.ACCOUNT, {isLoading: false}); | ||
}); | ||
} | ||
|
||
/** | ||
* User forgot the password so let's send them the link to reset their password | ||
*/ | ||
|
@@ -361,7 +297,7 @@ function invalidateAuthToken() { | |
function clearSignInData() { | ||
Onyx.multiSet({ | ||
[ONYXKEYS.ACCOUNT]: null, | ||
[ONYXKEYS.CREDENTIALS]: null, | ||
[ONYXKEYS.CREDENTIALS]: {}, | ||
}); | ||
} | ||
|
||
|
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.
Wait, this can't be removed, it needs to exist somewhere or push notifications don't work
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.
Need to check if this code is enough
App/src/Expensify.js
Lines 122 to 124 in e1c6a2e
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.
Should be fine, added a console log in to check
Screen.Recording.2022-12-21.at.15.13.55.mov
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.
So odd, so that call was useless? You sure it is not used in some other flow? Like signing in normally?
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.
Seems like it, runs on normal sign in as well
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.
Oh wow.... just to be sure, tagging @Julesssss, @arosiclair and @cristipaval that are probably familiar with this since they are working on the notifications doc.
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.
Yeah as long as the new accountID gets set on the session, the code @aimane-chnaif linked should register notifications.