teammates icon indicating copy to clipboard operation
teammates copied to clipboard

Provide admin a UI to update account data of instructors

Open damithc opened this issue 2 years ago • 14 comments

Context: An admin can search for a user and use the 'Mange this account' link to go the account details page of an instructor e.g.,

image

Suggestion: provide a way to update the following data:

  1. Name
  2. Email
  3. Institute(s)

Rationale: for cases such as fixing a typo noticed after the user has used TEAMMATES for awhile

Most likely this is more complicated than simply updating a few fields in the database

Perhaps senior devs can weigh in on the implementation implications

damithc avatar May 26 '22 09:05 damithc

@damithc I think we can update the details to be editable, and since we already have the API to update instructors, we only need to update the frontend, unless I miss something

daongochieu2810 avatar May 28 '22 14:05 daongochieu2810

@damithc I think we can update the details to be editable, and since we already have the API to update instructors, we only need to update the frontend, unless I miss something

You may be right; perhaps it's a simple API call. I'm not sure how the editing of an institute name work, for example, to fix a typo. If the typo'ed institute name has been used in multiple courses, do I update it in each course individually? I'm still fine to do so if that is required. Another use case is if the institute changed their email domain (e.g., 'nus.edu.sg' to 'u.nus.edu'). How can we help a user change the email address if it has been used in multiple courses?

More generally, from the user POV, their TEAMMATES account has one name, one email, and possibly multiple institutes (but usually one). If a user asks us to change one of them (e.g., name), am I able to change it in one place and make the change propagate everywhere? Or perhaps the change will only apply to future courses? I think probably we need the advice from @samuelfangjw and/or @wkurniawan07 on this one.

damithc avatar May 28 '22 15:05 damithc

@damithc we have updateInstructorCascade, so the changes will be propagated everywhere

daongochieu2810 avatar May 29 '22 10:05 daongochieu2810

@damithc we have updateInstructorCascade, so the changes will be propagated everywhere

I see. Just to check, suppose I'm fixing a typo in the institute name. Does the change propagate to all Course objects that has the same typo (because institute name is stored in Course objects).

damithc avatar May 29 '22 11:05 damithc

Comments, responses and deadline extensions will be updated if instructor is updated Fixing institute name = updating courses, and similarly we have updateCourseCascade for this, so yes the changes will be propagated

daongochieu2810 avatar May 29 '22 11:05 daongochieu2810

Comments, responses and deadline extensions will be updated if instructor is updated Fixing institute name = updating courses, and similarly we have updateCourseCascade for this, so yes the changes will be propagated

I see. It's fine, as long as the feature caters for typical use cases.

damithc avatar May 29 '22 12:05 damithc

Name & Email (tied to Instructor/Student object): This page actually has a slightly misleading way of showing name and email. Just looking at this page, one would assume that the name and email associated with the account is the name and email associated with all the courses listed. In actual fact, the name and email associated with the account (shown at the top of the page) and each course are independent and is currently updated independently.

Institute (tied to Course object): I don't believe update of institute is implemented yet (we currently only update it for testcases?) but should be able to update this for each course independently. I can't think of any issues with this at the moment. A note is that we currently do not allow updating of institute by instructors. If we do allow admins to do this, will have to add appropriate access control checks.

samuelfangjw avatar May 29 '22 14:05 samuelfangjw

@samuelfangjw so we need to add an endpoint to update all courses associated with the updated instructor and allow only the admin to do this?

daongochieu2810 avatar May 31 '22 12:05 daongochieu2810

@damithc Do you think this UI is ok? Its similar to how we update a question Screenshot from 2022-05-31 22-14-54

daongochieu2810 avatar May 31 '22 14:05 daongochieu2810

Do you think this UI is ok?

I don't think we need a multi-line text area, as these fields are single-line values only.

damithc avatar May 31 '22 14:05 damithc

@samuelfangjw so we need to add an endpoint to update all courses associated with the updated instructor and allow only the admin to do this?

I'm not intimately familiar with how this aspect of the system but I don't think this issue is as straightforward as it seems.

Firstly, the UI is not really suited to updating the name and email because the name and email of the instructor/student associated with each of the individual courses and the associated account are independent and can differ. If we were to support updating the name and email, we would likely have to find a way to display the name and email used for each course as well and allow each one to be updated independently, but this is not as simple as it seems because the data is not readily available in the course object (used to display each row of the table).

The current proposed UI for updating email and name suggests that either:

  1. It will only update the account's name & email, which I don't believe is used much (if at all) outside of admin pages so will have little effect from the instructor's POV.
  2. It will update the names and emails of all courses associated with the instructor, in which case the admin will be updating these name blindly, because we can't see from this page what the original name/email is for each course.

On a side note, instructors with appropriate permissions can already update the name and email of registered instructors, so in my view we might need to consider if there is enough value to adding this feature for name and email.

Secondly, for institute, while I can't think of any issues from a developers POV there might be some subtle issues missed because we now restrict creation of courses based on institute. Some possible considerations:

  1. Do we need to propagate this change to the original AccountRequest entity? This will likely depend on how we plan to use this entity in the future.
  2. For legacy courses where any instructor could make courses, there may be courses created by other instructors whose institutes may have the same typo as well. Do we need to propagate this change to all other courses as well?

As for the access control issue, I think it might be ok to reuse the endpoint with added access control checks (from my pov, there seems to be some precedent for this as many actions currently have different behaviour depending on intent/user type). I'm not really familiar with the pros and cons of separating vs reusing existing endpoints though.

samuelfangjw avatar May 31 '22 15:05 samuelfangjw

  1. It will only update the account's name & email, which I don't believe is used much (if at all) outside of admin pages so will have little effect from the instructor's POV.

Previously, those fields from the Account object were copied over when the instructor creates a new course. I guess that is no longer true in the current implementation.

On a side note, instructors with appropriate permissions can already update the name and email of registered instructors, so in my view we might need to consider if there is enough value to adding this feature for name and email.

Good point @samuelfangjw I don't think we need to give admin a way to do this.

Secondly, for institute, while I can't think of any issues from a developers POV there might be some subtle issues missed because we now restrict creation of courses based on institute. Some possible considerations:

  1. Do we need to propagate this change to the original AccountRequest entity? This will likely depend on how we plan to use this entity in the future.
  2. For legacy courses where any instructor could make courses, there may be courses created by other instructors whose institutes may have the same typo as well. Do we need to propagate this change to all other courses as well?

If it is simpler, I don't mind having to update the institute in each individual course. I'm not sure if the change should be propagated to the account request object, as I myself am not sure how we'll be using that in the future.

damithc avatar May 31 '22 15:05 damithc

Previously, those fields from the Account object were copied over when the instructor creates a new course. I guess that is no longer true in the current implementation.

It actually is still true. But with that said, this is still the only usage of account name + email outside of admin's page. The relationship between account name + email and instructor name + email is something that we really need to re-look at in Authentication Plus.

I'm not sure if the change should be propagated to the account request object, as I myself am not sure how we'll be using that in the future.

AccountRequest if anything is more of a historical record; especially so when the burden of registration is shifted entirely to users. That means once created it should not be mutable (other than the registration date), i.e. mistakes should remain as mistakes.

For legacy courses where any instructor could make courses, there may be courses created by other instructors whose institutes may have the same typo as well.

This depends on whether we want to maintain a central database or source of truth for institutes. Definitely something to consider for SQL database, but far from a hard requirement.

wkurniawan07 avatar Jun 02 '22 16:06 wkurniawan07

@wkurniawan07 @damithc should we keep this issue on hold until Auth Plus is done?

daongochieu2810 avatar Jun 07 '22 02:06 daongochieu2810