♻️ Move "User Profile" data into a subtype of User in GraphQL Schema

[tristan-orourke]

♻️ Debt/Refactor

The user model is too big - it encompasses multiple use cases. This makes permission checks and efficient caching difficult. We want to split it up based on use cases. An extension of #8288 and #8221.

🕵️ Details

Most of the User fields can be moved into a UserProfile subtype which excludes things like auth info and notifications.

🙋‍♀️ Proposed Solution

Move the profile data out of the User type and into a UserProfile type in the graphql schema:

type UserProfile {
  id: ID!

  # Personal info
  firstName: String @rename(attribute: "first_name")
  lastName: String @rename(attribute: "last_name")
  email: Email
  telephone: PhoneNumber
  preferredLang: Language @rename(attribute: "preferred_lang")
  preferredLanguageForInterview: Language
    @rename(attribute: "preferred_language_for_interview")
  preferredLanguageForExam: Language
    @rename(attribute: "preferred_language_for_exam")
  currentProvince: ProvinceOrTerritory @rename(attribute: "current_province")
  currentCity: String @rename(attribute: "current_city")
  citizenship: CitizenshipStatus
  armedForcesStatus: ArmedForcesStatus @rename(attribute: "armed_forces_status")

  # Language
  lookingForEnglish: Boolean @rename(attribute: "looking_for_english")
  lookingForFrench: Boolean @rename(attribute: "looking_for_french")
  lookingForBilingual: Boolean @rename(attribute: "looking_for_bilingual")
  bilingualEvaluation: BilingualEvaluation
    @rename(attribute: "bilingual_evaluation")
  comprehensionLevel: EvaluatedLanguageAbility
    @rename(attribute: "comprehension_level")
  writtenLevel: EvaluatedLanguageAbility @rename(attribute: "written_level")
  verbalLevel: EvaluatedLanguageAbility @rename(attribute: "verbal_level")
  estimatedLanguageAbility: EstimatedLanguageAbility
    @rename(attribute: "estimated_language_ability")

  # Gov info
  isGovEmployee: Boolean @rename(attribute: "is_gov_employee")
  govEmployeeType: GovEmployeeType @rename(attribute: "gov_employee_type")
  currentClassification: Classification @belongsTo # Users current classification
  department: Department @belongsTo
  hasPriorityEntitlement: Boolean @rename(attribute: "has_priority_entitlement")
  priorityNumber: String @rename(attribute: "priority_number")

  # Employment equity
  isWoman: Boolean @rename(attribute: "is_woman")
  hasDisability: Boolean @rename(attribute: "has_disability")
  isVisibleMinority: Boolean @rename(attribute: "is_visible_minority")
  indigenousCommunities: [IndigenousCommunity]
    @rename(attribute: "indigenous_communities")
  indigenousDeclarationSignature: String
    @rename(attribute: "indigenous_declaration_signature")

  # Applicant info
  hasDiploma: Boolean
    @rename(attribute: "has_diploma")
    @deprecated(reason: "hasDiploma to be replaced")
  locationPreferences: [WorkRegion] @rename(attribute: "location_preferences")
  locationExemptions: String @rename(attribute: "location_exemptions")
  acceptedOperationalRequirements: [OperationalRequirement]
    @rename(attribute: "accepted_operational_requirements")
  positionDuration: [PositionDuration] @rename(attribute: "position_duration")

  # Experiences
  experiences: [Experience]
    @with(relation: "awardExperiences")
    @with(relation: "communityExperiences")
    @with(relation: "educationExperiences")
    @with(relation: "personalExperiences")
    @with(relation: "workExperiences") # All experiences that a user owns
  awardExperiences: [AwardExperience] @hasMany # Award experiences that a user owns
  communityExperiences: [CommunityExperience] @hasMany # Community experiences that a user owns
  educationExperiences: [EducationExperience] @hasMany # Education experiences that a user owns
  personalExperiences: [PersonalExperience] @hasMany # Personal experiences that a user owns
  workExperiences: [WorkExperience] @hasMany # Work experiences that a user owns

  # Skill Library
  userSkills(includeSkillIds: [UUID] @in(key: "skill_id")): [UserSkill] @hasMany
  topTechnicalSkillsRanking: [UserSkill]
  topBehaviouralSkillsRanking: [UserSkill]
  improveTechnicalSkillsRanking: [UserSkill]
  improveBehaviouralSkillsRanking: [UserSkill]

  # Profile Status
  isProfileComplete: Boolean
  priorityWeight: Int @rename(attribute: "priority_weight")
}

type User {
  id: ID!
  createdDate: DateTime @rename(attribute: "created_at")
  updatedDate: DateTime @rename(attribute: "updated_at")
  deletedDate: DateTime @rename(attribute: "deleted_at")

  # Profile
  userProfile: UserProfile @self

  # Applications and qualified in pools
  poolCandidates: [PoolCandidate]
    @hasMany(scopes: ["authorizedToView"])
    @can(ability: "view", resolved: true) # PoolsCandidate objects associate the user with a pool

  # authorization and roles
  authInfo: UserAuthInfo @self

  # Notification settings
  enabledEmailNotifications: [NotificationFamily]
    @rename(attribute: "enabled_email_notifications")
  enabledInAppNotifications: [NotificationFamily]
    @rename(attribute: "enabled_in_app_notifications")
}

Rename UpdateUserAsUser mutation to UpdateUserProfile. Non-profile fields are already updated with other mutations.

Add a viewProfile method to the UserPolicy, which checks view-own/team/any-applicantProfile. Also, ensure view-own-applicantProfile is added to Applicant role.

Add a query to view UserProfile by id, and use the new UserPolicy.viewProfile method to guard it.

Remove the view-team-applicantProfile permission check from UserPolicy.view method.

Replace the PoolCandidate.user field with PoolCandidate.userProfile in schema, and user viewProfile policy ability.

Remove view-team-applicantProfile check from User::scopeAuthorizedToViewSpecific. (I don't think users with that permission actually need to use any paginated queries - if I am wrong, we may need to define scopeAuthoizedToViewProfile.)

🌎 Localization

(optional) Provide any new copy along with translations available.

✅ Acceptance Criteria

A set of assumptions which, when tested, verify that the debt was addressed and expected functionality has not been affected.

• [ ] Profile data is separated out of the User type and into its own type
• [ ] Client queries are updated to access data properly
• [ ] On the admin side, where possible, query for just the UserProfile and not the entire User object
• [ ] The view-team-applicantProfile permission doesn't give access to full User type anywhere.
• [ ] viewProfile method added to UserPolicy.
• [ ] Applicant role is given view-own-applicantProfile

🚚 Deployment

Run php artisan db:seed --class=RolePermissionSeeder

[vd1992]

What is this issue attempting to do?

[tristan-orourke]

This can be thought of as an extension of #8288, moving logical chunks of data out of the User type. In this case, all the Profile data can logically be considered its own type. Doing this might make it easier to reason about some permissions (maybe some roles can see Profiles but don't need to see the rest of the User object) but unlike with #8288 there isn't such a clear advantage to making this change.

[vd1992]

Doing this might make it easier to reason about some permissions (maybe some roles can see Profiles but don't need to see the rest of the User object) but unlike with #8288 there isn't such a clear advantage to making this change.

@tristan-orourke Still fine with this issue?

[tristan-orourke]

I still think this would be nice, but low value for effort. Very low priority for now.

[tristan-orourke]

In working on Community role changes, I've realized I'm uncomfortable with the way this is set up now. Permissions which are meant to give access to the Profile, currently give access to the whole user object. We rely on putting additional restrictions on specific user fields, if necessary. It's okay for now, but this seems like an easy place to make a mistake. I'm moving this to a higher priority.

[esizer]

@tristan-orourke We have a slight issue and I'm uncertain how to resolve it.

On the ViewPoolCandidatePage we need access to the users pool candidates to show other pools they have applied to. With this change, we have no easy way to do that.

Where we used to do poolCandidate.user.poolCandidates we can no longer to this. I think the only easy way to do this is making multiple queries on the same page :thinking:

// ViewPoolCandidatesPages.tsx
const { id } = poolCandidate.userProfile.id;

// PoolStatusTable.tsx
const res = useQuery({
   query: UserPoolStatuses,
   variables: {
      userId: id
   }
});

The annoying bit is, that table also appears on the admin user profile where we do have access to that info so we would be doing an unnecessary query there :sweat:

Update

Nevermind, looks like that table now only appears on the one page so this isn't that bad now :face_with_spiral_eyes:

[tristan-orourke]

Note to self: I need to address some questions from @esizer:

• User exists as a subfield on other types (Experience, UserSkill), and I wasn't sure if I should leave it like that or switch to userProfile
• We use the poolCandidates.user.poolCandidates relationship and it may be tricky to untangle that
• Wasn't sure what roles had permissions to view User vs UserProfile
• Was making a lot of queries fairly complicated and made passing data around tricky
• We just pass in full user object in a lot of utility functions so TypeScript is not happy about updating them all to UserProfile and in some places, it is not possible and the functions will also need to be refactored

[brindasasi]

@tristan-orourke any update on this ?

[tristan-orourke]

• User exists as a subfield on other types (Experience, UserSkill), and I wasn't sure if I should leave it like that or switch to userProfile
  • My initial position is to replace user with userProfile on UserSkill and the Experience types. If we find a situation for which the user relation is particularly useful, there's no reason we can't maintain both, as long as they are both Policy protected.
• We use the poolCandidates.user.poolCandidates relationship and it may be tricky to untangle that
  • If we think of UserProfile as containing all the data which may need to be visible to anyone besides the user themselves, it seems reasonable to put the poolCandidates relationship inside UserProfile, as well as in User.
• Wasn't sure what roles had permissions to view User vs UserProfile
  • Actually, the only people who should be able to view Users other than themselves should be Platform Admins. Request Responders should have view-any-applicantProfile (along with Platform Admins, though its redundant); and Pool Operator, Process Operator, Community Admin and Community Recruiter all have view-team-applicantProfile.
• Was making a lot of queries fairly complicated and made passing data around tricky
  • Is the new structure actually a problem, or is it just matter of lots of code needing to be updated?
• We just pass in full user object in a lot of utility functions so TypeScript is not happy about updating them all to UserProfile and in some places, it is not possible and the functions will also need to be refactored
  • Yes, I'll admit this could be a big refactoring 😞

[esizer]

• My initial position is to replace user with userProfile on UserSkill and the Experience types. If we find a situation for which the user relation is particularly useful, there's no reason we can't maintain both, as long as they are both Policy protected.

okay :+1: This actually makes me wonder. Do we need userSkill.user* at all? I can't think of anywhere where we need to access the user from a skill. Usually we just get the users skills rather than the other way around :thinking:

• Actually, the only people who should be able to view Users other than themselves should be Platform Admins. Request Responders should have view-any-applicantProfile (along with Platform Admins, though its redundant); and Pool Operator, Process Operator, Community Admin and Community Recruiter all have view-team-applicantProfile.

Would it make sense to rename one of these so they use like terms? Seems like it would be easier to reason about if we do view-any-userProfile instead.

• Is the new structure actually a problem, or is it just matter of lots of code needing to be updated?

I honestly can't remember :sweat_smile: I am leaning toward it just being a lot of code to update.

[tristan-orourke]

okay 👍 This actually makes me wonder. Do we need userSkill.user* at all? I can't think of anywhere where we need to access the user from a skill. Usually we just get the users skills rather than the other way around 🤔

Oh! Yeah, if we don't use that relationship, we can remove it from the schema. That follow's are newish rule of thumb to avoid unnecessary relationships that go "up" the hierarchy.

Would it make sense to rename one of these so they use like terms? Seems like it would be easier to reason about if we do view-any-userProfile instead.

Do you mean, can we give Platform Admins view-any-userProfile instead of view-any-user? That probably makes sense, as long as they separately have permission to view the uesr's AuthInfo.

[esizer]

Do you mean, can we give Platform Admins view-any-userProfile instead of view-any-user? That probably makes sense, as long as they separately have permission to view the uesr's AuthInfo.

Oh no not exactly, from your comment I thought that the permission we would be checking for the user profile info would be applicantInfo so if that were the case I was suggesting aligning the terms so userInfo instead of applicantInfo

[tristan-orourke]

Outcome of discussion:

• We should rename view-team-applicantProfile permissions (and similar) to view-team-userProfile because it's applying to the user (or UserProfile) object.
• If we want to split this up at the schema level, maybe it should also be normalized in the database. User vs Person (vs Applicant?)
• TODO: We should talk to designers to see if this matches how they think about distinctions or if they're on board