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. |
joemull
left a comment
joemull
left a comment
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
There was a problem hiding this comment.
hi @joemull -- sorry for my delayed response. I got waylaid by other tasks and some time off. I've checked in corrections to the more minor issues you've pointed out.
I like your idea for fixing the return url business. I'm also thinking we might be able to use this if fix something else Alainna requested. She doesn't want to have to add each new journal/repo url to the redirect urls but instead have a press or installation wide return url that is able to redirect appropriately.
There was a problem hiding this comment.
@joemull How do you want to proceed with this work? We'd like to get this and a few other orcid features that would depend on this rewrite into the next release.
There was a problem hiding this comment.
@everreau I think unifying the various Token models to a single one and passing the token ID in state to the ORCID API is the right direction. I could potentially separate that work from the rest of my branch and put it through review with higher priority. You could then rebase this branch on that and finish up this work.
I'd have to run it by @mauromsl and @ajrbyers to confirm they like the solution and figure out a time frame. It will be tight, because we are hoping to finish up 1.7 in the next few weeks.
Shall I do that and let you know?
Or if you have time to handle it ASAP, would you want to copy-paste and fix up my WIP code that unifies the Token objects and sends and receives state as a part of this PR?
There was a problem hiding this comment.
@joemull I can probably do it. I think this is my highest priority task at the moment. I'll check with Chad to be sure. Thank you!
There was a problem hiding this comment.
That would be super! Let me know if I can help with context.
There was a problem hiding this comment.
@everreau I just had a chat with Andy and Mauro, and can we actually ask you to hold off on this? We need a chance to discuss it properly.
There was a problem hiding this comment.
@joemull no problem. Chad has indeed deemed this my most important task so we'd like to get going as soon as we can. Thanks!
| try: | ||
| r.raise_for_status() | ||
| except HTTPError as e: | ||
| print(e) |
|
@joemull updated code to use state. If the test above is still failing for you please let me know what is missing. It's passing for me. |
joemull
left a comment
joemull
left a comment
There was a problem hiding this comment.
This looks good to me. I also tested it out manually to make sure I understood it.
As for the whole state thing, it's not quite what I had in mind but that's OK because your solution works. Just a heads up, I may refactor it a bit when I get to the next URL branch. OK If I add you as a reviewer on that PR when it is ready, to make sure I don't mess this up?
The test that was failing for me was only failing when I ran the orcid tests in isolation, and I also might have a fix for it in my refactor, so I am OK with merging this. I want to make sure we don't hold you up any more than we have already.
|
Oh, there are merge conflicts on this branch. Normally we ask the branch owner do a git rebase locally onto a later version and then force push. Is that alright with you? |
|
Thanks @everreau! As Joe mentioned, there are a few conflicts that need resolving, would you mind performing a rebase of |
everreau
force-pushed
the
3337-orcid-prefill
branch
from
d545100 to
38456d0
Compare
6 participants
Closes #3337.