Skip to content

Comments

ROR API client ID registration#237

Merged
kaysiz merged 18 commits intodevfrom
ks-org-id-reg
Apr 16, 2025
Merged

ROR API client ID registration#237
kaysiz merged 18 commits intodevfrom
ks-org-id-reg

Conversation

@kaysiz
Copy link
Member

@kaysiz kaysiz commented Mar 11, 2025

  • Add form for client id registration
Screenshot 2025-03-26 at 15 29 54 Screenshot 2025-03-26 at 15 29 41 Screenshot 2025-03-26 at 15 29 26 Screenshot 2025-03-26 at 15 29 13

@kaysiz kaysiz self-assigned this Mar 11, 2025
@lizkrznarich
Copy link
Contributor

lizkrznarich commented Mar 18, 2025

@kaysiz I did some testing, and it looks like some of the validation listed in the spec is not included https://docs.google.com/document/d/1odIr-LcjJ-hh7HP8RphReCvCL14iMEJLFM23X1vWv-0/edit?tab=t.0#heading=h.9wkd2a7lfe5s

Specific items to take a look at are:

  • For Email, Name and How do you plan to use the ROR API? fields, please add validation for character limit (255 for Email and Name; 500 for How do you plan to use...)
  • Sanitize input for all fields during validation to ensure no HTML or code can be submitted (possibly using something like https://www.npmjs.com/package/ember-sanitize). Note: I see we use sanitize-html in Bracco, which is fine, but I'd prefer the strictest sanitizing possible, since this is a public-facing form and bots like to submit all sorts of nasty garbage to public forms https://github.com/search?q=repo%3Adatacite%2Fbracco%20sanitize&type=code
  • Country field should be a dropdown/typeahead generated from a controlled list of countries

Additionally:

  • Please validate each field onblur (when user navigates away from a field) rather than only on submit, so that errors appear before submission. But also do validation again on submit, in case something was changed without firing onblur event.
  • I could not figure out how to type a manual entry into the Institution field - ex, I'm not sure what to do when I enter my institution name and it's not found in the list like in the screenshot below.
Screen Shot 2025-03-18 at 12 57 03 PM

@kaysiz kaysiz requested a review from lizkrznarich March 26, 2025 13:31
@kaysiz kaysiz marked this pull request as ready for review March 26, 2025 13:31
@lizkrznarich
Copy link
Contributor

@kaysiz Almost there! Validation is looking good.

A few tiny label tweaks:

  • Please remove the : char after "Email:"
  • Please add required/optional after each label, ex Email (required), Name (optional), Insitution (optional), etc. This wasn't in the spec, but I realize I should have included it.

I'm still having a heck of a time entering an institution name that is not in the ROR typeahead and have not been able to successfully add a manually-entered org name. Specifically:

  • When I type an org name that is not in ROR, ex "Org not in ROR" and press Enter, the top matching result from ROR is selected
  • When I click the X on the right side of the field to remove the ROR org, nothing happens
Screen.Recording.2025-03-28.at.9.36.14.AM.mov

@lizkrznarich
Copy link
Contributor

Looks good! Just waiting on the last bit about empty string vs null values for institution ROR and country code, as discussed on Slack.

@kaysiz kaysiz merged commit ab8e9c2 into dev Apr 16, 2025
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.

2 participants