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

refactor(UserService): simplify login by using findOrCreate() #1281

Merged
merged 5 commits into from
Apr 11, 2024

Conversation

timotheeg
Copy link
Contributor

@timotheeg timotheeg commented Apr 9, 2024

Problem

Incorrect comments on not being able to use findOrCreate() with a transaction.

(Also I think the old code for create might have been incorrect, because the first argument to create() must be an object, not a string (?) 🤔

While .findOrCreate() does in fact accept a transaction (see API documentation), I think the update to lastLoggedIn doesn't need to be in the transaction. Even if there is a conflict with multiple writes overwriting each other, the updates would be so close by that's the inaccuracy is not business critical.

Because of that, we can simplify the code by using the default transaction behaviour of .findOrCreate(), instead of providing our own transaction.

Solution

  1. Use .findOrCreate() in both login and loginWithEmail
  2. Move the update to lastLoggedIn out of transaction
  3. Update the tests

.findOrCreate() does provide its own transaction, documentation states

If no transaction is passed in the options object, a new transaction will be created internally, to prevent the race condition where a matching row is created by another connection after the find but before the insert call.

Verified on staging (sample trace here):
image

Tests

  • Login successfully via email
  • Login successfully via github

@timotheeg timotheeg requested a review from a team April 9, 2024 03:13
@timotheeg timotheeg force-pushed the refactor_findorcreate branch from 4439454 to addbe74 Compare April 9, 2024 04:37
@timotheeg timotheeg merged commit d750362 into develop Apr 11, 2024
21 checks passed
@timotheeg timotheeg deleted the refactor_findorcreate branch April 11, 2024 01:08
@harishv7 harishv7 mentioned this pull request Apr 11, 2024
6 tasks
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