ORCiD info is prefilled when registering #4279
Conversation
|
Can one of the admins verify this patch? |
|
Jenkins: Whitelist |
|
Jenkins: test this please |
|
related: #3337 |
|
@everreau @ajrbyers The rationale for allowing the user to remove the ORCID iD from the registration form (or their profile) is in case they authenticate the incorrect ORCID iD. This can occur when the user: a. accidentally registers a new ORCID iD; (My expectation would be that with the ORCID iD is removed, the access token is simultaneously revoked also. haven't checked whether that's part of the process.) |
|
@everreau the display of the ORCID iD should be HTTPS, not HTTP |
|
For clarification it allows the user to remove the ORCID but doesn’t remove it automatically? |
|
@alainna 1. hm.. if that's the use case for the remove orcid link I can probably just make it simpler. This remove the orcid removes the id but keeps the form auto-filled. (This seemed unnecessarily complicated to me but that was how I interpreted the spec). If it's just for the case that it's the wrong orcid we could just link back to the original form with no token. |
|
@everreau 2: excellent!! |
|
@alainna Leaving the form auto-filled is not simpler and it seems like a weird thing to do. "I want to use my orcid data to fill the form but I do NOT want to save that orcid". However, I already implemented this functionality so I can leave it as-is. I was just asking to make it simpler. If we don't simplify it then they can just click the "Register" link again to get a clean form. |
There was a problem hiding this comment.
Thanks @everreau--I think this is good but it is pulling on some pre-existing spaghetti we have around URL formation and carrying users through redirects that I think we need to address a bit more holistically. I happen to be working on another feature that touches the same code, so I am sharing some information related to that below. See comments inline.
As for the feature to remove the ORCID, I think we should take the additional context from Alainna and simplify it like you suggest. I think it would be counter-intuitive to offer just the deletion of the ORCID, without removing the rest of the form details, because the user will wonder what they are actually doing when they submit the form after that. Are they still connecting their orcid account? Maybe some kind of "Start over" or "Retry" button would be clearer?
| elif request.journal: | ||
| return redirect(reverse('core_dashboard')) | ||
| else: | ||
| return redirect(reverse('website_index')) |
There was a problem hiding this comment.
This bit has one too many indents.
| @@ -328,8 +342,6 @@ def register(request): | |||
| if form.is_valid(): | |||
| if token_obj: | |||
| new_user = form.save(commit=False) | |||
There was a problem hiding this comment.
This will need to be commit=True or no commit arg if the user is not being saved because line 332 is deleted.
| press= helpers.create_press() | ||
| repo = helpers.create_repository(press, [], []) | ||
| self.assertEqual(build_redirect_uri(repo), "http://localhost/login/orcid/?action=login") | ||
| self.assertEqual(build_redirect_uri(repo, action="register"), "http://localhost/login/orcid/?action=register") |
There was a problem hiding this comment.
This test is failing for me because it's missing some data around the request. But once that data is provided and the function runs, I think it will still not be a great idea to pass query parameters in the path argument of site_url and build_url functions. I understand why you are putting it in the path and not a query dict or something, because we don't provide a way to pass query parameters into that function and even if you could, the URL encoding is redundant so the URL would be garbled on its way back from ORCiD.
I discovered these things while working on a separate feature, #3899, whereby users will be carried through the login and registration process with a bit more care. It's still a work in progress but I'm thinking of expanding simplifying and unifying our token models so that we can retrieve next URLs and query parameters like this, even when we send things out to ORCiD.
If we were to fix up the tokens, we could make use of ORCiD's state parameter, which would let us link up the appropriate "next" url, whether login or registration (your feature) or something else (my feature).
I know this is a lot but I am curious what your thoughts are. If you want to glance over my still-messy WIP, it's here: #4322
| @@ -44,6 +44,7 @@ def retrieve_tokens(authorization_code, site): | |||
| try: | |||
| r.raise_for_status() | |||
| except HTTPError as e: | |||
| print(e) | |||
Closes #3337.