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.

Sign up for GitHub

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

Jump to bottom

fix: clear anonymous user records after login #48144

Merged
merged 5 commits into from
Sep 5, 2024
Merged

fix: clear anonymous user records after login #48144

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d0e97b1
fix: clear anonymous user records after login
dominictb Aug 28, 2024
24f4854
fix: only clear the current anonymous account
dominictb Aug 30, 2024
47f6883
Merge remote-tracking branch 'origin/main' into fix/47806-anon
dominictb Aug 30, 2024
48a7997
fix: re-order exports in Session/index.ts
dominictb Aug 30, 2024
dcfb320
fix: remove redundant constant
dominictb Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion src/libs/actions/Session/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ import type {HybridAppRoute, Route} from '@src/ROUTES';
import ROUTES from '@src/ROUTES';
import SCREENS from '@src/SCREENS';
import type Credentials from '@src/types/onyx/Credentials';
import type {AutoAuthState} from '@src/types/onyx/Session';
import type Session from '@src/types/onyx/Session';
import type {AutoAuthState} from '@src/types/onyx/Session';
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB - was this just a prettier / style thing?

import clearCache from './clearCache';
import updateSessionAuthTokens from './updateSessionAuthTokens';

Expand Down Expand Up @@ -418,6 +418,21 @@ function beginSignIn(email: string) {
API.read(READ_COMMANDS.BEGIN_SIGNIN, params, {optimisticData, successData, failureData});
}

/**
* Create Onyx update to clean up anonymous user data
*/
function buildOnyxDataToCleanUpAnonymousUser() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this if we have the anonymous session (and therefore the accountID) just before logging in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before this fix, there's a potential case where user:

  • Access the public room -> create anonymous record 1
  • Login then logout
  • Access the public room again -> create anonymous record 2

-> It's better to clear both records, no?

Also, I think it's a more holistic approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see your point! Just thinking though, don't we clear all Onyx data on logging out, so all those hypothetical extra anonymous records would get cleared on next log-out anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jjcoffee hmm 🤔 , I'll give it a thought and let you know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

const data: Record<string, null> = {};
if (session.authTokenType === CONST.AUTH_TOKEN_TYPES.ANONYMOUS && session.accountID) {
data[session.accountID] = null;
}
return {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
value: data,
onyxMethod: Onyx.METHOD.MERGE,
};
}

/**
* Creates an account for the new user and signs them into the application with the newly created account.
*
Expand All @@ -434,6 +449,8 @@ function signUpUser() {
},
];

const onyxOperationToCleanUpAnonymousUser = buildOnyxDataToCleanUpAnonymousUser();

const successData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
Expand All @@ -442,6 +459,7 @@ function signUpUser() {
isLoading: false,
},
},
onyxOperationToCleanUpAnonymousUser,
];

const failureData: OnyxUpdate[] = [
Expand Down Expand Up @@ -517,6 +535,8 @@ function signIn(validateCode: string, twoFactorAuthCode?: string) {
},
];

const onyxOperationToCleanUpAnonymousUser = buildOnyxDataToCleanUpAnonymousUser();

const successData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
Expand All @@ -533,6 +553,7 @@ function signIn(validateCode: string, twoFactorAuthCode?: string) {
validateCode,
},
},
onyxOperationToCleanUpAnonymousUser,
];

const failureData: OnyxUpdate[] = [
Expand Down Expand Up @@ -567,6 +588,7 @@ function signInWithValidateCode(accountID: number, code: string, twoFactorAuthCo
// If this is called from the 2fa step, get the validateCode directly from onyx
// instead of the one passed from the component state because the state is changing when this method is called.
const validateCode = twoFactorAuthCode ? credentials.validateCode : code;
const onyxOperationToCleanUpAnonymousUser = buildOnyxDataToCleanUpAnonymousUser();

const optimisticData: OnyxUpdate[] = [
{
Expand Down Expand Up @@ -607,6 +629,7 @@ function signInWithValidateCode(accountID: number, code: string, twoFactorAuthCo
key: ONYXKEYS.SESSION,
value: {autoAuthState: CONST.AUTO_AUTH_STATE.JUST_SIGNED_IN},
},
onyxOperationToCleanUpAnonymousUser,
];

const failureData: OnyxUpdate[] = [
Expand Down
Loading