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

[5.x] Improve authentication & account registering logic #318

Merged
merged 5 commits into from
Dec 1, 2023

Conversation

miguilimzero
Copy link
Contributor

@miguilimzero miguilimzero commented Nov 28, 2023

This pull request improves the new account registration handling. The current code has a duplicate code to handle that, and the logic gets slightly confusing.

Here is an example. Let's consider the flag hasCreateAccountOnFirstLoginFeatures is enabled:

Current version:

  • User tries auth from /login with social media, but the account already exists, and it's not connected.

The first code snippet will catch this because of the route match + flag.

  • User tries auth from /register with social media but the account already exists, and it's not connected.

The first code snippet will catch this because of the route match.

  • User tries auth from /potato with social media, but the account already exists, and it's not connected.

The second code snippet will catch this because there was no route match, and the second code snippet only needs the flag enabled.

With the code refactoring, the first code snippet will handle every route case if the "create account from login" flag is enabled (not only login) and the second code snipped is removed. We are preserving the exact same logic here.

Copy link
Owner

@joelbutcher joelbutcher left a comment

Choose a reason for hiding this comment

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

There are a few cases and features that need tests writing for them, which I'll look into once this PR is merged.

I know they don't currently exist, but please could you write tests for each stack in the stubs folder to check for the cases you've listed for this feature flag?

@miguilimzero
Copy link
Contributor Author

@joelbutcher Sorry, but I could not find a relation between this feature flag and the multiple stacks available. I made the following changes:

  • Split the specific feature tests into different files.
  • Added test with a random route for createAccountOnFirstLogin feature.
  • Added failure test for login and a random route for createAccountOnFirstLogin feature not enabled

@miguilimzero miguilimzero changed the title Improve authentication & account registering Improve authentication & account registering logic Nov 30, 2023
@joelbutcher joelbutcher changed the title Improve authentication & account registering logic [5.x] Improve authentication & account registering logic Dec 1, 2023
@joelbutcher joelbutcher merged commit ccfec50 into joelbutcher:5.x Dec 1, 2023
61 checks passed
@joelbutcher
Copy link
Owner

Released v5.3.2 with this fix included, sorry for the slowness here – thanks for your work!

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.

2 participants