Bump lxml from 4.5.2 to 4.6.2 in /requirements

.coveragerc

@@ -9,3 +9,5 @@ omit =
   */management/*
   */tests.py
   */experiments.py
+  */conftest.py
+  */tests/*

.gitlab/test-image.yml

@@ -9,5 +9,9 @@ test_image:
     - docker build --cache-from $CI_REGISTRY_IMAGE/celus-test:latest --tag $CI_REGISTRY_IMAGE/celus-test:latest --target celus-test -f ci/Dockerfile .
     - docker push $CI_REGISTRY_IMAGE/celus-test:latest
 
+  rules:
+    - if: '$CI_COMMIT_BRANCH == "master"'
+      when: on_success
+
   tags:
     - dind

apps/core/logic/sync.py

@@ -97,6 +97,7 @@ def sync_data(self, records: [dict]) -> dict:
             )
         }
         org_ext_id_to_db_obj = {org.ext_id: org for org in Organization.objects.all()}
+        seen_user_orgs = set()
         for (org_ext_id, user_ext_id), is_admin in self._org_user_status.items():
             uo = org_user_to_db_obj.get((org_ext_id, user_ext_id))
             if not uo:
@@ -116,6 +117,7 @@ def sync_data(self, records: [dict]) -> dict:
                     user=user, organization=organization, is_admin=is_admin, source=self.data_source
                 )
                 org_user_to_db_obj[(org_ext_id, user_ext_id)] = uo
+                seen_user_orgs.add(uo.pk)
                 stats['User-Org created'] += 1
             else:
                 if uo.is_admin != is_admin:
@@ -124,6 +126,13 @@ def sync_data(self, records: [dict]) -> dict:
                     stats['User-Org synced'] += 1
                 else:
                     stats['User-Org unchanged'] += 1
+                seen_user_orgs.add(uo.pk)
+        deleted, _details = (
+            UserOrganization.objects.filter(source=self.data_source)
+            .exclude(pk__in=seen_user_orgs)
+            .delete()
+        )
+        stats['User-Org deleted'] = deleted
         return stats
 
 

apps/core/tests/test_sync.py

@@ -6,6 +6,7 @@
 from core.logic.sync import sync_identities
 from core.models import Identity, DataSource
 from erms.sync import ERMSObjectSyncer
+from organizations.models import Organization, UserOrganization
 from ..logic.sync import sync_users
 
 
@@ -66,6 +67,33 @@ def test_sync_users_with_removal(self, data_source):
         assert stats['removed'][0] == 1
         assert User.objects.count() == 2
 
+    @pytest.mark.now
+    def test_sync_users_with_user_org_link_removal(self, data_source):
+        """
+        Test that after a user gets removed from an organization, the link is properly removed
+        """
+        User = get_user_model()
+        assert User.objects.count() == 0
+        Organization.objects.create(ext_id=10, name='Org 1', short_name='org_1')
+        Organization.objects.create(ext_id=100, name='Org 2', short_name='org_2')
+        input_data = [
+            {'id': 1, 'vals': {'name@cs': ['John Doe']}, 'refs': {'employee of': [10, 100]}},
+        ]
+        stats = sync_users(data_source, input_data)
+        assert stats['removed'][0] == 0
+        assert stats[ERMSObjectSyncer.Status.NEW] == 1
+        assert User.objects.count() == 1
+        assert UserOrganization.objects.count() == 2
+        # now remove one of the organizations
+        input_data[0]['refs']['employee of'].remove(10)
+        stats = sync_users(data_source, input_data)
+        assert stats[ERMSObjectSyncer.Status.NEW] == 0
+        assert stats[ERMSObjectSyncer.Status.UNCHANGED] == 1
+        assert stats['removed'][0] == 0
+        assert stats['User-Org deleted'] == 1
+        assert User.objects.count() == 1
+        assert UserOrganization.objects.count() == 1
+
 
 @pytest.mark.django_db
 class TestIdentitySync(object):

apps/logs/logic/data_import.py

@@ -4,7 +4,7 @@
 from typing import Optional
 
 from core.logic.debug import log_memory
-from logs.logic.validation import clean_and_validate_issn, ValidationError
+from logs.logic.validation import clean_and_validate_issn, ValidationError, normalize_isbn
 from logs.models import ImportBatch
 from nigiri.counter5 import CounterRecord
 from organizations.models import Organization
@@ -31,14 +31,7 @@ def get_or_create_with_map(model, mapping, attr_name, attr_value, other_attrs=No
 
 class TitleManager(object):
     def __init__(self):
-        # in the following, we use values_list to speed things up as there are a lot of objects
-        # and creating them takes a lot of time
-        # (e.g. processing time for import was cut from 3.5s to 1.2s by switching to this)
         self.key_to_title_id_and_pub_type = {}
-        # tuple(t[:5]): tuple(t[5:])
-        # for t in Title.objects.all().order_by().
-        # values_list('name', 'isbn', 'issn', 'eissn', 'doi', 'pk', 'pub_type')
-        # }
         self.stats = Counter()
 
     def prefetch_titles(self, records: [TitleRec]):
@@ -54,6 +47,29 @@ def prefetch_titles(self, records: [TitleRec]):
         }
         logger.debug('Prefetched %d records', len(self.key_to_title_id_and_pub_type))
 
+    @classmethod
+    def normalize_title_rec(cls, record: TitleRec) -> TitleRec:
+        """
+        Normalize specific fields in the record and return a new TitleRec with normalized data.
+        Should be run before one attempts to ingest the data into the database.
+        """
+        # normalize issn, eissn and isbn - they are sometimes malformed by whitespace in the data
+        issn = record.issn
+        if issn:
+            issn = clean_and_validate_issn(issn, raise_error=False)
+        eissn = record.eissn
+        if eissn:
+            eissn = clean_and_validate_issn(eissn, raise_error=False)
+        isbn = normalize_isbn(record.isbn) if record.isbn else record.isbn
+        return TitleRec(
+            name=record.name,
+            isbn=isbn,
+            issn=issn,
+            eissn=eissn,
+            doi=record.doi,
+            pub_type=record.pub_type,
+        )
+
     def get_or_create(self, record: TitleRec) -> Optional[int]:
         if not record.name:
             logger.warning(
@@ -64,23 +80,7 @@ def get_or_create(self, record: TitleRec) -> Optional[int]:
                 record.doi,
             )
             return None
-        # normalize issn, eissn and isbn - the are sometimes malformed by whitespace in the data
-        issn = record.issn
-        if issn:
-            try:
-                issn = clean_and_validate_issn(issn)
-            except ValidationError as e:
-                logger.error(f'Error: {e}')
-                issn = ''
-        eissn = record.eissn
-        if eissn:
-            try:
-                eissn = clean_and_validate_issn(eissn)
-            except ValidationError as e:
-                logger.error(f'Error: {e}')
-                eissn = ''
-        isbn = record.isbn.replace(' ', '') if record.isbn else record.isbn
-        key = (record.name, isbn, issn, eissn, record.doi)
+        key = (record.name, record.isbn, record.issn, record.eissn, record.doi)
         if key in self.key_to_title_id_and_pub_type:
             title_pk, db_pub_type = self.key_to_title_id_and_pub_type[key]
             # check if we need to improve the pub_type from UNKNOWN to something better
@@ -94,9 +94,9 @@ def get_or_create(self, record: TitleRec) -> Optional[int]:
         title = Title.objects.create(
             name=record.name,
             pub_type=record.pub_type,
-            isbn=isbn,
-            issn=issn,
-            eissn=eissn,
+            isbn=record.isbn,
+            issn=record.issn,
+            eissn=record.eissn,
             doi=record.doi,
         )
         self.key_to_title_id_and_pub_type[key] = (title.pk, record.pub_type)
@@ -113,11 +113,11 @@ def counter_record_to_title_rec(self, record: CounterRecord) -> TitleRec:
             if key == 'DOI':
                 doi = value
             elif key == 'Online_ISSN':
-                eissn = value
+                eissn = clean_and_validate_issn(value, raise_error=False) if value else value
             elif key == 'Print_ISSN':
-                issn = value
+                issn = clean_and_validate_issn(value, raise_error=False) if value else value
             elif key == 'ISBN':
-                isbn = value
+                isbn = normalize_isbn(value) if value else value
         pub_type = self.deduce_pub_type(eissn, isbn, issn, record)
         # convert None values for the following attrs to empty strings
         isbn = '' if isbn is None else isbn

apps/logs/logic/validation.py

@@ -1,5 +1,7 @@
+import logging
 import re
 
+logger = logging.getLogger(__name__)
 issn_matcher = re.compile(r'^\d{4}-\d{3}[\dX]$')
 
 
@@ -8,8 +10,15 @@ class ValidationError(Exception):
     pass
 
 
-def clean_and_validate_issn(text: str) -> str:
+def clean_and_validate_issn(text: str, raise_error=True) -> str:
     clean = ''.join(text.split())  # remove all whitespace
     if issn_matcher.match(clean):
         return clean
-    raise ValidationError(f'Invalid ISSN: "{text}"')
+    if raise_error:
+        raise ValidationError(f'Invalid ISSN: "{text}"')
+    logger.warning('Invalid ISSN: "%s"', text)
+    return ''
+
+
+def normalize_isbn(isbn: str) -> str:
+    return isbn.replace(' ', '')

apps/nigiri/tests/test_fetching.py

@@ -3,13 +3,13 @@
 import pytest
 
 from nigiri.counter5 import Counter5ReportBase
-from organizations.tests.conftest import organizations
-from logs.tests.conftest import report_type_nd
 
 from nigiri.client import Sushi5Client
 from publications.models import Platform
 from sushi.logic.data_import import import_sushi_credentials
 from sushi.models import SushiCredentials, CounterReportType
+from organizations.tests.conftest import organizations
+from logs.tests.conftest import report_type_nd
 
 
 @pytest.mark.django_db

apps/organizations/migrations/0015_nullable_ext_id.py

@@ -0,0 +1,20 @@
+# Generated by Django 2.2.13 on 2020-06-25 07:56
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('organizations', '0014_nullable_ico'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='organization',
+            name='ext_id',
+            field=models.PositiveIntegerField(
+                default=None, help_text='object ID taken from EMRS', null=True, unique=True
+            ),
+        ),
+    ]

apps/organizations/models.py

@@ -1,6 +1,5 @@
 from django.conf import settings
 from django.contrib.postgres.fields import JSONField
-from django.core.exceptions import ValidationError
 from django.db import models
 from mptt.fields import TreeForeignKey
 from mptt.models import MPTTModel
@@ -10,7 +9,9 @@
 
 class Organization(MPTTModel):
 
-    ext_id = models.PositiveIntegerField(unique=True, help_text='object ID taken from EMRS')
+    ext_id = models.PositiveIntegerField(
+        unique=True, help_text='object ID taken from EMRS', null=True, default=None
+    )
     parent = TreeForeignKey(
         'self', on_delete=models.CASCADE, null=True, blank=True, related_name='children'
     )

apps/organizations/tests/conftest.py

@@ -51,7 +51,7 @@ def identity_by_user_type(
 ):
     def fn(user_type):
         org = organizations[0]
-        # we do not user admin_client, master_client, etc. because the way the fixtures work
+        # we do not use admin_client, master_client, etc. because the way the fixtures work
         # they all point to the same client object which obviously does not work
         if user_type == 'no_user':
             identity = None

apps/organizations/tests/test_api.py

@@ -15,7 +15,7 @@
 class TestOrganizationAPI(object):
     def test_unauthorized_user(self, client, invalid_identity, authentication_headers):
         resp = client.get(reverse('organization-list'), **authentication_headers(invalid_identity))
-        assert resp.status_code == 403
+        assert resp.status_code in (403, 401)  # depends on auth backend
 
     def test_authorized_user_no_orgs(self, authenticated_client):
         resp = authenticated_client.get(reverse('organization-list'))

apps/publications/tests/test_title_manager.py

@@ -0,0 +1,23 @@
+import pytest
+
+from logs.logic.data_import import TitleManager, TitleRec
+from publications.models import Title
+
+
+@pytest.mark.django_db
+class TestTitleManager(object):
+    def test_mangled_isbn(self):
+        """
+        Test for a bug that TitleManager looks for data in database with non-normalized isbn
+        but uses normalized ISBN when storing new data. This discrepancy may lead to
+        database level integrity error because of constraints.
+        :return:
+        """
+        Title.objects.create(name='Foo', isbn='978-0-07-174521-5')
+        tm = TitleManager()
+        record = TitleRec(
+            name='Foo', isbn='978- 0-07-174521-5', issn='', eissn='', doi='', pub_type='U'
+        )
+        record = tm.normalize_title_rec(record)
+        tm.prefetch_titles(records=[record])
+        tm.get_or_create(record)

apps/sushi/admin.py

@@ -79,6 +79,24 @@ def queryset(self, request, queryset):
         return queryset
 
 
+class HistoryMode(admin.SimpleListFilter):
+    title = 'history mode'
+    parameter_name = 'hm'
+
+    def lookups(self, request, model_admin):
+        return (
+            ('current', 'Current only'),
+            ('current_and_success', 'Current and sucessful'),
+        )
+
+    def queryset(self, request, queryset):
+        if self.value() == 'current':
+            return queryset.current()
+        if self.value() == 'current_and_success':
+            return queryset.current_or_successful()
+        return queryset
+
+
 @admin.register(models.SushiFetchAttempt)
 class SushiFetchAttemptAdmin(admin.ModelAdmin):
 
@@ -100,6 +118,7 @@ class SushiFetchAttemptAdmin(admin.ModelAdmin):
         'has_import_batch',
     ]
     list_filter = [
+        HistoryMode,
         'download_success',
         'processing_success',
         'is_processed',