-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Rename Unintuitive Function Names in Authentication Flow #9706
Rename Unintuitive Function Names in Authentication Flow #9706
Conversation
…and twenty-server
@FelixMalfait @AMoreaux Ready for review! |
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.
PR Summary
This PR renames authentication functions across the frontend and backend to improve clarity and maintainability, replacing generic terms like "challenge" and "verify" with more descriptive names that better reflect their purposes.
- Renamed backend mutations from
Challenge
toGetLoginTokenFromCredentials
andVerify
toGetAuthTokensFromLoginToken
- Updated frontend hooks and components to use new mutation names while maintaining existing functionality
- Modified test files and mocks to reflect renamed functions
- Updated excluded operations in GraphQL middleware to match new mutation names
- Added new feature flag
IsLocalizationEnabled
in generated metadata
16 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings | Greptile
variant: SnackBarVariant.Error, | ||
}); | ||
} | ||
|
||
if (isDefined(loginToken)) { | ||
setIsAppWaitingForFreshObjectMetadata(true); | ||
verify(loginToken); | ||
getAuthTokensFromLoginToken(loginToken); |
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.
logic: missing error handling for getAuthTokensFromLoginToken call
}, | ||
})), | ||
}, | ||
{ | ||
request: { | ||
query: queries.signup, | ||
variables: variables.challenge, | ||
variables: variables.getLoginTokenFromCredentials, |
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.
logic: variables.getLoginTokenFromCredentials is used here for signup, but signup likely needs its own variables object
variables: { loginToken }, | ||
}); | ||
|
||
if (isDefined(verifyResult.errors)) { | ||
throw verifyResult.errors; | ||
} | ||
|
||
if (!verifyResult.data?.verify) { | ||
if (!verifyResult.data?.getAuthTokensFromLoginToken) { |
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.
syntax: error message still references old terminology 'No verify result' instead of 'No auth tokens result'
async (email: string, password: string, captchaToken?: string) => { | ||
try { | ||
const challengeResult = await challenge({ | ||
const challengeResult = await getLoginTokenFromCredentials({ |
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.
style: variable name 'challengeResult' should be renamed to 'loginTokenResult' for consistency with new naming
async (loginToken: string) => { | ||
setIsVerifyPendingState(true); | ||
|
||
const verifyResult = await verify({ | ||
const verifyResult = await getAuthTokensFromLoginToken({ |
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.
style: variable name 'verifyResult' should be renamed to 'authTokensResult' for consistency with new naming
const user = await this.authService.challenge( | ||
getLoginTokenFromCredentialsInput, | ||
workspace, | ||
); |
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.
style: method name 'challenge' in authService should also be renamed to 'getLoginTokenFromCredentials' for consistency
async challenge( | ||
challengeInput: GetLoginTokenFromCredentialsInput, | ||
targetWorkspace: Workspace, | ||
) { |
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.
logic: function name 'challenge' should be renamed to 'getLoginTokenFromCredentials' to match PR's intent
Specifically, the 'IsLocalizationEnabled' and 'IsWorkflowEnabled' fields were removed from the 'FeatureFlagKey' enum in both 'graphql.ts' and 'graphql.tsx' files to streamline the generated code.
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.
Thanks for the refactor. It’s better now, but I noticed a few missed renames.
packages/twenty-server/src/engine/core-modules/auth/services/auth.service.ts
Outdated
Show resolved
Hide resolved
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.
Thank you @samyakpiya
@AMoreaux Took your comments
Log
|
Resolves twentyhq#9623 ## Description This PR renames the following functions to better reflect their purpose. - Backend: - Verify → GetAuthTokensFromLoginToken - Challenge → GetLoginTokenFromCredentials - Frontend: - challenge → getLoginTokenFromCredentials - verify → getAuthTokensFromLoginToken ## Testing _Sign in works as expected:_ https://github.com/user-attachments/assets/7e8f73c7-2c7d-4cd2-9965-5ad9f5334cd3 _Sign up works as expected:_ https://github.com/user-attachments/assets/d1794ee4-8b59-4934-84df-d819eabd5224 --------- Co-authored-by: Charles Bochet <[email protected]>
Resolves #9623
Description
This PR renames the following functions to better reflect their purpose.
Backend:
Frontend:
Testing
Sign in works as expected:
9706-Sign.In.mp4
Sign up works as expected:
9706-Sign.Up.mp4