[DUOS-1967][risk=no] Only show registered users in `Add New Researcher` modal #1737

connorlbark · 2022-08-05T17:21:53Z

Addresses

https://broadworkbench.atlassian.net/browse/DUOS-1967

Have you read Terra's Contributing Guide lately? If not, do that first.

Label PR with a Jira ticket number and include a link to the ticket
Label PR with a security risk modifier [no, low, medium, high]
PR describes scope of changes
Get a minimum of one thumbs worth of review, preferably two if enough team members are available
Get PO sign-off for all non-trivial UI or workflow changes
Verify all tests go green
Test this change deployed correctly and works on dev environment after deployment

shaemarks

Approach makes sense to me. The only thing I'm wondering is if there are liability reasons for needing the lca text available on the page

rushtong · 2022-08-05T18:10:59Z

@connorlbark - let's bring this up with Jonathan/Pamela. From my poor memory, we do want to be able to add a library card for an unregistered user - that was an original design goal. There was some logic somewhere that would look for unregistered user LCs and link them up if a new user signed in with the email associated to the LC.

connorlbark · 2022-08-08T14:51:24Z

@connorlbark - let's bring this up with Jonathan/Pamela. From my poor memory, we do want to be able to add a library card for an unregistered user - that was an original design goal. There was some logic somewhere that would look for unregistered user LCs and link them up if a new user signed in with the email associated to the LC.

Talked to Pamela - so you're right that the ability to type in a freetext email is desirable, but the ability to see users without insitutions is not. I will add back the Add New Researcher popup to this PR but remove the users without institutions.

shaemarks · 2022-08-08T18:31:23Z

src/components/signing_official_table/SigningOfficialTable.js

@@ -198,6 +198,10 @@ const generateLcaText = (text) => {
  </ReactMarkdown>;
 };

+const onlyResearchersWithoutCardFilter = (researcher) => {
+  return isNil(researcher.libraryCards);


I can imagine an edge case where a researcher belongs to this institution, doesn't have a library card from this institution, but does have another library card (since admins can technically give researchers library cards from any institution). In that case, they'll appear in the table but not the dropdown in the modal. Not sure how big of an issue this is though

Great catch @shaemarks ... and you're right. There will be cases where a user will have library cards from other institutions.

changed the filter here and also the filter in the table itself to check that the SO's institution id the same as the card's institution id

rushtong · 2022-08-10T11:50:34Z

FYI, the modal has a couple of issues. The prompt text and selection lets me add new users via email. It would be nice to clean up the button alignment as well if possible.

rushtong

Looking good, see comments inline.

src/components/signing_official_table/SigningOfficialTable.js

rushtong · 2022-08-10T11:32:17Z

src/pages/SigningOfficialResearchers.js

@@ -20,13 +19,7 @@ export default function SigningOfficialResearchers() {
      try {
        setIsLoading(true);
        const soUser = await User.getMe();
-        const soPromises = await Promise.all([
-          User.list(USER_ROLES.signingOfficial),
-          User.getUnassignedUsers()


This was the only usage of User.getUnassignedUsers() so we can remove it, along with deprecating the API method in consent: GET /api/user/institution/unassigned

it's also used in SigningOfficialDataSubmitters.js; is that deprecated code? (also signing official console, but that is being deprecated)

Looks like I was incorrect ... it is used in three places in develop right now, not sure how I missed them.

SigningOfficialConsole

SigningOfficialDataSubmitters

SigningOfficialResearchers

rushtong · 2022-08-10T11:33:26Z

src/pages/SigningOfficialResearchers.js

-          User.getUnassignedUsers()
-        ]);
-        const researcherList = soPromises[0];
-        const unregisteredResearchers = soPromises[1];


~~Just a note that the naming here was incorrect, these were not unregistered users, they were registered users with no institution selection.~~

Naming is hard. The API call was actually returning unregistered users with a LC for the calling user's institution, so User.getUnassignedUsers() is what is mis-named here.

connorlbark · 2022-08-12T17:12:10Z

FYI, the modal has a couple of issues. The prompt text and selection lets me add new users via email. It would be nice to clean up the button alignment as well if possible.

@rushtong I messaged Pamela about this. Here is what she said about the popup: I think the popup is still helpful because I believe SOs can type in the email of an unregistered user and issue them a Library Card. I think that is still a good feature. I believe the ability to enter freetext emails is good, however the users which displayed when searching were the issue.

I am confused though on the getUnassignedUsers call; we don't need this call, right? In other words, the current behavior of the modal is right, but the feedback/changers here are to update the styling?

rushtong · 2022-08-12T18:37:16Z

@rushtong I messaged Pamela about this. Here is what she said about the popup: I think the popup is still helpful because I believe SOs can type in the email of an unregistered user and issue them a Library Card. I think that is still a good feature. I believe the ability to enter freetext emails is good, however the users which displayed when searching were the issue.

I may have been confused about the goal of the ticket/PR - I was under the impression that the intention was to STOP creating Library Cards for users who are not already registered with the signing official's institution. I would be happy to be wrong here, but it does add some complications to the new designs of Signing Official pages. If so, ignore my feedback about the modal and free text usage.

I am confused though on the getUnassignedUsers call; we don't need this call, right? In other words, the current behavior of the modal is right, but the feedback/changers here are to update the styling?

User.getUnassignedUsers() calls GET /api/user/institution/unassigned which returns a list of users who do not have an institution. If we're not querying for those users, we don't need to make that call here.

connorlbark · 2022-08-12T18:48:27Z

I may have been confused about the goal of the ticket/PR - I was under the impression that the intention was to STOP creating Library Cards for users who are not already registered with the signing official's institution. I would be happy to be wrong here, but it does add some complications to the new designs of Signing Official pages. If so, ignore my feedback about the modal and free text usage.

Sounds good to me! We can also open a dialogue with Pamela/Jonathan if you think the behavior should be different.

shaemarks

Looks good to me! Styling on modal looks much better!

src/components/signing_official_table/SigningOfficialTable.js

Co-authored-by: shaemarks <81024249+shaemarks@users.noreply.github.com>

connorlbark changed the title ~~[DUOS-1967][risk=no] WIP Remove add new researcher from SO console~~ [DUOS-1967][risk=no] Remove add new researcher from SO console Aug 5, 2022

connorlbark marked this pull request as ready for review August 5, 2022 17:39

connorlbark requested a review from a team as a code owner August 5, 2022 17:39

connorlbark mentioned this pull request Aug 5, 2022

[DUOS-1967][risk=no] SOs don't see users not in their institution DataBiosphere/consent#1687

Merged

shaemarks reviewed Aug 5, 2022

View reviewed changes

show only registered users in SO modal

b33f57f

connorlbark force-pushed the DUOS-1967 branch from 930a395 to b33f57f Compare August 8, 2022 17:27

connorlbark marked this pull request as draft August 8, 2022 17:34

filter to only researchers without cards

daee670

connorlbark changed the title ~~[DUOS-1967][risk=no] Remove add new researcher from SO console~~ [DUOS-1967][risk=no] Only show registered users in Add New Researcher modal Aug 8, 2022

connorlbark marked this pull request as ready for review August 8, 2022 18:07

connorlbark requested review from shaemarks and rushtong August 8, 2022 18:08

shaemarks reviewed Aug 8, 2022

View reviewed changes

connorlbark added 3 commits August 9, 2022 16:14

Merge branch 'develop' into DUOS-1967

0abd0b2

filter cards more intelligently

92d5a8a

filter cards more intelligently

218b63f

connorlbark requested a review from shaemarks August 9, 2022 20:26

rushtong requested changes Aug 10, 2022

View reviewed changes

feedback

6388aa1

connorlbark requested a review from rushtong August 12, 2022 20:16

shaemarks approved these changes Aug 17, 2022

View reviewed changes

src/components/signing_official_table/SigningOfficialTable.js Outdated Show resolved Hide resolved

Update src/components/signing_official_table/SigningOfficialTable.js

6380d67

Co-authored-by: shaemarks <81024249+shaemarks@users.noreply.github.com>

rushtong approved these changes Aug 24, 2022

View reviewed changes

Merge branch 'develop' into DUOS-1967

3b5069c

connorlbark merged commit 363e197 into develop Aug 31, 2022

connorlbark deleted the DUOS-1967 branch August 31, 2022 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DUOS-1967][risk=no] Only show registered users in `Add New Researcher` modal #1737

[DUOS-1967][risk=no] Only show registered users in `Add New Researcher` modal #1737

connorlbark commented Aug 5, 2022 •

edited

Loading

shaemarks left a comment

rushtong commented Aug 5, 2022

connorlbark commented Aug 8, 2022

shaemarks Aug 8, 2022

rushtong Aug 8, 2022

connorlbark Aug 9, 2022

rushtong commented Aug 10, 2022

rushtong left a comment

rushtong Aug 10, 2022

connorlbark Aug 12, 2022 •

edited

Loading

rushtong Aug 12, 2022

rushtong Aug 10, 2022 •

edited

Loading

connorlbark commented Aug 12, 2022

rushtong commented Aug 12, 2022

connorlbark commented Aug 12, 2022

shaemarks left a comment

[DUOS-1967][risk=no] Only show registered users in Add New Researcher modal #1737

[DUOS-1967][risk=no] Only show registered users in Add New Researcher modal #1737

Conversation

connorlbark commented Aug 5, 2022 • edited Loading

Addresses

shaemarks left a comment

Choose a reason for hiding this comment

rushtong commented Aug 5, 2022

connorlbark commented Aug 8, 2022

shaemarks Aug 8, 2022

Choose a reason for hiding this comment

rushtong Aug 8, 2022

Choose a reason for hiding this comment

connorlbark Aug 9, 2022

Choose a reason for hiding this comment

rushtong commented Aug 10, 2022

rushtong left a comment

Choose a reason for hiding this comment

rushtong Aug 10, 2022

Choose a reason for hiding this comment

connorlbark Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

rushtong Aug 12, 2022

Choose a reason for hiding this comment

rushtong Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

connorlbark commented Aug 12, 2022

rushtong commented Aug 12, 2022

connorlbark commented Aug 12, 2022

shaemarks left a comment

Choose a reason for hiding this comment

[DUOS-1967][risk=no] Only show registered users in `Add New Researcher` modal #1737

[DUOS-1967][risk=no] Only show registered users in `Add New Researcher` modal #1737

connorlbark commented Aug 5, 2022 •

edited

Loading

connorlbark Aug 12, 2022 •

edited

Loading

rushtong Aug 10, 2022 •

edited

Loading