-
Notifications
You must be signed in to change notification settings - Fork 77
Author interface for ROR, CRediT, and ORCiD in submission workflow #4697
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
Conversation
@joemull can you check you've pushed everything? I think there are some missing imports on |
I don't get any import errors @ajrbyers--was it on server start or on loading a page? |
Check
|
Ah, right you are. I put the author search functionality in that file at the last minute. To guard against simple things like this, I should probably write quick view tests for each branch of the view's POST logic, eh? |
We did a demo of this in the design meeting last week. Here are notes from that.
|
My other todos when this comes back from review:
|
0cefb4f
to
5713e3d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some general thoughts:
-
When attempting to update affiliations from ORCID using (https://orcid.org/0000-0003-2126-266X) I was informed that there were no affiliations. My public profile shows I have them. Are we using something else to find the user on ORCID? The email address this user has does not match one I have on ORCID.
-
When posting a CREDIT for an author can we return the user to that record so they don't need to scroll to find them?
-
Should we include a link for the submitting author to share with their co-author in this text "If this author's name, email, ORCiD, or affiliations need to be updated, please invite them to edit their profile on the Open Library of Humanities website."?
-
Lastly, could the CREDIT record items themselves be remove buttons and be on a single line? This would reduce the height of each record which was an issue we discussed.
[ Conceptualization X ] [ Formal Analysis X ] [ Investigation X ]
Some code comments in line.
src/core/forms/forms.py
Outdated
def __init__( | ||
self, | ||
orcid_affiliation, | ||
tzinfo=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than set to None
, why not set it to timezone.get_current_timezone()
here?
src/core/models.py
Outdated
@@ -710,6 +719,7 @@ def remove_credit(self, credit_role_text, article): | |||
role=credit_role_text, | |||
) | |||
record.delete() | |||
return record |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this being returned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the view can use it in the message that's printed for the user.
src/core/tests/test_views.py
Outdated
) | ||
|
||
@patch('utils.orcid.get_orcid_record') | ||
def test_affiliation_bulk_update_from_orcid_confirmed(self, get_orcid_record): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test name here is a dupe of the one above.
else: | ||
return redirect(reverse('core_edit_profile')) | ||
|
||
orcid_details = orcid.get_orcid_record_details(cleaned_orcid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the ORCID API request fails? We should handle that gracefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just gets an empty dict if there is an HTTPError
in get_orcid_record
, so it will be fine.
src/submission/logic.py
Outdated
if emails: | ||
email = emails[0] | ||
else: | ||
email = generate_dummy_email(orcid_details) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this occurs there is no way to invite the author to update their record. Should we ask the submitting author for the person's email address in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a bit wonky. If they have a valid orcid, I was anticipating they'd log in using orcid. That's the idea behind this message:
janeway/src/templates/admin/submission/submit_authors.html
Lines 86 to 92 in 5713e3d
{% elif author.orcid %} | |
<p> | |
{% blocktrans %} | |
An account has been created using their ORCiD ID, | |
so they should select the ORCiD login option. | |
{% endblocktrans %} | |
</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not flagging the account as active, so they won't be able to login with orcid.
And the email address is fake, so they won't be able to activate it either. I reckon for these authors (no email address) we should NOT create an account, only a FrozenAuthor
record.
src/submission/tests/test_views.py
Outdated
def test_submit_authors_get(self): | ||
self.client.force_login(self.kathleen) | ||
response = self.client.get( | ||
f'/submit/{self.article.pk}/authors/', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why aren't we reversing the URL here? If the URL pattern changes the test will break. This is common across this module, sorry if I missed the reason for this but I found it odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first few view tests I wrote for Janeway were based on the Django docs, which do not reverse URLs but write them out.
https://docs.djangoproject.com/en/4.2/topics/testing/tools/#overview-and-a-quick-example
Later I noticed this was different from the way they've been written in Janeway before. At this point I've added 50+ tests with literal URLs. Shall I reverse them?
OK @ajrbyers this is ready for your review again. I tested it manually and made a few small fixes to the UI, including to the broken "Back to all authors" button. |
I mainly tested the interface here, and am really happy with it. Going to hand this over to @mauromsl who is going to take a look over the code again. |
a67bfa3
to
113253b
Compare
Signed-off-by: Mauro MSL <[email protected]>
Closes #4519.
Closes #3111.
Closes #4755.
Fixes #1485.
This follows the proposed design of #4519 as closely as possible. The ORCiD-based flow works really well, CRediT roles are easy to add, it's easy to change the correspondence author, and authors are easy to re-order.
However, there's a bit more work to do around editing co-author affiliations. We may need to move straight on to #145, which was out of scope for this PR but is a natural way to solve the problem of allowing maximum control over affiliation data, while not introducing security risks where bad actors could edit any user's affiliations.