Add Advisor Role

[oscarwang20]

Summary

This PR adds the isAdvisor field to IDOL members. All members are not advisors by default. Admins can add advisors by setting the form field to "yes" in admin/edit. There can be advisors for every role except TPM.

Dev Portfolios have also been adjusted since dev advisors are only required to submit for documentation.

Test Plan

Manually edited and created users, modifying the field and seeing the changes update in the db.

Edit User

• changed advisor field around for existing members -> changes reflected in the database

Create User

• new users are not advisors by default, this is reflected in the frontend in the admin/edit page and in the backend once the new user is created

Upload CSV

• changed the template to reflect the addition of the isAdvisor field
  • this is an optional field just like website, bio, etc... so there is no consequence to leaving it out it'll just default to false

Dev Portfolio

• made sure submissions without PRs don't emit errors for advisors

Notes

• scripts/add-advisor.ts is being used to add the field to all members on the db; will stay up for reference and be removed before this pr gets merged
• will manually change the two/three advisors we have

[dti-github-bot]

[diff-counting] Significant lines: 66.

[patriciaahuang]

Looks good! Just make sure to run the formatter.

[alyssayzhang]

This looks good to me! I see that there is no TPM advisor and it's disabled for TPMs - is this the same for leads role?

[oscarwang20]

This looks good to me! I see that there is no TPM advisor and it's disabled for TPMs - is this the same for leads role?

According to the leads, a lead advisor is possible

[cchrischen]

I found a minor bug while testing. To reproduce:

1. Change the role of a user to something not tpm.
2. Change isAdvisor to true.
3. Change the role back to tpm.
   The admin can now save the user.
   
   This bug is unlikely to show up in production, and admin still has to review these changes after it is saved. However, to be safe, a check to enforce that an advisor cannot have the role of TPM can be put in setUser.

[andrew032011]

I found a minor bug while testing. To reproduce:

1. Change the role of a user to something not tpm.
2. Change isAdvisor to true.
3. Change the role back to tpm.
   The admin can now save the user.
   
   This bug is unlikely to show up in production, and admin still has to review these changes after it is saved. However, to be safe, a check to enforce that an advisor cannot have the role of TPM can be put in setUser.

+1 Good catch @cchrischen!

[kevinmram]

The addition of the 'isAdvisor' field definitely LGTM, streamlining the role management process! However, maybe it'd be beneficial to consider implementing a more robust validation check when toggling the advisor status, especially when changing roles to ensure consistency (could prevent potential oversights and reinforce the integrity of the role management system). 🐐

[andrew032011]

nit: maybe add "Note that previous TPMs may become dev advisors."

Also, just sanity checking: business advisor, and PM advisor are both a thing?

[oscarwang20]

Ops Leads say so. I'm waiting to hear back from the other role leads about this too. We can probably double check during leads sync.

[andrew032011]

Ok! Let's leave it then!

[andrew032011]

Nice, thanks for the detailed PR description too! Makes this PR easy to review.

Another question: are there plans to adjust the way Dev Portfolio/TEC behaves for advisors? Off the top of my head advisors have the same TEC requirement, but not the same Dev Portfolio requirement (for Dev Advisors).

[oscarwang20]

Nice, thanks for the detailed PR description too! Makes this PR easy to review.

Another question: are there plans to adjust the way Dev Portfolio/TEC behaves for advisors? Off the top of my head advisors have the same TEC requirement, but not the some Dev Portfolio requirement (for Dev Advisors).

Nice, thanks for the detailed PR description too! Makes this PR easy to review.

Another question: are there plans to adjust the way Dev Portfolio/TEC behaves for advisors? Off the top of my head advisors have the same TEC requirement, but not the some Dev Portfolio requirement (for Dev Advisors).

Yep, for dev advisors the Dev Portfolio only requires a documentation submission. I'll modify the code to reflect that.

[andrew032011]

I would just split this into two lines. Easier to read

[cchrischen]

Looks great!

[henry-li-06]

This kinda change raises a bunch of changes questions for me:

• What's the purpose of adding this new role? Is it to add the ability for finer management of policies by role (e.g. different grading requirements)? Is it for finer access management within the IDOL system? Is it for display on the new website?
• As @andrew032011 kinda already hinted at, is it clear what the distinction between the role and the corresponding advisor role? Should members of the role and members of the corresponding advisor role have the same permissions? If not, is the relationship between the role and the corresponding advisor role's permission always the same across all the roles? For example, would the relationship between the permissions for a lead and lead advisor be the same as for a dev and dev advisor?

The existing roles in IDOL aren't really meant to fully capture authorization policies within IDOL or other policies like grading policies. All of those things are kinda crammed together into the existing roles, but the main purpose of it at the time was to reflect the roles that are displayed on the website. (On a side note, consider updating those because there's new roles since IDOL was originally built that aren't accounted for like PMM, APM, etc.) The purpose of #471 is to separate the concerns of those roles and roles used for access management. It also allows for better flexibility for access management. Something similar could be used to define grading policies as well such that it's all in a single configuration file instead of scattered across the codebase.

But those are just some broader questions and ideas.

As for this PR itself if those questions/ideas aren't relevant b/c I'm missing a lot of context:

• Using a boolean flag for this might not be the best idea. Booleans are very restrictive by nature and using them increases the chance of needing some refactor or type change + database migration in the future. It might be better to consider some kind of attributes associated with each user instead. For example

  interface IdolMember {
      ...,
      role_attributes: ["advisor", <other role attributes that can be added in the future>]
  }

  and you can determine if someone is an advisor by

  user.role_attributes.some(attr => attr === 'adivsor')

  Or something along those lines. The main idea here is that something like that would be much more extensible and
  would be much easier to make changes to things related to it.
• re: tpm advisors - So I understand that advisors shouldn't exist for tpms. I see that this is enforced on the FE, but this enforcement also needs to exist on the BE logic. As a rule of thumb, validation always needs to exist on the BE, but of course it's better if it's on the FE as well. (EDIT: This is the 1 thing that's 100% relevant to this PR)

Also feel free to ignore the first thing about the boolean flag if that's not really in the scope of this work.

P.S. good catch on the bug @cchrischen :D We love some bug bashing 🐛

[JustKong13]

Just to introduce myself and provide a little bit of context, I'm the current Ops Lead of DTI and I think I'm more suited to answer these questions since these were decisions I was involved in.

This kinda change raises a bunch of changes questions for me:

• What's the purpose of adding this new role? Is it to add the ability for finer management of policies by role (e.g. different grading requirements)? Is it for finer access management within the IDOL system? Is it for display on the new website?

This new role was created by the leads team, not IDOL, curated specifically for senior members of the team who don't want to do full time DTI work, but are willing to still help out newer members of DTI and impart their wisdom. They are functionally different from general members in the sense that they have significantly less (if any) deliverables. For example, a dev advisor does not have to submit any PRs as supposed to general developers, but they are required to submit code documentation instead.

In the future when we do create the new website, I expect the new advisor role to be accurately reflected on the new website as well.

• As @andrew032011 kinda already hinted at, is it clear what the distinction between the role and the corresponding advisor role? Should members of the role and members of the corresponding advisor role have the same permissions? If not, is the relationship between the role and the corresponding advisor role's permission always the same across all the roles? For example, would the relationship between the permissions for a lead and lead advisor be the same as for a dev and dev advisor?

This was answered a little bit in the first question, but the relationship between the general role and the role advisor varies on the role, so the nature of the relationship is dependent on role. The reasoning for this is that each of the roles are functionally very different, and thus the points of emphasis for the advisor varies. If you are at all curious, here are the expectations of all the advisor roles: https://docs.google.com/document/d/1SaxycR6-Dn-22LAPY10SNK526UrxXVr9s2c2F9LwEo4/edit?usp=sharing

In terms of IDOL permissions, to my knowledge the only notable change at the moment is that dev advisors do not have to submit PRs in their dev portfolios, where as devs are required to. Everything else should remain the same.

The existing roles in IDOL aren't really meant to fully capture authorization policies within IDOL or other policies like grading policies. All of those things are kinda crammed together into the existing roles, but the main purpose of it at the time was to reflect the roles that are displayed on the website. (On a side note, consider updating those because there's new roles since IDOL was originally built that aren't accounted for like PMM, APM, etc.) The purpose of #471 is to separate the concerns of those roles and roles used for access management. It also allows for better flexibility for access management. Something similar could be used to define grading policies as well such that it's all in a single configuration file instead of scattered across the codebase.

It's actually very interesting what the purpose of roles on IDOL started off as, but I think our vision going into the (long-term) future is to have IDOL be a managerial tool for the leads, in order to keep track of assignments, policies, rubrics, and whatnot, although our current short-term goal is to get the new website up. 😃

But those are just some broader questions and ideas.

As for this PR itself if those questions/ideas aren't relevant b/c I'm missing a lot of context:

• Using a boolean flag for this might not be the best idea. Booleans are very restrictive by nature and using them increases the chance of needing some refactor or type change + database migration in the future. It might be better to consider some kind of attributes associated with each user instead. For example

  interface IdolMember {
      ...,
      role_attributes: ["advisor", <other role attributes that can be added in the future>]
  }

  and you can determine if someone is an advisor by

  user.role_attributes.some(attr => attr === 'adivsor')

  Or something along those lines. The main idea here is that something like that would be much more extensible and
  would be much easier to make changes to things related to it.

• re: tpm advisors - So I understand that advisors shouldn't exist for tpms. I see that this is enforced on the FE, but this enforcement also needs to exist on the BE logic. As a rule of thumb, validation always needs to exist on the BE, but of course it's better if it's on the FE as well. (EDIT: This is the 1 thing that's 100% relevant to this PR)

Also feel free to ignore the first thing about the boolean flag if that's not really in the scope of this work.

P.S. good catch on the bug @cchrischen :D We love some bug bashing 🐛

I'll leave the remainder of this comment to the devs on IDOL as that information is more suited towards them (but also I agree the backend should be protected as well), but hope that clarified some things!

[henry-li-06]

Hey @JustKong13!

This new role was created by the leads team, not IDOL, curated specifically for senior members of the team who don't want to do full time DTI work, but are willing to still help out newer members of DTI and impart their wisdom. They are functionally different from general members in the sense that they have significantly less (if any) deliverables. For example, a dev advisor does not have to submit any PRs as supposed to general developers, but they are required to submit code documentation instead.

In the future when we do create the new website, I expect the new advisor role to be accurately reflected on the new website as well.

These questions aren't asked with the intention of questioning why the roles were added into to IDOL or why the roles exist within DTI. Rather, these questions pose engineering questions of the longer-term vision of what the introduction of new or modified role system can hope to achieve.

This was answered a little bit in the first question, but the relationship between the general role and the role advisor varies on the role, so the nature of the relationship is dependent on role. The reasoning for this is that each of the roles are functionally very different, and thus the points of emphasis for the advisor varies. If you are at all curious, here are the expectations of all the advisor roles: https://docs.google.com/document/d/1SaxycR6-Dn-22LAPY10SNK526UrxXVr9s2c2F9LwEo4/edit?usp=sharing

That's the answer I was expecting! This again is another engineering question, not a question to challenge the difference in policy between the general role and the advisor role. The idea of this question is to guide the current team to an understanding that the current implementation is not extensible. The current implementation of a set of fixed "general" roles with a boolean flag to indicate an advisor role (if you look at the actual PR) which requires many conditional checks to determine the correct policies to apply to a given user. It's also not very declarative this way which can be potentially confusing.

In terms of IDOL permissions, to my knowledge the only notable change at the moment is that dev advisors do not have to submit PRs in their dev portfolios, where as devs are required to. Everything else should remain the same.

This is what I was referring to as "grading policies" in my original comment. Given the usage of IDOL, I would say there are essentially 2 types of policies:

1. Grading policies - These are policies on roles that determine the grading requirements for the
2. Authorization policies - What permissions are granted to a particular role

While grading policies may not have too many notable changes at the moment, it potentially will also be very un-declarative in the future if there becomes more differences in the grading policies between general roles and advisor roles.

As for authorization policies, there is the question of whether the advisor role should be treated the same as the general role when it comes to access control. For instance, many admin level checks check that a user is a lead or admin. That poses the question - should an advisor lead be granted to full admin permission on IDOL as other general role leads? While the answer may very well be yes, it's a question to ask. Another example would be for Candidate Decider (context for what this is isn't that important). Access to specific instances can be granted by user roles such as dev. Should this apply to dev advisors as well?

Point is that just because it's simple right now doesn't mean that the underlying system should be haphazardly built without considering the implications. There's of course a tradeoff of premature abstraction vs spaghetti code but asking questions about these things is where those conversations start.

The existing roles in IDOL aren't really meant to fully capture authorization policies within IDOL or other policies like grading policies. All of those things are kinda crammed together into the existing roles, but the main purpose of it at the time was to reflect the roles that are displayed on the website. (On a side note, consider updating those because there's new roles since IDOL was originally built that aren't accounted for like PMM, APM, etc.) The purpose of #471 is to separate the concerns of those roles and roles used for access management. It also allows for better flexibility for access management. Something similar could be used to define grading policies as well such that it's all in a single configuration file instead of scattered across the codebase.

It's actually very interesting what the purpose of roles on IDOL started off as, but I think our vision going into the (long-term) future is to have IDOL be a managerial tool for the leads, in order to keep track of assignments, policies, rubrics, and whatnot, although our current short-term goal is to get the new website up. 😃

The original purpose of roles on IDOL is the same as it is now - The dual purpose of serving as a display on the website as well as handling authorization within the IDOL platform. And it looks like those 2 primary goals isn't changing anytime soon. But given that the IDOL platform is getting more complex, a change like this to update the role system should ideally take into account how to setup the role system to be extensible enough in the future. That is to build the system in a way that avoids wasteful refactors and data changes. Again just engineering problems :D

I'll leave the remainder of this comment to the devs on IDOL as that information is more suited towards them (but also I agree the backend should be protected as well), but hope that clarified some things!

Yup thanks for clarifying things. Again all the questions are more engineering related to guide better engineering discussion but still ultimately achieve the requirements that are asked for.

cc @andrew032011 @oscarwang20 @JasonMun7 as this comment explains the reasoning in my previous comment