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

[DUOS-1964][risk=no] Add DataAccessGovernance section to Data Submission Form #1791

Merged
merged 17 commits into from
Sep 28, 2022

Conversation

connorlbark
Copy link
Contributor

Addresses

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

Replaced the old one (closed that PR) because it had so many conflicts that it was easier to make a new branch from develop.


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

@connorlbark connorlbark changed the title [DUOS-1964][risk=no] WIP Add DataAccessGovernance section to Data Submission Form [DUOS-1964][risk=no] Add DataAccessGovernance section to Data Submission Form Sep 7, 2022
@connorlbark connorlbark marked this pull request as ready for review September 7, 2022 18:45
@connorlbark connorlbark requested a review from a team as a code owner September 7, 2022 18:45
@connorlbark
Copy link
Contributor Author

connorlbark commented Sep 16, 2022

I am not sure why npm audit is failing (bootstrap requirement didn't change?) but this should be ready for re-review

Changes:

  • Search for diseases in ontology
  • More idiomatic / cleaner form field usage
  • Date validation + field for date

The select field has become more complex. It allows both asynchronous and synchronous usage. This could be split into a SELECT and an ASYNCSELECT but there was a lot of overlapping functionality, so I preferred combining them.

@quazi-h
Copy link
Contributor

quazi-h commented Sep 16, 2022

Looking good - I can now select items under "Secondary Data Use Terms".
With the new changes however, section 1.1 Data Custodian is grayed out and I can't enter anything into the form field. I'm also unable to enter anything for section 1.10 Data Access Committee and the drop down doesn't have any DACs listed. If I remember correctly this was working before.

@connorlbark
Copy link
Contributor Author

Looking good - I can now select items under "Secondary Data Use Terms". With the new changes however, section 1.1 Data Custodian is grayed out and I can't enter anything into the form field. I'm also unable to enter anything for section 1.10 Data Access Committee and the drop down doesn't have any DACs listed. If I remember correctly this was working before.

Hmm, curious. The data access committee dropdown works fine locally. Is it possible there is an error on you hitting the DAC endpoint on consent?

Also, I believe the data submitter (I assume you mean submitter? the custodian field also works for me locally) field is supposed to be greyed out - it's prefilled with the current user's information.

Copy link
Contributor

@quazi-h quazi-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took another look and everything on the data_submission_form page is working correctly. Sorry, I was mistakenly looking at the dataset_registration page (Researcher Console -> Dataset Catalog -> Add Dataset) when I was reviewing it last.

id: idx+'_url',
name: 'url',
title: 'Data URL',
validators: [FormValidators.REQUIRED, FormValidators.URL],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor issue on the URL Form validator. www. isn't always representative of a valid url. For example, www.duos.org or https://www.duos.org are invalid urls, but duos.org or https://duos.org will get us to the DUOS production page.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait so what is the desired behavior here?

duos.org should pass, and www.duos.org should not?

Copy link
Contributor

@JVThomas JVThomas Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, www. shouldn't be a requirement at all. You can try this out with the DUOS example I listed. Typing in www.duos.org will give you a 'Site can't be reached' but duos.org will get you to our production site.

www is not a hard requirement for websites. It's up to the domain administrator to determine whether or not they want their website accessible by pre-prending www or not.

The very first website (which still works) also operates under the same premise. Pre-prending it with www. will not work
http://info.cern.ch/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://duos.org should work; I am currently using javascript's inbuilt new URL() functionality to test this. I believe the only catch is that you have to specify http:// or https:// for it to parse properly. If we want duos.org to pass I can change the functionality to assume https:// schema always.

Copy link
Contributor

@JVThomas JVThomas Sep 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a matter of functionality, its the issue of User Experience.

The average user does not type in https://www.google.com, they type in google.com. From there the browser executes the request under https for you, and DNS will redirect you to www.google.com.

Now imagine I'm a researcher for DUOS. I type in duos.org because that's what I always do when I visit that site.

Screen Shot 2022-09-23 at 8 18 57 AM

Validator is now telling me that the url is invalid and provides me https://www.google.com as a valid url. Me, the user, will then input https://www.duos.org to match the example given. I've seen http and www before, so I would assume that its a matter of providing the full URL rather than the shorthand that I use in my day to day.

Screen Shot 2022-09-23 at 8 20 45 AM

Great, the validator isn't complaining anymore. The problem is the URL is wrong and any user who views that url on the UI will copy and paste that link on their browser only to find themselves at a page that does not exist.

The application essentially prompted the user to input an invalid URL in the field, meaning we're potentially guiding users to input incorrect information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, would be assuming https:// always and showing duos.org as an example url be enough to fix the user experience?

Copy link
Contributor Author

@connorlbark connorlbark Sep 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

URL validation now is simpler (from user perspective) and allows duos.org. Provides duos.org as an example.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep this works. This way we're allowing the user to input the url without influencing them in anyway. If the value ends up being wrong we can either point people to SOs or contact them (SOs) directly and tell them to fix it.

Copy link
Contributor

@JVThomas JVThomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes! The code is good to go

@connorlbark connorlbark merged commit 46c64ee into develop Sep 28, 2022
@connorlbark connorlbark deleted the DUOS-1964-new branch September 28, 2022 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants