-
Notifications
You must be signed in to change notification settings - Fork 86
2610 orcid tokens #5110
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
base: master
Are you sure you want to change the base?
2610 orcid tokens #5110
Changes from all commits
efafcf3
3d051e7
d2b237d
c1d4244
1250329
7b90eb1
aba83d4
90c8b1d
0fc0f01
be0af16
c68057d
641995c
1a6767d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| # Generated by Django 4.2.26 on 2026-01-21 20:34 | ||
|
|
||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| ("core", "0109_salutation_name_20250707_1420"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AddField( | ||
| model_name="account", | ||
| name="orcid_token", | ||
| field=models.CharField(blank=True, max_length=40, null=True), | ||
| ), | ||
| migrations.AddField( | ||
| model_name="account", | ||
| name="orcid_token_expiration", | ||
| field=models.DateTimeField(blank=True, null=True), | ||
| ), | ||
| migrations.AddField( | ||
| model_name="orcidtoken", | ||
| name="access_token", | ||
| field=models.CharField(blank=True, max_length=40, null=True), | ||
| ), | ||
| migrations.AddField( | ||
| model_name="orcidtoken", | ||
| name="access_token_expiration", | ||
| field=models.DateTimeField(blank=True, null=True), | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| # Generated by Django 4.2.26 on 2026-02-13 18:12 | ||
|
|
||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ('core', '0110_account_orcid_token_account_orcid_token_expiration_and_more'), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AddField( | ||
| model_name='account', | ||
| name='date_orcid_requested', | ||
| field=models.DateTimeField(blank=True, null=True), | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,6 +71,7 @@ | |
| from utils import logic as utils_logic | ||
| from utils.forms import plain_text_validator | ||
| from production import logic as production_logic | ||
| from utils.orcid import is_token_valid | ||
|
|
||
| fs = JanewayFileSystemStorage() | ||
| logger = get_logger(__name__) | ||
|
|
@@ -485,6 +486,9 @@ class Account(AbstractBaseUser, PermissionsMixin): | |
| orcid = models.CharField( | ||
| max_length=40, null=True, blank=True, verbose_name=_("ORCiD") | ||
| ) | ||
| orcid_token = models.CharField(max_length=40, null=True, blank=True) | ||
| orcid_token_expiration = models.DateTimeField(null=True, blank=True) | ||
| date_orcid_requested = models.DateTimeField(blank=True, null=True) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not immediately clear to me why we'd need to add fields to the account rather than using the orcidtoken object, maybe in combination with some property methods that make it easy to get the needed info from the account object in templates--what's the thinking here? |
||
| twitter = models.CharField( | ||
| max_length=300, null=True, blank=True, verbose_name=_("Twitter Handle") | ||
| ) | ||
|
|
@@ -948,6 +952,12 @@ def hypothesis_username(self): | |
| )[:30] | ||
| return username.lower() | ||
|
|
||
| def get_orcid_url(self): | ||
| return f"{settings.ORCID_URL.replace('oauth/authorize', '')}{self.orcid}" | ||
|
|
||
| def is_orcid_token_valid(self): | ||
| return is_token_valid(self.orcid, self.orcid_token) | ||
|
|
||
|
|
||
| def generate_expiry_date(): | ||
| return timezone.now() + timedelta(days=1) | ||
|
|
@@ -959,6 +969,8 @@ class OrcidToken(models.Model): | |
| expiry = models.DateTimeField( | ||
| default=generate_expiry_date, verbose_name=_("Expires on") | ||
| ) | ||
| access_token = models.CharField(max_length=40, null=True, blank=True) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as the other one--charfields should not have |
||
| access_token_expiration = models.DateTimeField(null=True, blank=True) | ||
|
|
||
| def __str__(self): | ||
| return "ORCiD Token [{0}] - {1}".format(self.orcid, self.token) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |
| from django.urls.base import clear_script_prefix | ||
| from django.utils import timezone | ||
| from django.core import mail | ||
| from journal.tests.utils import make_test_journal | ||
|
|
||
| from utils.testing import helpers | ||
| from utils import setting_handler, install | ||
|
|
@@ -219,7 +220,7 @@ def test_register_with_orcid_token(self, record_mock): | |
| self.assertContains(response, "Campbell") | ||
| self.assertContains(response, "Kasey") | ||
| self.assertContains(response, "campbell@evu.edu") | ||
| self.assertNotContains(response, "Register with ORCiD") | ||
| self.assertNotContains(response, "Register with ORCID") | ||
| self.assertContains(response, "http://sandbox.orcid.org/0000-0000-0000-0000") | ||
| self.assertContains( | ||
| response, | ||
|
|
@@ -252,13 +253,13 @@ def test_register_with_orcid_token(self, record_mock): | |
| def test_registration(self): | ||
| response = self.client.get(reverse("core_register")) | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertContains(response, "Register with ORCiD") | ||
| self.assertContains(response, "Register with ORCID") | ||
|
|
||
| @override_settings(ENABLE_ORCID=False) | ||
| def test_registration(self): | ||
| response = self.client.get(reverse("core_register")) | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertNotContains(response, "Register with ORCiD") | ||
| self.assertNotContains(response, "Register with ORCID") | ||
|
|
||
| @override_settings(URL_CONFIG="domain", CAPTCHA_TYPE=None) | ||
| def test_mixed_case_login_different_case(self): | ||
|
|
@@ -582,3 +583,80 @@ def setUp(self): | |
| ) | ||
|
|
||
| clear_script_prefix() | ||
|
|
||
| @override_settings(ENABLE_ORCID=False) | ||
| def test_profile_orcid_disabled(self): | ||
| self.client.force_login(self.admin_user) | ||
| response = self.client.get(reverse("core_edit_profile")) | ||
| self.assertContains( | ||
| response, '<input type="text" name="orcid" maxlength="40" id="id_orcid">' | ||
| ) | ||
|
|
||
| def test_profile_orcid_enabled_no_orcid(self): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add an |
||
| # Profile should offer to connect orcid | ||
| self.client.force_login(self.admin_user) | ||
| response = self.client.get(reverse("core_edit_profile")) | ||
| self.assertNotContains(response, "ORCID could not be validated.") | ||
| self.assertContains(response, "Connect your ORCID") | ||
|
|
||
| @override_settings(ORCID_URL="https://sandbox.orcid.org/oauth/authorize") | ||
| def test_profile_orcid_unverified(self): | ||
| self.admin_user.orcid = "0000-0000-0000-0000" | ||
| self.admin_user.save() | ||
| self.client.force_login(self.admin_user) | ||
| response = self.client.get(reverse("core_edit_profile")) | ||
| self.assertContains(response, "ORCID iD could not be validated.") | ||
| self.assertContains(response, "Connect your ORCID") | ||
| self.assertContains(response, "https://sandbox.orcid.org/0000-0000-0000-0000") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this test (or any of the ones below) rely on a network connection? We'll need to put mocks into any test that relies on a network connection. |
||
|
|
||
| @patch.object(models.Account, "is_orcid_token_valid") | ||
| @override_settings(ORCID_URL="https://sandbox.orcid.org/oauth/authorize") | ||
| def test_profile_orcid(self, mock_method): | ||
| # override is_orcid_token valid make if valid | ||
| mock_method.return_value = True | ||
| self.admin_user.orcid = "0000-0000-0000-0000" | ||
| self.admin_user.orcid_token = "0a0aaaaa-0aa0-0000-aa00-a00aa0a00000" | ||
| self.admin_user.save() | ||
| self.client.force_login(self.admin_user) | ||
| response = self.client.get(reverse("core_edit_profile")) | ||
| self.assertContains(response, "https://sandbox.orcid.org/0000-0000-0000-0000") | ||
| self.assertContains(response, "remove_orcid") | ||
| self.assertContains( | ||
| response, '<input type="hidden" name="orcid" value="0000-0000-0000-0000"/>' | ||
| ) | ||
| self.assertNotContains(response, "ORCID could not be validated.") | ||
|
|
||
| @patch.object(models.Account, "is_orcid_token_valid") | ||
| @override_settings( | ||
| URL_CONFIG="domain", ORCID_URL="https://sandbox.orcid.org/oauth/authorize" | ||
| ) | ||
| def test_profile_orcid_not_admin(self, mock_method): | ||
| mock_method.return_value = True | ||
|
|
||
| journal_kwargs = { | ||
| "code": "fetests", | ||
| "domain": "fetests.janeway.systems", | ||
| } | ||
| journal = make_test_journal(**journal_kwargs) | ||
|
|
||
| journal_manager = helpers.create_user( | ||
| "jmanager@mailinator.com", ["journal-manager"], journal=journal | ||
| ) | ||
| journal_manager.is_active = True | ||
| journal_manager.save() | ||
|
|
||
| self.regular_user.orcid = "0000-0000-0000-0000" | ||
| self.regular_user.orcid_token = "0a0aaaaa-0aa0-0000-aa00-a00aa0a00000" | ||
| self.regular_user.save() | ||
|
|
||
| self.client.force_login(journal_manager) | ||
|
|
||
| url = reverse("core_user_edit", kwargs={"user_id": self.regular_user.pk}) | ||
| response = self.client.get(url, SERVER_NAME=journal.domain) | ||
|
|
||
| self.assertContains(response, "https://sandbox.orcid.org/0000-0000-0000-0000") | ||
| self.assertContains( | ||
| response, '<input type="hidden" name="orcid" value="0000-0000-0000-0000"/>' | ||
| ) | ||
| self.assertNotContains(response, "ORCID could not be validated.") | ||
| self.assertNotContains(response, "remove_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.
Charfields should not have
null=True