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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(identity-provider): use service hooks and resolvers #9033

Merged
merged 14 commits into from Oct 22, 2023

Conversation

aditya-mitra
Copy link
Collaborator

@aditya-mitra aditya-mitra commented Oct 11, 2023

Summary

馃 Generated by Copilot at 0ff62e2

This pull request refactors and simplifies the identity-provider service and its related files, using hooks, KnexService, and removing unused or redundant code. It also implements the invite code feature for users, using the getFreeInviteCode function and modifying the user resolvers.

References

closes part of #8871

Explanation

馃 Generated by Copilot at 0ff62e2

  • Simplify and modularize the identity-provider service by using hooks instead of class methods (link, link, link, link, link, link, link, link, link)
  • Remove unused imports and variables from the identity-provider files (link, link, link, link, link)
  • Remove the userId resolver from the identity-provider resolvers, as it is added by the hooks (link)
  • Update the identity-provider tests to match the new functionality and query parameters (link, link, link, link, link, link, link, link, link)
  • Modify the checkScope function to return false instead of throwing an error when no scopes are available (link)
  • Implement the functionality of generating a unique invite code for each user using the getFreeInviteCode function (link, link, link)

馃 Generated by Copilot at 0ff62e2

We're heaving on the ropes, me hearties, on the count of three
We're simplifying code, me hearties, for the IdentityProviderService
We're adding invite codes, me hearties, for each user to join the app
We're improving type safety, me hearties, and cleaning up the scrap

QA Steps

List any additional steps required to QA the changes of this PR, as well as any supplemental images or videos.

Checklist

  • If this PR is still a WIP, convert to a draft
  • When this PR is ready, mark it as "Ready for review"
  • ensure all checks pass
  • Changes have been manually QA'd
  • Changes reviewed by at least 2 approved reviewers

Changed order of identity-provider create hooks. validateAuthParams needs to occur
after createNewUser, or else it won't have a userId to act on.
Copy link
Member

@barankyle barankyle left a comment

Choose a reason for hiding this comment

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

There's some bugs surrounding validateAuthParams and createNewUser. As it currently stands, creating a guest user is failing because validateAuthParams never resolves to a userId; there's no authentication present when creating a guest user, and there's no userId on the request data because requests to create a new guest user by definition don't have a userId already.

I tried moving validateAuthParams to the end of the array of create hooks, but then requests to create new identity-providers with a userId on the request data are causing createNewUser to trigger, as there's no existingUser object on the context when it gets to that check. This makes new users when it shouldn't, and the identity-providers get associated with those new guest users by mistake.

I changed a few of the test checks to not just ensure that a userId is present but that it matches the userId passed as part of the request. If the hook logic is altered so that all tests pass, I think that will resolve the current issues.

@aditya-mitra
Copy link
Collaborator Author

There's some bugs surrounding validateAuthParams and createNewUser. As it currently stands, creating a guest user is failing because validateAuthParams never resolves to a userId; there's no authentication present when creating a guest user, and there's no userId on the request data because requests to create a new guest user by definition don't have a userId already.

I tried moving validateAuthParams to the end of the array of create hooks, but then requests to create new identity-providers with a userId on the request data are causing createNewUser to trigger, as there's no existingUser object on the context when it gets to that check. This makes new users when it shouldn't, and the identity-providers get associated with those new guest users by mistake.

I changed a few of the test checks to not just ensure that a userId is present but that it matches the userId passed as part of the request. If the hook logic is altered so that all tests pass, I think that will resolve the current issues.

You are right. I totally missed this scenario due to an incorrect arrangement of hooks.

I have changed the validateAuthParams to do an early return if the identity-provider type is guest

The tests are now passing!

image

@hanzlamateen hanzlamateen merged commit 4fa898e into dev Oct 22, 2023
11 of 13 checks passed
@hanzlamateen hanzlamateen deleted the refactor/identity-provider-service-hooks branch October 22, 2023 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants