From 942c00eb1a84b916a83e97f77db2820b0119ee5a Mon Sep 17 00:00:00 2001 From: Joseph Muller Date: Mon, 9 Dec 2024 18:20:10 +0000 Subject: [PATCH] Work through regressions #3168 --- src/core/admin.py | 10 ++--- src/core/models.py | 71 ++++++++++++++++++++++-------------- src/core/tests/test_app.py | 1 - src/core/views.py | 9 ++--- src/repository/views.py | 2 +- src/review/logic.py | 14 +++---- src/submission/tests.py | 41 ++++++++++++++------- src/utils/testing/helpers.py | 6 ++- 8 files changed, 90 insertions(+), 64 deletions(-) diff --git a/src/core/admin.py b/src/core/admin.py index a70079d543..10405c220d 100755 --- a/src/core/admin.py +++ b/src/core/admin.py @@ -451,11 +451,11 @@ class AffiliationAdmin(admin.ModelAdmin): 'pk', 'title', 'department', - 'organization__ror_display_for', - 'organization__custom_label_for', - 'organization__label_for', - 'organization__alias_for', - 'organization__acronym_for' + 'organization__ror_display__value', + 'organization__custom_label__value', + 'organization__labels__value', + 'organization__aliases__value', + 'organization__acronyms__value', 'account__first_name', 'account__last_name', 'account__email', diff --git a/src/core/models.py b/src/core/models.py index d955e26bcb..da0d10d702 100644 --- a/src/core/models.py +++ b/src/core/models.py @@ -566,8 +566,6 @@ def snapshot_self(self, article, force_update=True): 'middle_name': self.middle_name, 'last_name': self.last_name, 'name_suffix': self.suffix, - 'institution': self.institution, - 'department': self.department, 'display_email': True if self == article.correspondence_author else False, } @@ -576,6 +574,8 @@ def snapshot_self(self, article, force_update=True): if frozen_author and force_update: for k, v in frozen_dict.items(): setattr(frozen_author, k, v) + frozen_author.institution = self.institution + frozen_author.department = self.department frozen_author.save() else: @@ -589,11 +589,15 @@ def snapshot_self(self, article, force_update=True): defaults={'order': order_integer} ) - submission_models.FrozenAuthor.objects.get_or_create( + frozen_author, created = submission_models.FrozenAuthor.objects.get_or_create( author=self, article=article, defaults=dict(order=order_object.order, **frozen_dict) ) + if created: + frozen_author.institution = self.institution + frozen_author.department = self.department + frozen_author.save() def frozen_author(self, article): try: @@ -2074,28 +2078,26 @@ def naive_get_or_create( value, account=None, frozen_author=None, + preprint_author=None, ): """ Backwards-compatible API for finding a matching organization name. - Intended for use in batch importers where ROR data is not available. + Intended for use in batch importers where ROR data is not available + in the data being imported. Does not support ROR ids, ROR name types, or location data. """ - no_exact_match = ( - cls.DoesNotExist or cls.MultipleObjectsReturned - ) - # Is there a single exact match in the # canonical name data from ROR (e.g. labels)? try: organization = cls.objects.get(labels__value=value) created = False - except no_exact_match: + except cls.DoesNotExist or cls.MultipleObjectsReturned: # Or maybe one in the past or alternate # name data from ROR (e.g. aliases)? try: organization = cls.objects.get(aliases__value=value) created = False - except no_exact_match: + except cls.DoesNotExist or cls.MultipleObjectsReturned: # Or maybe an organization has already been # entered without a ROR for this # account / frozen author / preprint author? @@ -2103,11 +2105,12 @@ def naive_get_or_create( organization = cls.objects.get( affiliation__account=account, affiliation__frozen_author=frozen_author, + affiliation__preprint_author=preprint_author, ror__exact='', ) created = False - except no_exact_match: - # Otherwise, create a naive, disconnected record + except cls.DoesNotExist or cls.MultipleObjectsReturned: + # Otherwise, create a naive, disconnected record. organization = cls.objects.create() created = True @@ -2350,17 +2353,24 @@ def naive_get_or_create( value, account=account, frozen_author=frozen_author, - ) - - # Create or update the actual affiliation - affiliation, created = Affiliation.objects.get_or_create( - account=account, - frozen_author=frozen_author, preprint_author=preprint_author, - organization=organization, ) - affiliation.is_primary = True - affiliation.save() + + # Create or update the actual affiliation if the associated + # account / frozen author / preprint author has been saved already + try: + affiliation, created = Affiliation.objects.get_or_create( + account=account, + frozen_author=frozen_author, + preprint_author=preprint_author, + organization=organization, + ) + affiliation.is_primary = True + affiliation.save() + except ValueError: + logger.warn('The affiliation could not be created.') + affiliation = None + created = False return affiliation, created @classmethod @@ -2375,13 +2385,18 @@ def naive_set_primary_department( Intended for use in batch importers where ROR data is not available. Does not support RORs or multiple affiliations. """ - affiliation, _created = Affiliation.objects.get_or_create( - account=account, - frozen_author=frozen_author, - is_primary=True, - ) - affiliation.department = value - affiliation.save() + # Create or update an affiliation if the associated + # account / frozen author / preprint author has been saved already + try: + affiliation, _created = Affiliation.objects.get_or_create( + account=account, + frozen_author=frozen_author, + is_primary=True, + ) + affiliation.department = value + affiliation.save() + except ValueError: + logger.warn('The department could not be set.') @classmethod def naive_set_primary_country( diff --git a/src/core/tests/test_app.py b/src/core/tests/test_app.py index 0e8fbc6ecf..31d2fca332 100755 --- a/src/core/tests/test_app.py +++ b/src/core/tests/test_app.py @@ -206,7 +206,6 @@ def test_orcid_registration(self, record_mock): self.assertEqual(response.status_code, 200) self.assertContains(response, "Campbell") self.assertContains(response, "Kasey") - self.assertContains(response, "Elk Valley University") self.assertContains(response, "campbell@evu.edu") self.assertNotContains(response, "Register with ORCiD") self.assertContains(response, "http://sandbox.orcid.org/0000-0000-0000-0000") diff --git a/src/core/views.py b/src/core/views.py index 882aa046bc..3bcb8f9c38 100755 --- a/src/core/views.py +++ b/src/core/views.py @@ -329,11 +329,6 @@ def register(request): initial["last_name"] = orcid_details.get("last_name", "") if orcid_details.get("emails"): initial["email"] = orcid_details["emails"][0] - if orcid_details.get("affiliation"): - initial['institution'] = orcid_details['affiliation'] - if orcid_details.get("country"): - if models.Country.objects.filter(code=orcid_details['country']).exists(): - initial["country"] = models.Country.objects.get(code=orcid_details['country']) form = forms.RegistrationForm( journal=request.journal, @@ -355,6 +350,10 @@ def register(request): if form.is_valid(): if token_obj: new_user = form.save() + if new_user.orcid: + orcid_details = orcid.get_orcid_record_details(token_obj.orcid) + if orcid_details.get("affiliation"): + new_user.institution = orcid_details['affiliation'] token_obj.delete() # If the email matches the user email on ORCID, log them in if new_user.email == initial.get("email"): diff --git a/src/repository/views.py b/src/repository/views.py index 9e9fbcefc5..2636ba48ee 100644 --- a/src/repository/views.py +++ b/src/repository/views.py @@ -377,7 +377,7 @@ def repository_search(request, search_term=None): Q(account__first_name__in=split_search_term) | Q(account__middle_name__in=split_search_term) | Q(account__last_name__in=split_search_term) | - Q(account__institution__icontains=search_term) + Q(account__affiliation__organization__labels__value__icontains=search_term) ) & ( diff --git a/src/review/logic.py b/src/review/logic.py index b9c1e32c56..0e14ddf8f9 100755 --- a/src/review/logic.py +++ b/src/review/logic.py @@ -824,11 +824,6 @@ def process_reviewer_csv(path, request, article, form): reader = csv.DictReader(csv_file) reviewers = [] for row in reader: - try: - country = core_models.Country.objects.get(code=row.get('country')) - except core_models.Country.DoesNotExist: - country = None - reviewer, created = core_models.Account.objects.get_or_create( email=row.get('email_address'), defaults={ @@ -836,12 +831,15 @@ def process_reviewer_csv(path, request, article, form): 'first_name': row.get('firstname', ''), 'middle_name': row.get('middlename', ''), 'last_name': row.get('lastname', ''), - 'department': row.get('department', ''), - 'institution': row.get('institution', ''), - 'country': country, 'is_active': True, } ) + if row.get('department', ''): + reviewer.department = row.get('department', '') + reviewer.save() + if row.get('institution', ''): + reviewer.institution = row.get('institution', '') + reviewer.save() try: review_interests = row.get('interests') diff --git a/src/submission/tests.py b/src/submission/tests.py index de565fe4ba..c51bb3f7bd 100644 --- a/src/submission/tests.py +++ b/src/submission/tests.py @@ -66,28 +66,32 @@ def create_journal(): return journal_one - @staticmethod - def create_authors(): + @classmethod + def create_authors(cls): author_1_data = { + 'email': 'one@example.org', 'is_active': True, 'password': 'this_is_a_password', 'salutation': 'Prof.', 'first_name': 'Martin', + 'middle_name': '', 'last_name': 'Eve', 'department': 'English & Humanities', 'institution': 'Birkbeck, University of London', } author_2_data = { + 'email': 'two@example.org', 'is_active': True, 'password': 'this_is_a_password', 'salutation': 'Sr.', 'first_name': 'Mauro', + 'middle_name': '', 'last_name': 'Sanchez', 'department': 'English & Humanities', 'institution': 'Birkbeck, University of London', } - author_1 = Account.objects.create(email="1@t.t", **author_1_data) - author_2 = Account.objects.create(email="2@t.t", **author_2_data) + author_1 = helpers.create_author(cls.journal_one, **author_1_data) + author_2 = helpers.create_author(cls.journal_one, **author_2_data) return author_1, author_2 @@ -105,12 +109,15 @@ def create_sections(self): journal=self.journal_one, ) + @classmethod + def setUpTestData(cls): + cls.journal_one = cls.create_journal() + def setUp(self): """ Setup the test environment. :return: None """ - self.journal_one = self.create_journal() self.editor = helpers.create_editor(self.journal_one) self.press = helpers.create_press() self.create_sections() @@ -273,7 +280,9 @@ def test_snapshot_author_metadata_override(self): article.snapshot_authors() new_department = "New department" - article.frozen_authors().update(department=new_department) + for frozen in article.frozen_authors(): + frozen.department = new_department + frozen.save() article.snapshot_authors(force_update=True) frozen = article.frozen_authors().all()[0] @@ -576,15 +585,12 @@ def test_author_form_harmful_inputs(self): "middle_name", "name_prefix", "suffix", - "institution", - "department", }): form = forms.AuthorForm( { 'first_name': 'Andy', 'last_name': 'Byers', 'biography': 'Andy', - 'institution': 'Birkbeck, University of London', 'email': f'andy{i}@janeway.systems', 'orcid': 'https://orcid.org/0000-0003-2126-266X', **{attr: harmful_string}, @@ -783,37 +789,44 @@ def create_journal(): return journal_one - @staticmethod - def create_authors(): + @classmethod + def create_authors(cls): author_1_data = { + 'email': 'one@example.org', 'is_active': True, 'password': 'this_is_a_password', 'salutation': 'Prof.', 'first_name': 'Martin', + 'middle_name': '', 'last_name': 'Eve', 'department': 'English & Humanities', 'institution': 'Birkbeck, University of London', } author_2_data = { + 'email': 'two@example.org', 'is_active': True, 'password': 'this_is_a_password', 'salutation': 'Sr.', 'first_name': 'Mauro', + 'middle_name': '', 'last_name': 'Sanchez', 'department': 'English & Humanities', 'institution': 'Birkbeck, University of London', } - author_1 = Account.objects.create(email="1@t.t", **author_1_data) - author_2 = Account.objects.create(email="2@t.t", **author_1_data) + author_1 = helpers.create_author(cls.journal_one, **author_1_data) + author_2 = helpers.create_author(cls.journal_one, **author_2_data) return author_1, author_2 + @classmethod + def setUpTestData(cls): + cls.journal_one = cls.create_journal() + def setUp(self): """ Setup the test environment. :return: None """ - self.journal_one = self.create_journal() self.editor = helpers.create_editor(self.journal_one) self.press = helpers.create_press() diff --git a/src/utils/testing/helpers.py b/src/utils/testing/helpers.py index 1f53959ef3..ae26f43f3a 100755 --- a/src/utils/testing/helpers.py +++ b/src/utils/testing/helpers.py @@ -62,6 +62,7 @@ def create_user(username, roles=None, journal=None, **attrs): journal=journal ) + user.save() for attr, value in attrs.items(): setattr(user, attr, value) @@ -179,8 +180,6 @@ def create_author(journal, **kwargs): "first_name": "Author", "middle_name": "A", "last_name": "User", - "institution": "Author institution", - "department": "Author Department", "biography": "Author test biography" } attrs.update(kwargs) @@ -190,6 +189,8 @@ def create_author(journal, **kwargs): journal=journal, **attrs, ) + author.institution = "Author institution" + author.department = "Author Department" author.is_active = True author.save() return author @@ -353,6 +354,7 @@ def create_preprint(repository, author, subject, title='This is a Test Preprint' account=author, order=1, ) + preprint_author.save() preprint_author.affiliation = 'Made Up University' preprint_author.save() return preprint