Skip to content

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

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/core/example_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

# ORCID Settings
ENABLE_ORCID = True
ORCID_API_URL = 'http://pub.orcid.org/v1.2_rc7/'
ORCID_API_URL = '' # Not needed any more. Requests are delegated to python-orcid.
ORCID_URL = 'https://orcid.org/oauth/authorize'
ORCID_TOKEN_URL = 'https://pub.orcid.org/oauth/token'
ORCID_CLIENT_SECRET = ''
Expand Down
1 change: 1 addition & 0 deletions src/core/forms/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
JournalSubmissionForm,
LoginForm,
NotificationForm,
OrcidAffiliationForm,
OrganizationNameForm,
PasswordResetForm,
PressJournalAttrForm,
Expand Down
89 changes: 81 additions & 8 deletions src/core/forms/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@

import uuid
import json
import os

from django import forms
from django.db.models import Q
from django.utils.datastructures import MultiValueDict
from django.forms.fields import Field
from django.utils import timezone
from django.utils.translation import gettext_lazy as _
Expand Down Expand Up @@ -975,14 +977,15 @@ def __init__(self, *args, **kwargs):

def clean(self):
cleaned_data = super().clean()
query = Q(account=self.account, organization=self.organization)
for key, value in cleaned_data.items():
query &= Q((key, value))
if self._meta.model.objects.filter(query).exists():
self.add_error(
None,
"An affiliation with matching details already exists."
)
if not self.instance:
query = Q(account=self.account, organization=self.organization)
for key, value in cleaned_data.items():
query &= Q((key, value))
if self._meta.model.objects.filter(query).exists():
self.add_error(
None,
"An affiliation with matching details already exists."
)
return cleaned_data

def save(self, commit=True):
Expand All @@ -994,6 +997,76 @@ def save(self, commit=True):
return affiliation


class OrcidAffiliationForm(forms.ModelForm):
"""
A form for creating ControlledAffiliation objects
from ORCID data.
"""

class Meta:
model = models.ControlledAffiliation
fields = '__all__'

def __init__(
self,
orcid_affiliation,
tzinfo=timezone.get_current_timezone(),
data=None,
*args,
**kwargs,
):
if not data:
data = MultiValueDict()

# The `get` methods below are used together with `or`
# defensively because of the data population in the API.
# It can have keys pointing to None values like:
# {"year": {"value": 2019}, "month": None}

data['title'] = orcid_affiliation.get('role-title', '') or ''
data['department'] = orcid_affiliation.get('department-name', '') or ''

org = None
orcid_org = orcid_affiliation.get('organization', {}) or {}
disamb_org = orcid_org.get('disambiguated-organization', {}) or {}
disamb_id = disamb_org.get('disambiguated-organization-identifier', '') or ''
if disamb_id.startswith('https://ror.org/'):
ror_id = os.path.split(disamb_id)[-1]
try:
org = models.Organization.objects.get(ror_id=ror_id)
except models.Organization.DoesNotExist:
pass
if not org:
address = orcid_org.get('address', {}) or {}
org, _created = models.Organization.get_or_create_without_ror(
institution=orcid_org.get('name', '') or '',
country=address.get('country', '') or '',
account=data.get('account'),
frozen_author=data.get('frozen_author'),
preprint_author=data.get('preprint_author'),
)
data['organization'] = org

orcid_start = orcid_affiliation.get('start-date', {}) or {}
if orcid_start:
data['start'] = timezone.datetime(
int((orcid_start.get('year', {}) or {}).get('value', 1)),
int((orcid_start.get('month', {}) or {}).get('value', 1)),
int((orcid_start.get('day', {}) or {}).get('value', 1)),
tzinfo=tzinfo,
)
orcid_end = orcid_affiliation.get('end-date', {}) or {}
if orcid_end:
data['end'] = timezone.datetime(
int((orcid_end.get('year', {}) or {}).get('value', 1)),
int((orcid_end.get('month', {}) or {}).get('value', 1)),
int((orcid_end.get('day', {}) or {}).get('value', 1)),
Comment on lines +1061 to +1063
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to handle the case of explicit None like for the other cases? (e.g {'year': {'value': None}})

tzinfo=tzinfo,
)

super().__init__(data=data, *args, **kwargs)


class ConfirmDeleteForm(forms.Form):
"""
A generic form for use on confirm-delete pages
Expand Down
5 changes: 5 additions & 0 deletions src/core/include_urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,11 @@
core_views.affiliation_update,
name='core_affiliation_update'
),
re_path(
r'^profile/affiliation/update-from-orcid/$',
core_views.affiliation_bulk_update_from_orcid,
name='core_affiliation_bulk_update_from_orcid'
),
re_path(
r'^profile/affiliation/(?P<affiliation_id>\d+)/delete/$',
core_views.affiliation_delete,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class Migration(migrations.Migration):
('preprint_author', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='repository.preprintauthor')),
],
options={
'ordering': ['is_primary', '-pk'],
'ordering': ['-is_primary', '-pk'],
},
),
migrations.AddConstraint(
Expand Down
13 changes: 13 additions & 0 deletions src/core/model_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
__license__ = "AGPL v3"
__maintainer__ = "Birkbeck Centre for Technology and Publishing"
from contextlib import contextmanager
from hashlib import md5
from io import BytesIO
import re
import sys
Expand Down Expand Up @@ -912,3 +913,15 @@ def create(self, **kwargs):
def filter(self, *args, **kwargs):
kwargs = self._remap_old_affiliation_lookups(kwargs)
return super().filter(*args, **kwargs)


def generate_dummy_email(details):
"""
:param details: a dict whose keys and values will serve as the hash seed
:type details: dict
"""
seed = ''.join([str(key) + str(val) for key, val in details.items()])
hashed = md5(str(seed).encode("utf-8")).hexdigest()
# Avoid validation bug where two @@ symbols are used in the email
domain = settings.DUMMY_EMAIL_DOMAIN.replace('@', '')
return "{0}@{1}".format(hashed, domain)
40 changes: 33 additions & 7 deletions src/core/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
from repository import models as repository_models
from utils.models import RORImportError
from submission import models as submission_models
from utils.forms import clean_orcid_id
from submission.models import CreditRecord
from utils.logger import get_logger
from utils import logic as utils_logic
Expand Down Expand Up @@ -390,6 +391,13 @@ def password_policy_check(self, request, password):
def string_id(self):
return str(self.id)

@property
def real_email(self):
if not self.email.endswith(settings.DUMMY_EMAIL_DOMAIN):
return self.email
else:
return ''

def get_full_name(self):
"""Deprecated in 1.5.2"""
return self.full_name()
Expand Down Expand Up @@ -692,9 +700,10 @@ def add_credit(self, credit_role_text, article):
"""
Adds a CRediT role to the article for this user
"""
record, _ = (
submission_models.CreditRecord.objects.get_or_create(
article=article, author=self, role=credit_role_text)
record, _created = submission_models.CreditRecord.objects.get_or_create(
article=article,
author=self,
role=credit_role_text,
)

return record
Expand All @@ -710,6 +719,7 @@ def remove_credit(self, credit_role_text, article):
role=credit_role_text,
)
record.delete()
return record
Copy link
Member

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?

Copy link
Member Author

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.

except submission_models.CreditRecord.DoesNotExist:
pass

Expand Down Expand Up @@ -2654,13 +2664,21 @@ def get_or_create_without_ror(
# entered without a ROR for this
# account / frozen author / preprint author?
try:
organization = cls.objects.get(
# If there is no `institution`, this method is being used to update
# the department or country in isolation, so we want the primary
# affiliation's org regardless of what its custom label is.
query = models.Q(
controlledaffiliation__is_primary=True,
controlledaffiliation__account=account,
controlledaffiliation__frozen_author=frozen_author,
controlledaffiliation__preprint_author=preprint_author,
ror_id__exact='',
)
# If there is an institution name, we should only match organizations
# with that as a custom label.
if institution:
query &= models.Q(custom_label__value=institution)
organization = cls.objects.get(query)
except (cls.DoesNotExist, cls.MultipleObjectsReturned):
# Otherwise, create a new, disconnected record.
organization = cls.objects.create()
Expand Down Expand Up @@ -2757,7 +2775,7 @@ class Meta:
['account', 'frozen_author', 'preprint_author'],
)
]
ordering = ['is_primary', '-pk']
ordering = ['-is_primary', '-pk']

def title_department(self):
elements = [
Expand Down Expand Up @@ -2836,10 +2854,12 @@ def get_or_create_without_ror(
cls,
institution='',
department='',
title='',
country='',
account=None,
frozen_author=None,
preprint_author=None,
defaults=None,
):
"""
Backwards-compatible API for setting affiliation from unstructured text.
Expand All @@ -2856,6 +2876,8 @@ def get_or_create_without_ror(
:type account: core.models.Account
:type frozen_author: submission.models.FrozenAuthor
:type preprint_author: repository.models.PreprintAuthor
:param defaults: default dict passed to ControlledAffiliation.get_or_create:
:type defaults: dict:
"""
organization, _created = Organization.get_or_create_without_ror(
institution=institution,
Expand All @@ -2864,12 +2886,16 @@ def get_or_create_without_ror(
frozen_author=frozen_author,
preprint_author=preprint_author,
)
if not defaults:
defaults = {}

defaults = {
defaults.update({
'organization': organization,
}
})
if department:
defaults['department'] = department
if title:
defaults['title'] = title
kwargs = {
'is_primary': True,
'account': account,
Expand Down
4 changes: 4 additions & 0 deletions src/core/templatetags/text.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,7 @@ def startswith(text, starts):
if isinstance(text, str):
return text.startswith(starts)
return False

@register.filter
def concat(original, added):
return str(original) + str(added)
46 changes: 46 additions & 0 deletions src/core/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,25 @@ def setUpTestData(cls):
account=cls.user,
organization=cls.organization_bbk,
)
cls.country_us = core_models.Country.objects.create(
code='US',
name='United States',
)
cls.location_oakland = core_models.Location.objects.create(
name='Oakland',
geonames_id=5378538,
country=cls.country_us,
)
cls.organization_cdl = core_models.Organization.objects.create(
ror_id='03yrm5c26',
)
cls.organization_cdl.locations.add(cls.location_oakland)
cls.name_cdl = core_models.OrganizationName.objects.create(
value='California Digital Library',
language='en',
ror_display_for=cls.organization_cdl,
label_for=cls.organization_cdl,
)

# The raw unicode string of a 'next' URL
cls.next_url_raw = '/target/page/?a=b&x=y'
Expand Down Expand Up @@ -829,3 +848,30 @@ def test_affiliation_delete_post(self):
response = self.client.post(url, post_data, follow=True)
with self.assertRaises(core_models.ControlledAffiliation.DoesNotExist):
core_models.ControlledAffiliation.objects.get(pk=affil_id)

@patch('utils.orcid.get_orcid_record')
def test_affiliation_bulk_update_from_orcid(self, get_orcid_record):
get_orcid_record.return_value = helpers.get_orcid_record_all_fields()
self.client.force_login(self.user)
get_data = {}
affil_id = self.affiliation.pk
url = f'/profile/affiliation/update-from-orcid/'
response = self.client.get(url, get_data)
self.assertEqual(
response.context['new_affils'][0].__str__(),
'California Digital Library, Oakland, United States'
)

@patch('utils.orcid.get_orcid_record')
def test_affiliation_bulk_update_from_orcid_confirmed(self, get_orcid_record):
Copy link
Member

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.

get_orcid_record.return_value = helpers.get_orcid_record_all_fields()
self.client.force_login(self.user)
post_data = {}
affil_id = self.affiliation.pk
url = f'/profile/affiliation/update-from-orcid/'
self.client.post(url, post_data, follow=True)
self.user.refresh_from_db()
self.assertEqual(
self.user.primary_affiliation().__str__(),
'California Digital Library, Oakland, United States'
)
Loading