[web] Add option to select pending users
ClosedPublic
Actions

Authored by tomek on Jun 21 2022, 5:38 AM.

Details

Reviewers

• jacek
atul

Commits

rCOMM071e53347dbc: [web] Add option to select pending users

Summary

Introduce a set to which we add users after clicking their button. Filter out the selected users so that they can be added only once.

Depends on D4307

Test Plan

Render the component and add some users - every user should disappear from the list after being added.

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

tomek requested review of this revision.Jun 21 2022, 5:43 AM

web/settings/relationship/add-users-list.react.js
108 ↗	(On Diff #13622)	This will cause the insertion to take `O(nlogn)` time - which shouldn't be an issue because the number of users that would be added is small. The best possible complexity here is `O(logn)` if we use a tree, but that would require modifying it - which won't work with the state. We could store the tree outside of the state and somehow force a render, but this is too complicated. If we want to store pending users in state, we can use e.g. insertion from insertion sort, and that would be `O(n)`, but it is significantly more complicated than the current solution and the difference between `O(n)` and `O(nlogn)` isn't significant for us (only a handful of elements).

atul added inline comments.

web/settings/relationship/add-users-list.react.js
96–100 ↗	(On Diff #13622)	Maybe something like this to reduce level of indentation? It's less verbose, but not sure that it's easier to read? if (pendingUsers.some(({id}) => id === userID) { return pendingUsers; }
108 ↗	(On Diff #13622)	Insertion from insertion sort makes sense since we can (probably) assume that the previous `pendingUsers` is in sorted order. But like you said, it probably doesn't make much of a difference in practice... so maybe not worth it if it's too involved?

This revision is now accepted and ready to land.Jun 21 2022, 11:48 AM

web/settings/relationship/add-users-list.react.js
96–100 ↗	(On Diff #13622)	Agree, using more functional approach is a good idea.
108 ↗	(On Diff #13622)	I think that using a function `sortedIndex` from lodash might make this more performant while still readable - will give it a try.

web/settings/relationship/add-users-list.react.js
108 ↗	(On Diff #13622)	I tried using `sortedIndexBy` from lodash and it almost works. The problem is that in lodash we specify a prop by which items are sorted - in this case a `username`. Lodash then compare the props and decide the index based on it. This approach is inconsistent with our sorting procedure where we use `localeCompare`. Using lodash would result in differences between ordering in tags and list.