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

3013: Consolidate users #3050

Merged
merged 8 commits into from
Jul 30, 2022
Merged

3013: Consolidate users #3050

merged 8 commits into from
Jul 30, 2022

Conversation

dorner
Copy link
Collaborator

@dorner dorner commented Jul 24, 2022

Resolves #3013

Description

This change accomplishes the following:

  • Adds a partner_id to the User model.
  • Merge Partners::User and PartnerUser with the User model.
  • Adds new routes to allow users who have multiple "roles" (a partner_id and an organization_id) to switch between areas of the app.
  • Removes the consolidated login process with a single login page. Partner users can now log in via Google as well.

This change does not add the pundit and rolify gems, which would be an additional feature on top of this to enable things like partner admin roles.

One thing I'm unsure about is if partner users are given a separate URL somewhere to log in, and if we have to forward that URL to the new single login page.

Type of change

  • Internal change

How Has This Been Tested?

Local and unit tests.

Screenshots

image

image

TO DO: Run full test suite on CI and fix broken specs

if main_user.nil?
attrs = user.attributes.except('id').merge(discarded_at: nil)
# null constraint on name for User table doesn't exist for Partner User table
attrs['name'] ||= "CHANGEME"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
attrs['name'] ||= "CHANGEME"
attrs['name'] ||= "CHANGEME"

Could we guess the name using the email?

Copy link
Collaborator

@edwinthinks edwinthinks left a comment

Choose a reason for hiding this comment

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

@dorner nice work! I combed over the code and it looks pretty solid. I'am going to pull down the branch and test this on a copy of production as well.

@edwinthinks
Copy link
Collaborator

@dorner I noticed a few things when testing.

  • The logo for the invitation flow when inviting a partner is large and it is an outdated logo.

Captura de Pantalla 2022-07-29 a la(s) 7 31 41 a m

  • I think it can be a bit strange & cause confusion for our bank users to be directed to the partner page first. Could we update the logic so bank users are directed to their bank page first?

Otherwise, I didn't see any other issues with this release. I think it is important though we communicate with our banks and partners that the login page is going to be different in advance.

@dorner
Copy link
Collaborator Author

dorner commented Jul 29, 2022

@edwinthinks Updated! New page looks like this:
image

@dorner dorner force-pushed the 3013-consolidate-users-take2 branch from 8d2fbdb to fc99140 Compare July 29, 2022 19:58
Copy link
Collaborator

@edwinthinks edwinthinks left a comment

Choose a reason for hiding this comment

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

LGTM! This looks great! I'll probably want to set some messaging up before we make the release with this big change.

@edwinthinks edwinthinks merged commit 838d511 into main Jul 30, 2022
@seanmarcia seanmarcia deleted the 3013-consolidate-users-take2 branch September 27, 2022 01:26
danielabar added a commit to danielabar/human-essentials that referenced this pull request Jul 9, 2024
danielabar added a commit to danielabar/human-essentials that referenced this pull request Jul 19, 2024
danielabar added a commit to danielabar/human-essentials that referenced this pull request Jul 27, 2024
danielabar added a commit to danielabar/human-essentials that referenced this pull request Jul 27, 2024
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.

[Feat] Consolidate user accounts
2 participants