Issuer #1324: User should be global scope

[BurkeMtn]

[In Progress]

Step 1: Update User, Person, Organization models and associations

• Update models (see below) and validate backwards compatibility

Model Changes

• Users are global and belong to many Organizations and have many People.
  • The User model should contain attributes that are required for authentication and that are global in nature.
• People belong to an Organization.
  • The Person model will contain Organization specific attributes and represent the underlying User profile for a particular organization.
• Organizations have many People and Users.

Existing Model Associations

class Organization < ApplicationRecord
  has_many :people
  has_many :users
end

class Person < ApplicationRecord
  acts_as_tenant(:organization)
  has_one :user, dependent: :destroy
end

class User < ApplicationRecord
  acts_as_tenant(:organization)
  belongs_to :person
end

Proposed Model Associations

class Organization < ApplicationRecord
  has_many :people, dependent: :destroy
  has_many :users, through: :people
end

class Person < ApplicationRecord
  acts_as_tenant(:organization)
  belongs_to :user
end

class User < ApplicationRecord
  has_many :people, dependent: :destroy
  has_many :organizations, through: :people
end

Step 2: Migrate first_name and last_name from User to Person

Questions:

• Why do we want to move these fields to Person? E.g. Can a User have a different name for different organizations?
• Should roles be applied to Person instead of User?

April 2025 Update: we will remove first name and last name from User, and when a User accesses a new Org, and we ask them to join, we will look up the User's other person records and pull one and grab its first and last name to pre populate the Join this Org form. Update User sign up form to accept first and last name for Person.

Step 3: Move to context-aware Person creation

• Identify user flows that should create a Person
• Add join feature on sign_up and sign_in flows
• Do we need to delete the existing User callback or add context-aware hooks in addition to this?

April 2025 - double check this is all completed.

Step 4: Authorization

• Should roles be scoped to a Person given they are already scoped to an Organization? Alternatively, treat registered Users without a Person association as unregistered.

[kasugaijin]

@BurkeMtn

The proposed associations look good to me. We might want to make the belongs_to :user on Person optional. Just because we want to enable the future possibility of a Person existing without a User. Think of a case where staff manually enter details on someone who has not, and never will, log into the app, but they would like to have a record of this Person's details, and actions e.g. adopting a dog.

There has been a lot of historical discussion on the domains of User vs Person. We ultimately decided that User should focus solely on authentication and authorization for the application. This means roles would actually stay on User. For, Person we'd store all personal details, including first and last name. I could be convinced otherwise to keep first and last name on User instead, but there was debate on this as it's technically not info that should live on the User model if we stick to the described domains. If there's strong rationale for either approach, it can be discussed here.

Do we need to delete the existing User callback or add context-aware hooks in addition to this?

Given the optional association, I am of the opinon we de-couple User and Person in things like callbacks and get rid of this callback entirely. We can create a user's Person as needed depending on the flow (adopter sign up, invitation of staff, invitation of fosterers). This links back to the aforementioned possibility of creating a Person in the future without an associated User. Callbacks tying people to users could make this more complicated...and I think the two models don't need to co-exist necessarily.

Alternatively, treat registered Users without a Person association as unregistered.

Maybe I am not interpreting this properly, but it might be mixing authentication with authorization? I think we can still have an authenticated user who does not have a Person for a given tenant context, they are just missing a role that grants them authorizations. That's why they will need to Join a new org.

I think we can flesh out Step 3 above more once we finalize the preparation work for steps 1 and 2 here. I think step 3 is going to touch the most files to update what we have done in the first 2 steps. So, will be important to plan ahead and probably a good thing for us to work closer on. I am happy to help with that.

[BurkeMtn]

There has been a lot of historical discussion on the domains of User vs Person. We ultimately decided that User should focus solely on authentication and authorization for the application. This means roles would actually stay on User. For, Person we'd store all personal details, including first and last name.

Conceptually, is it accurate to think of a Person as being 1:1 with an organization (sort of like a profile) such that a user could have different personas depending on the organization? I was just thinking that if this is the case it might make sense to tie roles to people rather than users. That said, the existing model works fine and its probably not worth re-visiting this.

I could be convinced otherwise to keep first and last name on User instead, but there was debate on this as it's technically not info that should live on the User model if we stick to the described domains

I'm aligned on moving them to Person. Agree on the rationale.

Maybe I am not interpreting this properly, but it might be mixing authentication with authorization?

I think we are saying the same thing. Was just referring to the fact that permissions (authorization) for authenticated users without a person are the same as an unauthenticated users (hence should be easy to implement) -> "If I registered on Org A and visit Org B I should not be able to access any private routes (dashboard) until I join the org (have a Person)".

Given the optional association, I am of the opinon we de-couple User and Person in things like callbacks and get rid of this callback entirely.

Makes sense. If needed (depending on complexity) this can be deprecated incrementally with the callback running as fallback until it is no longer used.

[mononoken]

I am not sure we want Class User; has_many :people, dependent: :destroy; end since there is adoption information in these Person records that the organizations may require keeping. We will have to think separately about what data gets scrubbed when a User account gets deleted, especially in regards to user privacy and the organizations bookkeeping. It may be worthwhile to think about storing critical transaction info completely separate from the models as a "snapshot" somewhere to further prevent losing info that may be important. For example, if someone adopts a pet, deletes their account, and the shelter needs to contact them, they should be able to still find their contact info somewhere.

I think the rest of the proposed associations look good.

Thanks for thinking on this one!

[kasugaijin]

Very good point!

[BurkeMtn]

Do we provide the option for users to delete their accounts? If so, I would assume they would also like all of their other personal information removed from the website (most of which is stored in the Person object). We should understand what information (if any) the pet organizations are required to store permanently and work backwards from that. There are also edge cases related to handling in-flight adoptions etc. I can see this getting complicated.

[kasugaijin]

No we don’t have a delete account function yet. We figured we would keep most of the Person data, regardless, to maintain records for organizations. We can figure out how we want to handle this function later I think. We might just soft delete rather than wipe everything.

There are also edge cases related to handling in-flight adoptions etc. I can see this getting complicated.

Can you expand on this please?

[jmilljr24]

Looks like a solid plan!

Not a priority in any way but there was a discussion on adding a system admin role in the future. I'm assuming that would be easier with keeping roles on user. I'm not ever sure what this role's abilities would entail (maybe creating orgs on the frontend or debugging issues). Just thought I'd mention in case it would reduce any headache's in the future.

[kasugaijin]

Yeah that makes sense. Another option is to create a new devise resource entirely eg SysAdmin and that has its own set of routes to play around with….but we have options if we keep User for auth.

[kasugaijin]

@BurkeMtn how's this going?

[BurkeMtn]

Working on it.

[kasugaijin]

Deactivations
We have to refactor how we have been handling this to date. We currently deactivate on the User. This needs to change, because Org A deactivating User Z will prevent the user from being active in all Orgs. So, we need to move deactivations to the Org scope, and to the user type scope.

We are thinking that in order to deactivate a user, we should:

• remove their role e.g., staff
• update their associated Group (this is a new model) (this name sucks, but works for this explanation) record to have a deactivated_at (datetime) set.
• With the role removed, the policy should prevent actions under that role
• The Group record allows us to know what type of user this person has been to date e.g., adopter or staff

Model associations
A Person has many Groups through PersonGroups (through table)
A Group would belong_to Person through PersonGroups
Type would have attributes title, and enum for adopter, fosterer, staff; and a deactivated_at

We have to make sure we set the Group on a Person whenever we add a role to a User under an org (invitations for staff/fosterers, and upon new adopter sign ups)

There's probably better names to use than this. And alternative ideas to acheiving the same/similar also welcome.

[jmilljr24]

@kasugaijin I can take this on when it's ready.

I had a few thoughts after thinking about this a bit more.

The deactivated_at should probably go on PersonGroup. That keeps the Group table clean with just what groups each org has title: "adopter", organization_id: 1. Adding deactivated_at to group would defeat the purpose of the through table because we would need a unique group for each person. Many people should have the same group. Tracking a specific persons deactivation would be on the PersonGroup association.

I do think for simplicity sake we leave role on user for the time being. It will be a big change to move it to Person with little gain at this point. Conceptually I like the idea of roles being on the user for authorization purposes and being thought of separately from what "group" a person falls into. However as we discussed there are some downsides with our multi-tenant structure.

If/when we decide to remove the role from User, I don't think we should add it back to Person. The deactivated_at serves the purpose of tracking which "role" a person is authorized to do within their org. The Group association is still intact. This does remove the logical speration of "what a person is" (adopter), vs "what a person can do" (apply for adoptions...) but on the plus side is removes the somewhat duplication of adding a group and role. This also would remove the Rolify dependency. We are not using much of its features, and to some extent we are kind of trying to mold it to fit our use case vs. just using our own simple solution. Some previous discussion in #679

[kasugaijin]

@jmilljr24

The deactivated_at should probably go on PersonGroup. That keeps the Group table clean with just what groups each org has title: "adopter", organization_id: 1. Adding deactivated_at to group would defeat the purpose of the through table because we would need a unique group for each person. Many people should have the same group. Tracking a specific persons deactivation would be on the PersonGroup association.

Yes I agree...there should only be one record per group. This does make me wonder, the groups should be adopter, fosterer, and then staff or admin and superadmin? There might be reason to combine the admin/superadmin into a single group, unless we want to be able to differentiate them.

[jmilljr24]

Admin and Super_Admin have different permissions so we need a way to differentiate them if there is the potential to use this to determine authorization. I would keep them separate and use a scope if we need to query all staff.

[kasugaijin]

Yeah I agree. It feels more appropriate to map a group to a role than to start combining things.

[kasugaijin]

These have been moved to the Kanban board

- set up groups through table (comment above)
- migrate first and last name from User to Person - issue made
- Check invitations flow works as expected - to be manually tested
- manual testing of core flows before merging base branch into main branch
- Remove need for staff to input location details when inviting, and move this to adopter fosterer dash side link that takes user to a locations form to complete
- OmniAuth credentials (there is a set role method, we also need to be able to set the group)
- OrgCreateService also adds super admin role, we need to add super admin group here, and create the Person
- Sign up/in flow. Currently these are scoped to the org, and should be moved to global scope. We need to redirect the authenticated user back to the org they were on.

- Contacts flow - we use several layouts depending on user role, but now a user can have several roles, it doesnt't make sense - we need a single layout/or use a modal.
- Prevent adopter group only user from being able to visit the fostered pets page under adopter fosterer dashboard~~~

[jmilljr24]

@kasugaijin I think this is at the point where we can merge into main. The last couple of things required aren't major and we could open issues for contributions if you think that is appropriate.

I believe all that is left from this discussion and the initial issue are the following:

Verify Omniauth flow
remove first/last name from user
UI - handle authenticated user on org without person ( button to join)

I have a few other minor issues I can put up as well.

[kasugaijin]

Yes I agree it’s ready to go into main and we can once again open up issues to other contributors.