Conversation
|
supersedes: #4399 |
|
So great, @everreau! I've suggested some changes in line with ORCID's brand guidelines This is only for journals at the moment, is that correct? Could the co-author use the ORCID login option to connect their iD to their account rather than connecting the iD in their profile? What happens if the co-author is a frozen author without an account, or if the account isn't active? |
|
@alainna. All of the login features also work for preprints. I don't see any reference at all to orcids in the preprint author interface. I think we (cdl) would send whatever orcid was in the author's account to crossref even though it's not visible and it appears in the preprint front end. The profile and the account are the same as far as I know. The interface on main is a bit different than what we're currently running. Maybe we should setup a time so you can take a look at what I have. I hadn't really thought about what would happen if a user connects their orcid after the frozen authors were already made. I'll take a look at that. |
On the |
|
@joemull can you do the first technical review on this and then pass it to me please? |
|
Just an update: I should be able to get to this |
|
Some suggestions for this dev: eScholarship#46 |
joemull
left a comment
There was a problem hiding this comment.
This is hugely appreciated--thank you for putting it in.
I'm having trouble testing it out (an issue with OLH's ORCID sandbox that will take a few days to resolve), but I've gone ahead and given you some comments from reading the code. Hope they're clear but please do ping me if anything does not make sense.
|
@joemull I think this may be ready for review. I think the login logic in core.views is getting quite confusing. I was trying to rewrite so we can favor accounts that have authenticated orcids and prevent duplicate authenticated orcids. Maybe it would be easier to just make the orcid field unique at this point? I'm not sure if there is anything else in the queue that is needed before that. |
|
OK @everreau I will try to get to it first thing next week (I'm out the rest of this week). Yes every time I try to read through that logic I get confused. I agree it is high time to make the ORCID unique. I think the hard thing there is that there are duplicates currently. So the data migration would have to enforce the unique constraint while also recording the duplicates somehow and making them visible to someone (press manager, editor)? to resolve. |
|
@joemull so, my thought on dealing with the duplicates would be to create a migration that looks for the best account to retain the orcid by a set of criteria (eg is active, matches info held in orcid, last login), remove it from the others and create a log entry that says it was removed. Then we could add some logic that would allow a user to reclaim the orcid to their own account by authenticating with orcid and we could add a view that lets managers see everything that has the given log entry and resolve manually? |
joemull
left a comment
There was a problem hiding this comment.
This is getting really close to being ready, and it's great that you were able to loop in the frozen author piece. I took a careful read through the logic and tested it out, and it's mostly working great for me. I just have a few requests around tests, the admin view, and maintainability, and one suggestion for usability--in the verification request, taking the user straight to the verification step rather than to their profile.
| self.assertIn( | ||
| f"/register/step/orcid/", | ||
| response.redirect_chain[0][0], | ||
| ) |
There was a problem hiding this comment.
If I'm reading this right, the redirect to registration would happen here even if the account was active, because neither the ORCID nor email matches. Should we add some of those in here so it's clear the test is just testing active / inactive status?
| "user": user, | ||
| "user_profile_url": request.site_type.site_url( | ||
| reverse("core_edit_profile"), | ||
| ), |
There was a problem hiding this comment.
Have you considered making the link straight to the verification step? I think this might be clearer for users who do not remember what the profile page looks like. If they are taken to the profile page, they have to scroll down to the social media and accounts section and notice the button to connect their ORCID, but if they are taken straight to the redirect cycle, they'll be prompted at each step what to do.
We have a helper function in this file that might be useful:
reverse_with_query("core_edit_profile", { "action": "add_profile_orcid" })There was a problem hiding this comment.
Thanks for making this change. I'd just request two small further changes for readability:
-
Can we get away with just one level of reversing here, and rely on logic in the view to kick the user to the login screen?
In other words, here we'd just send them to "core_login_orcid" with action="add_profile_orcid", and then, in that view, if they're not logged in, we'd get the current path, save it in next_url (see other comment), and kick them out to the login screen, expecting that Janeway will send them back to "core_login_orcid". OK with you?
-
Let's change the name of this template variable from "user_profile_url" to "orcid_verification_link" or something like that so it's clear in the template what it is.
There was a problem hiding this comment.
I do think the flow of logging in to janeway first and then orcid is a bit better for users but it likely won't affect many users since most will already be logged in to janeway so I'm fine with it.
There was a problem hiding this comment.
@everreau OH! Now I understand the intent of having two levels of reversed URLs. If you want to change it back and add a comment explaining why, then I'd be happy with that. Let me know and then I'll move this along.
| return username.lower() | ||
|
|
||
| def get_orcid_url(self): | ||
| return f"{settings.ORCID_URL.replace('oauth/authorize', '')}{self.orcid}" |
There was a problem hiding this comment.
I wonder what are the benefits and drawbacks of this versus the logic in orcid_uri for the frozen author (and now the preprint author)? I'm thinking of a few things:
- The ORCID IDs in Janeway databases have sometimes been input as URLs (obviously something to fix when we make the field unique), so
get_orcid_urlwould potentially result in repeating the host name and domain parts. - It might be useful in some contexts to surface the
sandbox.orcid.orgdomain, but this is not accounted for byorcid_uribecause it hard-codes the domain.
Maybe it's time to combine the logic and have one method for Account, FrozenAuthor, and PreprintAuthor?
There was a problem hiding this comment.
Good idea. I added a utility method that combines both sets of logic.
That does sound like a good approach to me. I think we'd open another can of worms by putting it in this PR, but I added your comment on the thread for #4751, where a few people commented on how to go about de-duping. |
|
@joemull I think I've addressed all your comments. Let me know if I've missed something. |
joemull
left a comment
There was a problem hiding this comment.
Looking really good. Just missing one bit of logic around the redirection after non-ORCID login, and I had two nitpicks about readability.
| result = COMPILED_ORCID_REGEX.search(orcid) | ||
| if result: | ||
| return f"{url.scheme}://{url.netloc}/{result.group(0)}" | ||
| return "" |
| "user": user, | ||
| "user_profile_url": request.site_type.site_url( | ||
| reverse("core_edit_profile"), | ||
| ), |
There was a problem hiding this comment.
Thanks for making this change. I'd just request two small further changes for readability:
-
Can we get away with just one level of reversing here, and rely on logic in the view to kick the user to the login screen?
In other words, here we'd just send them to "core_login_orcid" with action="add_profile_orcid", and then, in that view, if they're not logged in, we'd get the current path, save it in next_url (see other comment), and kick them out to the login screen, expecting that Janeway will send them back to "core_login_orcid". OK with you?
-
Let's change the name of this template variable from "user_profile_url" to "orcid_verification_link" or something like that so it's clear in the template what it is.
|
changes complete. |
|
@everreau OK super. See my reply here on the double redirects: https://github.com/openlibhums/janeway/pull/5110/changes#r3050292149 |
Addresses: #2610
When orcid is enabled: