[DUOS-2033][risk=no] Update ResearcherProfile to use FormField components #1814

connorlbark · 2022-09-26T15:31:26Z

Addresses

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

I also added the ability to show both the suggested and selected institutions when the page loads.

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

rushtong · 2022-09-27T20:19:28Z

On page load, this is over-writing my existing, selected institution with the suggested values. Checked on dev and it is behaving correctly ... if I have an institution, then that is what is populated.

connorlbark · 2022-09-27T20:22:44Z

On page load, this is over-writing my existing, selected institution with the suggested values. Checked on dev and it is behaving correctly ... if I have an institution, then that is what is populated.

I'm not sure what you mean by override. I changed it to where it will show your institution and also what you have previously suggested in the form if both exist, but you still have your current institution. This is because I (personally) found it confusing when I suggested an institution, and the pagre refreshed but nothing changed on the page. Thus, I added this as a way to give some feedback to the user. I am more than happy to revert if the previous approach is preferable.

rushtong · 2022-09-27T23:25:30Z

I'm not sure what you mean by override. I changed it to where it will show your institution and also what you have previously suggested in the form if both exist, but you still have your current institution. This is because I (personally) found it confusing when I suggested an institution, and the page refreshed but nothing changed on the page. Thus, I added this as a way to give some feedback to the user. I am more than happy to revert if the previous approach is preferable.

What I mean by override is that the institution value I have saved is not what is displayed in the institution field when the page loads. The value that is displayed is from one of the user properties which is incorrect. The two suggested fields are only meant to be displayed when real values are not present. For example, if there is no institution "Fred Flintstone University", I can enter that into the the field and it is saved as a user property. Later on, Pamela or Jonathan gets an email saying "Greg Rushton requested 'Fred Flintstone University'". They can then add that institution and set it as my user's institution. From then on, that is my real institution. The current approach as it works now on all environments is the correct approach so yes, let's re-implement that.

connorlbark · 2022-09-28T14:53:01Z

What I mean by override is that the institution value I have saved is not what is displayed in the institution field when the page loads. The value that is displayed is from one of the user properties which is incorrect. The two suggested fields are only meant to be displayed when real values are not present. For example, if there is no institution "Fred Flintstone University", I can enter that into the the field and it is saved as a user property. Later on, Pamela or Jonathan gets an email saying "Greg Rushton requested 'Fred Flintstone University'". They can then add that institution and set it as my user's institution. From then on, that is my real institution. The current approach as it works now on all environments is the correct approach so yes, let's re-implement that.

Ok, I understand. I still feel showing both the real institution and the suggested institution makes more sense to me as a user, since it provides feedback to the user if they suggest something, since, if not show, that value just disappears (I would think, as a user, that nothing happened). Since it was still populating and showing the real institution alongside the suggested, I thought it could be helpful feedback. That being said, I understand just showing only the current value of the institution, it just seems weird to me from a UX perspective that the user both 'selects valid institution' and 'suggests the creation of a new institution' in the same input box, since, they seem like two different operations to me, and effect different fields.

rushtong · 2022-09-28T15:42:18Z

That being said, I understand just showing only the current value of the institution, it just seems weird to me from a UX perspective that the user both 'selects valid institution' and 'suggests the creation of a new institution' in the same input box, since, they seem like two different operations to me, and effect different fields.

They are two different operations - but for different phases of a user's registration process. This was the intended design that was run through Yvonne/Jonathan/Pamela so if we are going to make changes at this point, we should be going through a design review. I don't have a problem if we choose to do that, but PR reviews are the wrong forum for that discussion.

connorlbark · 2022-09-28T18:34:44Z

They are two different operations - but for different phases of a user's registration process. This was the intended design that was run through Yvonne/Jonathan/Pamela so if we are going to make changes at this point, we should be going through a design review. I don't have a problem if we choose to do that, but PR reviews are the wrong forum for that discussion.

That is very fair - the original behavior should be returned (but it look slike some cypress errors, will work on those)

connorlbark · 2022-09-30T19:14:47Z

@rushtong I have been investigating this cypress error but I cannot seem to get to the bottom of what is causing it. I can replicate the error locally on my e2e tests; it seems to be an error with e2e background sign in authentication. Very odd since I did not touch that side of the code base. Kind of confused at what's causing it. Rerunning it on github doesn't seem to help, so it's a replicable issue. Just no idea what's causing it. Do you have any ideas?

rushtong · 2022-09-30T19:39:48Z

I have been investigating this cypress error but I cannot seem to get to the bottom of what is causing it. I can replicate the error locally on my e2e tests; it seems to be an error with e2e background sign in authentication. Very odd since I did not touch that side of the code base. Kind of confused at what's causing it. Rerunning it on github doesn't seem to help, so it's a replicable issue. Just no idea what's causing it. Do you have any ideas?

I think you need to pull in the latest from dev, your branch doesn't have changes from #1818. That PR moves auth testing to alpha instead of perf.

connorlbark · 2022-10-03T14:07:03Z

I think you need to pull in the latest from dev, your branch doesn't have changes from #1818. That PR moves auth testing to alpha instead of perf.

ahhhh, that got it! thank you!

rushtong

👍🏽

JVThomas

Updates are looking good! There are a few things that I've left comments on.

JVThomas · 2022-10-05T12:01:25Z

src/pages/ResearcherProfile.js

+              institutionId: selectedInstitution?.institutionId,
+              suggestedInstitution: profile.suggestedInstitution,
+              displayText: (
+                (!isNil(selectedInstitution)


I think you can shorthand this to

displayText: selectedInstitution?.name || profile?.suggestedInstitution || ''

JVThomas · 2022-10-05T12:45:32Z

src/pages/ResearcherProfile.js

-                      className: 'form-control'
-                    })
-                  ]),
+                h(FormField, {


The form field error for suggestedSigningOfficial shows up if a user selects a SO from their institution. You can replicate this by selecting an SO from the list, save the changes to your profile, and then either revisit or reload the Researcher Profile Page.

Wasn't quite able to recreate this, but I should have fixed it by only displaying the error if there is no SO id.

The validation error is still there. Are you checking to see if currentSigningOfficial is populated?

Hmm curious, theoretically they should be the same but I added that check too.

JVThomas

LGTM

connorlbark added 2 commits September 26, 2022 11:27

researcher profile use formfield

727c7e1

undo

fdab3fb

connorlbark marked this pull request as ready for review September 26, 2022 15:46

connorlbark requested a review from a team as a code owner September 26, 2022 15:46

Merge branch 'develop' into DUOS-2033

05f81a3

connorlbark added 2 commits September 28, 2022 10:54

Merge branch 'develop' into DUOS-2033

6e98730

feedback

d3efba2

Merge branch 'develop' into DUOS-2033

c157f0b

rushtong approved these changes Oct 5, 2022

View reviewed changes

JVThomas requested changes Oct 5, 2022

View reviewed changes

feedbac

b56c86e

connorlbark requested a review from JVThomas October 5, 2022 19:29

maybe fix?

9824bcb

JVThomas approved these changes Oct 11, 2022

View reviewed changes

Merge branch 'develop' into DUOS-2033

1f42ffd

connorlbark merged commit a6d49f2 into develop Oct 11, 2022

connorlbark deleted the DUOS-2033 branch October 11, 2022 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DUOS-2033][risk=no] Update ResearcherProfile to use FormField components #1814

[DUOS-2033][risk=no] Update ResearcherProfile to use FormField components #1814

connorlbark commented Sep 26, 2022

rushtong commented Sep 27, 2022

connorlbark commented Sep 27, 2022

rushtong commented Sep 27, 2022

connorlbark commented Sep 28, 2022 •

edited

Loading

rushtong commented Sep 28, 2022

connorlbark commented Sep 28, 2022

connorlbark commented Sep 30, 2022

rushtong commented Sep 30, 2022

connorlbark commented Oct 3, 2022

rushtong left a comment

JVThomas left a comment

JVThomas Oct 5, 2022

JVThomas Oct 5, 2022

connorlbark Oct 5, 2022 •

edited

Loading

JVThomas Oct 6, 2022

connorlbark Oct 6, 2022

JVThomas left a comment

[DUOS-2033][risk=no] Update ResearcherProfile to use FormField components #1814

[DUOS-2033][risk=no] Update ResearcherProfile to use FormField components #1814

Conversation

connorlbark commented Sep 26, 2022

Addresses

rushtong commented Sep 27, 2022

connorlbark commented Sep 27, 2022

rushtong commented Sep 27, 2022

connorlbark commented Sep 28, 2022 • edited Loading

rushtong commented Sep 28, 2022

connorlbark commented Sep 28, 2022

connorlbark commented Sep 30, 2022

rushtong commented Sep 30, 2022

connorlbark commented Oct 3, 2022

rushtong left a comment

Choose a reason for hiding this comment

JVThomas left a comment

Choose a reason for hiding this comment

JVThomas Oct 5, 2022

Choose a reason for hiding this comment

JVThomas Oct 5, 2022

Choose a reason for hiding this comment

connorlbark Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

JVThomas Oct 6, 2022

Choose a reason for hiding this comment

connorlbark Oct 6, 2022

Choose a reason for hiding this comment

JVThomas left a comment

Choose a reason for hiding this comment

connorlbark commented Sep 28, 2022 •

edited

Loading

connorlbark Oct 5, 2022 •

edited

Loading