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

API: Fix the usage of userType #1711

Closed
palisadoes opened this issue Jan 17, 2024 · 44 comments
Closed

API: Fix the usage of userType #1711

palisadoes opened this issue Jan 17, 2024 · 44 comments
Assignees
Labels
bug Something isn't working Ready 4 PR Review

Comments

@palisadoes
Copy link
Contributor

palisadoes commented Jan 17, 2024

Describe the bug

  1. The schema doesn't apply a person's userType by organization.
  2. The userType is currently a part of the User definition not part of a userOrganization definition or something similar.
    1. This means that an Admin is an Admin for all organizations which grants the same rights as a SuperAdmin. This is not the intention.
    2. This has created historical confusion in the Admin and SuperAdmin roles.

This needs to be resolved as it is a showstopper bug.

Expected behavior

These conditions must apply:

  1. The userType must be applied per person per organization.
  2. Each SuperAdmin must be a SuperAdmin for all organizations
  3. All other userTypes would be on a per organization basis

This means that all code logic referencing the userType will need to be updated accordingly.

TALAWA API

Additional considerations include:

  1. New user registrations will need to default to the userType USER if not done so already
  2. The Schema will need to be updated to reflect this requirement
  3. Mutations and queries will need to be updated and in some cases new ones added.

TALAWA ADMIN

Additional considerations include:

  1. The current screen that edits a person's userType will need to be updated. A design is being created for this and will be reflected in the Figma file on our Design Guide page:
    1. https://docs.talawa.io/docs/design/ux/ux-design-system

TALAWA MOBILE

No expected changes as userType isn't referenced in the code base

Actual behavior

  • The schema doesn't apply a person's userType by organization.

Screenshots

The current schema:

image

Additional details

Related issues:

  1. API: Fix the usage of userType #1711
  2. Admin: Fix the usage of userType talawa-admin#1445

Potential internship candidates

Please read this if you are planning to apply for a Palisadoes Foundation internship

@palisadoes palisadoes added the bug Something isn't working label Jan 17, 2024
@github-actions github-actions bot added the unapproved Unapproved for Pull Request label Jan 17, 2024
@Saturn225
Copy link

Would like to work on this issue.

1 similar comment
@NayOoLwin5
Copy link

Would like to work on this issue.

@AVtheking
Copy link
Contributor

I would like to work on this issue

@Pranavx1
Copy link

Can i work on this?

@Cioppolo14 Cioppolo14 removed the unapproved Unapproved for Pull Request label Jan 17, 2024
@Cioppolo14
Copy link
Contributor

@KoraboinaSankeerth You requested this in both the api & admin repos. Since you had no issues, I did give you both. These are both noted as urgent issues, so if having both will slow down the resolution, please unassign yourself from one, so we can make sure this gets addressed as fast as possible. Thank you!

@AVtheking
Copy link
Contributor

Hey @palisadoes if the admin is going to per organisation then I think there is no purpose of user type field as we already have adminfor field in the user schema which contain the list of organisation in which user is admin ,we can directly check from it . Instead of user type field we can have Is_Super_Admin field

@palisadoes
Copy link
Contributor Author

  1. The super admin flag would solve part of the workflow
  2. We need the user type of USER and NON_USER because the app will need to distinguish between people in the database who do, and do not, use the app.
    1. This will be required for the check-in process to record attendance of Members who cannot, do not want or have yet to use the app.
    2. The current registration process assumes email address and Internet access, but older or incapacitated people may not have access.

Other challenges to consider:

  1. Ways to uniquely identify people. (We have an issue to create an identifier auto incrementing field which could help)
  2. How to assign Admin rights only to USERs in a clean way as suggested by @xoldyckk
    1. feat: Created an User Type Flag Field - AppUserType #1633 (comment)

How would can we best achieve all these goals?

@AVtheking
Copy link
Contributor

How will the user would get userType =Non_user as the user registering through mobile app and web portal will get userType=user by default?

@palisadoes
Copy link
Contributor Author

In at least these circumstances:

  1. An administrator decides to add them manually. This will allow the person to be checked-in to future events even though they don't use the app.
  2. During the event check-in process where anyone who attends an event may need to be added to the system.

Both these features are planned to be added soon.

@xoldd
Copy link
Contributor

xoldd commented Jan 18, 2024

  1. The super admin flag would solve part of the workflow

  2. We need the user type of USER and NON_USER because the app will need to distinguish between people in the database who do, and do not, use the app.

    1. This will be required for the check-in process to record attendance of Members who cannot, do not want or have yet to use the app.
    2. The current registration process assumes email address and Internet access, but older or incapacitated people may not have access.

Other challenges to consider:

  1. Ways to uniquely identify people. (We have an issue to create an identifier auto incrementing field which could help)

  2. How to assign Admin rights only to USERs in a clean way as suggested by @xoldyckk

    1. feat: Created an User Type Flag Field - AppUserType #1633 (comment)

How would can we best achieve all these goals?

I should remind that this suggestion was made considering the approach used in that PR(adding NON_USER variant to userType), it's not a bulletproof suggestion.

@palisadoes
Copy link
Contributor Author

@xoldyckk Even so, the points need to be considered in the PR for this issue for their applicability.
@AVtheking Let us know your updated final proposed solution would be.

@AVtheking
Copy link
Contributor

AVtheking commented Jan 18, 2024

ok then we can go with the userType field but I think we can have separate model for the non-users as temp user with basic fields of the user and as we already have the field adminFor which contain the list of organization for which the user is admin so I think there is no need to specify user as admin in a different field .This could solve the problem of confusion between admin and superadmin

@AVtheking
Copy link
Contributor

AVtheking commented Jan 18, 2024

when the non user would become user then we can migrate data to User model

@AVtheking
Copy link
Contributor

with this we can easily provide admin right only to user as we would querying on User model only and Non User would be stored in different model

@palisadoes
Copy link
Contributor Author

palisadoes commented Jan 19, 2024

  1. It makes more sense to make User be the default with the minimum required fields with additional data added for app users.
    1. For example, an AppUser object / class would inherit User object / class characteristics to have expanded access to fields. This is what @xoldyckk was suggesting.
    2. It's going to require more coordination with the other repos and the earlier we start the better before we have too much legacy code.
    3. adminFor would be under AppUser
    4. A superAdmin flag would be a part of AppUser, and when set, there would be no need for adminFor. This would make the logic easier because when a new organization is added, you wouldn't have to add the superAdmins to it because the flag was already set.
  2. Having temporary users and migrating data back and forth is going to be very cumbersome. We are trying to move towards greater simplicity.

@AVtheking
Copy link
Contributor

Yes this approach of having an AppUser object and generic User object is good we can implement it

@palisadoes
Copy link
Contributor Author

Unassigning due to no PR activity or assignee comments in the past 10 days

@AVtheking
Copy link
Contributor

Can I work on this issue ,although I am already assigned with two issues but the one in mobile repo is ready to be merged

@manishjha-04
Copy link

Hey can I work on this ?

@NayOoLwin5
Copy link

I would like to work on this

@git-init-priyanshu
Copy link
Member

I would love to work on this issue. Thanks.

@Cioppolo14
Copy link
Contributor

I assigned this to the first person on the list with less than 2 issues. I appreciate that sometimes issues have PRs close to merging, but it becomes too much for us to manage and track without strictly following the 2 issues rules. Thank you for understanding this policy.

@AVtheking
Copy link
Contributor

AVtheking commented Feb 4, 2024

ok sir

@AVtheking
Copy link
Contributor

AVtheking commented Feb 4, 2024

this is the User model after moving app related fields to AppUser model
image
and this is AppUser model
image

are these schemas correct or require any changes ?

@palisadoes
Copy link
Contributor Author

palisadoes commented Feb 4, 2024

  1. We may have situations where someone attends an event and wants to join the organization, but doesn't wan to use the app. This is will help the Admin keep in contact with them somehow. They will need to be added to the DB and made a member of an organization at the door of the event. This means that they will have a membership request, but just not submitted by them. This will need to be moved to User.
  2. Similarly, they may register for events at the door too, or ask an admin to do it for them. This will need to be moved to User.

image

Rename AppUser to AppUserProfile for clarity.

@xoldyckk any thoughts?

@palisadoes
Copy link
Contributor Author

FYI - We'll soon be creating issues to convert this from Boolean to [Organization]. This may create conflicts with your PR if it is merged before. Approvals will be per organization, not per API instance (all organizations)

image

@palisadoes
Copy link
Contributor Author

@AVtheking Please proceed. The solution was mostly agreed upon in that referenced issue. The fields with the suggested modifications will meet our needs.

@AVtheking
Copy link
Contributor

Ok sir

@AVtheking
Copy link
Contributor

@palisadoes sir should I deprecate the updateUserType mutation as there will be no userType field in the User model ,or modify it to make the user superUser?

@AVtheking
Copy link
Contributor

One more thing @palisadoes sir in organization model should I change the admin field refrence to AppUserProfile or it is okay to be refrenced to the User ?

@palisadoes
Copy link
Contributor Author

User

@AVtheking
Copy link
Contributor

@palisadoes sir should I deprecate the updateUserType mutation as there will be no userType field in the User model ,or modify it to make the user superUser?

what about this sir ?

@palisadoes
Copy link
Contributor Author

@palisadoes sir should I deprecate the updateUserType mutation as there will be no userType field in the User model ,or modify it to make the user superUser?

Yes

@palisadoes
Copy link
Contributor Author

Remember that you'll need to update:

  1. The imported sample data
  2. How the SUPER_ADMIN_OF_LAST_RESORT process works
  3. Many of the user related queries and updates

@AVtheking
Copy link
Contributor

AVtheking commented Feb 6, 2024

Ok sir
I have updated most of the queries ,tests and sample data are remaining

This was referenced Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Ready 4 PR Review
Projects
None yet
Development

No branches or pull requests

9 participants