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

Refactor adminIds in UserList #388

Open
matthew-white opened this issue Nov 29, 2020 · 0 comments
Open

Refactor adminIds in UserList #388

matthew-white opened this issue Nov 29, 2020 · 0 comments
Labels
refactor Improves code without altering behavior

Comments

@matthew-white
Copy link
Member

matthew-white commented Nov 29, 2020

Right now in UserList, the data property adminIds is a Set that contains the ids of users who are sitewide administrators. We keep the Set up-to-date, adding or removing an id after a user's role is changed. However, a Set is not reactive, which means that Vue will not automatically react to an addition or removal. I have confirmed this using Vue devtools:

  • A UserRow is passed an admin prop based on whether the associated user's id is in the Set.
  • If the user's role is changed, the admin prop does not change.

I think the reason why things work today is that UserRow has its own data property, selectedRole, that indicates the user's currently selected role. UserRow uses the admin prop to initialize selectedRole, but it does not react to changes to admin.

If adminIds is only used to initialize UserRow, then I think we should make that explicit in a comment and remove the code that tries to keep adminIds up-to-date — since Vue won't see those changes anyway.

Alternatively, we could change adminIds from a Set to a simple object so that Vue reacts to changes to it. We could then have UserRow only use the admin prop, removing the selectedRole data property. Maybe we could use v-model?

I think something similar is happening in ProjectUserList. After a user's role is changed, we update the roleId property of the associated element of projectAssignments. However, I think the roleId property is just used to initialize ProjectUserRow, which has its own selectedRoleId data property. There are some differences from UserList though. First, ProjectUserList does not use a Set, so Vue will react to the change. Second, although an existing ProjectUserRow will not react to an update of the roleId property, we regularly destroy and recreate ProjectUserRow components as part of user search.

@matthew-white matthew-white added the refactor Improves code without altering behavior label Nov 29, 2020
matthew-white added a commit that referenced this issue Jan 18, 2022
- Closes #388.
- requestData.actors and requestData.projectAssignments are now Maps.
- Don't disable search in ProjectUserList. This means that it is now
  possible for the table to change while a request to change an
  assignment is in progress, a race condition similar to those that we
  allow in UserList.
- ProjectUserRow now mostly uses the role `system` property of the role,
  not the role id. This simplifies some things and matches how UserRow
  works.
- Other changes to make UserList and ProjectUserList more similar and
  UserRow and ProjectUserRow more similar
@matthew-white matthew-white linked a pull request Jan 19, 2022 that will close this issue
matthew-white added a commit that referenced this issue Jan 20, 2022
- Closes #388.
- requestData.actors and requestData.projectAssignments are now Maps.
- Don't disable search in ProjectUserList. This means that it is now
  possible for the table to change while a request to change an
  assignment is in progress, a race condition similar to those that we
  allow in UserList.
- ProjectUserRow now mostly uses the role `system` property of the role,
  not the role id. This simplifies some things and matches how UserRow
  works.
- Other changes to make UserList and ProjectUserList more similar and
  UserRow and ProjectUserRow more similar
@matthew-white matthew-white removed a link to a pull request May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Improves code without altering behavior
Projects
None yet
Development

No branches or pull requests

1 participant