Merge pull request #25 from TailorDev/add-doi-tests

Make batch importation more robust

requirements/dev.txt

@@ -13,6 +13,7 @@ whitenoise
 pytest>=3.0.7
 pytest-cov>=2.4.0
 pytest-django>=3.1.2
+pytest-mock>=1.6.3
 flake8
 coveralls
 

sandbox/static/styles.css

@@ -66,6 +66,11 @@ div[role="main"] {
   color: #fff;
 }
 
+.messages li.error {
+  background-color: red;
+  color: #fff;
+}
+
 .import #main-content {
   padding: 0;
 }

setup.cfg

@@ -3,7 +3,7 @@ universal=1
 
 [tool:pytest]
 DJANGO_SETTINGS_MODULE=sandbox.settings
-addopts = -vs --cov=td_biblio
+addopts = -vs --cov=td_biblio --cov-report term-missing
 testpaths = td_biblio/tests
 
 [flake8]

td_biblio/exceptions.py

@@ -0,0 +1,15 @@
+# td_biblio exceptions
+class BaseLoaderError(Exception):
+    pass
+
+
+class BibTeXLoaderError(BaseLoaderError):
+    pass
+
+
+class PMIDLoaderError(BaseLoaderError):
+    pass
+
+
+class DOILoaderError(BaseLoaderError):
+    pass

td_biblio/models.py

@@ -48,14 +48,21 @@ def get_formatted_name(self):
     def _set_user(self):
         """Look for local django user based on human name"""
 
+        if '' in (self.last_name, self.first_name):
+            return
+
+        self._set_first_initial()
+
         User = get_user_model()
         try:
             self.user = User.objects.get(
                 models.Q(last_name__iexact=self.last_name),
                 models.Q(first_name__iexact=self.first_name) |
                 models.Q(first_name__istartswith=self.first_initial[0])
             )
-        except:
+        except User.DoesNotExist:
+            pass
+        except User.MultipleObjectsReturned:
             pass
 
 

td_biblio/tests/fixtures/entries.py

@@ -23,6 +23,8 @@
     '26408185',
     '26588162',
     '27095822',
+    '24357323',
+    '25190796',
 ]
 
 # Digital Object Identfiers
@@ -52,4 +54,6 @@
     '10.1186/s13059-015-0769-z',
     '10.1210/jc.2007-0927',
     '10.1210/jc.2014-4047',
+    '10.1126/science.1241089',
+    '10.1126/science.1255274',
 ]

td_biblio/tests/test_loaders.py

@@ -13,6 +13,7 @@
 from eutils.exceptions import EutilsNCBIError
 from requests.exceptions import HTTPError
 
+from ..exceptions import DOILoaderError, PMIDLoaderError
 from ..utils.loaders import BibTeXLoader, DOILoader, PubmedLoader
 from ..models import Author, Entry, Journal
 from .fixtures.entries import PMIDs as FPMIDS, DOIs as FDOIS
@@ -206,7 +207,33 @@ def test_save_records_from_mutiple_pmid(self):
         self.loader.load_records(PMIDs=FPMIDS)
         self.loader.save_records()
 
-        self.assertEqual(Entry.objects.count(), 21)
+        self.assertEqual(Entry.objects.count(), len(FPMIDS))
+
+
+@pytest.mark.django_db
+class PubmedLoaderToRecordTests(TestCase):
+    """
+    Tests for the patched pubmed loader
+    """
+    def setUp(self):
+        """
+        Set object level vars
+        """
+        self.PMID = 26588162
+        self.loader = PubmedLoader()
+
+    @pytest.fixture(autouse=True)
+    def mock_to_record_with_exception(self, mocker):
+
+        def raise_exception(self, msg):
+            raise PMIDLoaderError('Patched PMIDLoaderError')
+
+        mocker.patch.object(PubmedLoader, 'to_record', raise_exception)
+
+    def test_load_records_with_to_record_exception(self):
+
+        with pytest.raises(PMIDLoaderError):
+            self.loader.load_records(PMIDs=self.PMID)
 
 
 @pytest.mark.django_db
@@ -252,7 +279,7 @@ def test_load_records_from_an_existing_doi(self):
                     'last_name': 'Tufféry'
                 }
             ],
-            'journal': 'J. Chem. Theory Comput.',
+            'journal': 'Journal of Chemical Theory and Computation',
             'volume': '10',
             'number': '10',
             'pages': '4745-4758',
@@ -291,7 +318,6 @@ def test_save_records_from_an_existing_doi(self):
         self.assertEqual(Journal.objects.count(), 0)
 
         self.loader.load_records(DOIs=[self.doi, ])
-        print('type: {}'.format(self.doi.__class__.__name__))
         self.loader.save_records()
 
         self.assertEqual(Author.objects.count(), 4)
@@ -321,4 +347,30 @@ def test_save_records_from_several_dois(self):
         self.loader.load_records(DOIs=FDOIS)
         self.loader.save_records()
 
-        self.assertEqual(Entry.objects.count(), 25)
+        self.assertEqual(Entry.objects.count(), len(FDOIS))
+
+
+@pytest.mark.django_db
+class DOILoaderToRecordTests(TestCase):
+    """
+    Tests for the patched pubmed loader
+    """
+    def setUp(self):
+        """
+        Set object level vars
+        """
+        self.doi = '10.1021/ct500592m'
+        self.loader = DOILoader()
+
+    @pytest.fixture(autouse=True)
+    def mock_to_record_with_exception(self, mocker):
+
+        def raise_exception(self, msg):
+            raise DOILoaderError('Patched DOILoaderError')
+
+        mocker.patch.object(DOILoader, 'to_record', raise_exception)
+
+    def test_load_records_with_to_record_exception(self):
+
+        with pytest.raises(DOILoaderError):
+            self.loader.load_records(DOIs=self.doi)

td_biblio/utils/loaders.py

@@ -7,6 +7,7 @@
 import datetime
 import json
 import logging
+import sys
 
 import bibtexparser
 import eutils.client
@@ -19,6 +20,7 @@
 from django.utils.translation import ugettext_lazy as _
 from habanero import cn
 
+from ..exceptions import DOILoaderError, PMIDLoaderError
 from ..models import Author, Journal, Entry, AuthorEntryRank
 
 
@@ -155,7 +157,7 @@ class BibTeXLoader(BaseLoader):
 
     Usage:
 
-    >>> from td_biblio.utils.managers import BibTeXLoader
+    >>> from td_biblio.utils.loaders import BibTeXLoader
     >>> loader = BibTeXLoader()
     >>> loader.load_records(bibtex_filename='foo.bib')
     >>> loader.save_records()
@@ -179,7 +181,7 @@ def to_record(self, input):
         # Check if month is numerical or not
         try:
             int(pub_date['month'])
-        except:
+        except ValueError:
             pub_date['month'] = strptime(pub_date['month'], '%b').tm_mon
         # Convert date fields to integers
         pub_date = dict(
@@ -221,7 +223,7 @@ class PubmedLoader(BaseLoader):
 
     Usage:
 
-    >>> from td_biblio.utils.managers import PubmedLoader
+    >>> from td_biblio.utils.loaders import PubmedLoader
     >>> loader = PubmedLoader()
     >>> loader.load_records(PMIDs=26588162)
     >>> loader.save_records()
@@ -261,7 +263,24 @@ def load_records(self, PMIDs=None):
         """Load all PMIDs as valid records"""
 
         entries = self.client.efetch(db='pubmed', id=PMIDs)
-        self.records = [self.to_record(r) for r in entries]
+        self.records = []
+
+        for entry in entries:
+            try:
+                record = self.to_record(entry)
+            except Exception:
+                e, v, tb = sys.exc_info()
+                msg = _(
+                    "An error occured while loading the following PMID: {}. "
+                    "Check logs for details."
+                ).format(
+                    entry.pmid
+                )
+                logger.error(
+                    '{}, error: {} [{}], data: {}'.format(msg, e, v, entry)
+                )
+                raise PMIDLoaderError(msg)
+            self.records.append(record)
 
 
 class DOILoader(BaseLoader):
@@ -271,7 +290,7 @@ class DOILoader(BaseLoader):
 
     Usage:
 
-    >>> from td_biblio.utils.managers import DOILoader
+    >>> from td_biblio.utils.loaders import DOILoader
     >>> loader = DOILoader()
     >>> loader.load_records(DOIs='10.1021/ct500592m')
     >>> loader.save_records()
@@ -298,9 +317,9 @@ def to_record(self, input):
             'title': input.get('title', ''),
             'authors': [
                 {
-                    'first_name': a['given'],
-                    'last_name': a['family']
-                } for a in input['author']
+                    'first_name': a.get('given', ''),
+                    'last_name': a.get('family', '')
+                } for a in input.get('author')
             ],
             'journal': journal,
             'volume': input.get('volume', ''),
@@ -320,4 +339,21 @@ def load_records(self, DOIs=None):
         # Records might be a str or unicode (python 2)
         if not isinstance(records, list):
             records = [records, ]
-        self.records = [self.to_record(json.loads(r)) for r in records]
+        self.records = []
+        for r in records:
+            data = json.loads(r)
+            try:
+                record = self.to_record(data)
+            except Exception:
+                e, v, tb = sys.exc_info()
+                msg = _(
+                    "An error occured while loading the following DOI: {}. "
+                    "Check logs for details."
+                ).format(
+                    data.get('DOI')
+                )
+                logger.error(
+                    '{}, error: {} [{}], data: {}'.format(msg, e, v, data)
+                )
+                raise DOILoaderError(msg)
+            self.records.append(record)

td_biblio/views.py

@@ -1,16 +1,20 @@
 # -*- coding: utf-8 -*-
 import datetime
+import logging
 
 from django.contrib import messages
 from django.contrib.auth.decorators import login_required, user_passes_test
 from django.core.urlresolvers import reverse_lazy
 from django.utils.translation import ugettext_lazy as _
 from django.views.generic import FormView, ListView
 
+from .exceptions import DOILoaderError, PMIDLoaderError
 from .forms import EntryBatchImportForm
 from .models import Author, Collection, Entry, Journal
 from .utils.loaders import DOILoader, PubmedLoader
 
+logger = logging.getLogger('td_biblio')
+
 
 def superuser_required(function=None):
     """
@@ -53,23 +57,23 @@ def get(self, request, *args, **kwargs):
         # Is it an integer?
         try:
             self.current_publication_date = datetime.date(int(year), 1, 1)
-        except:
+        except TypeError:
             self.current_publication_date = None
 
         # -- Publication author
         author = self.request.GET.get('author', None)
         # Is it an integer?
         try:
             self.current_publication_author = int(author)
-        except:
+        except TypeError:
             self.current_publication_author = None
 
         # -- Publication collection
         collection = self.request.GET.get('collection', None)
         # Is it an integer?
         try:
             self.current_publication_collection = int(collection)
-        except:
+        except TypeError:
             self.current_publication_collection = None
 
         return super(EntryListView, self).get(request, *args, **kwargs)
@@ -157,14 +161,26 @@ def form_valid(self, form):
         pmids = form.cleaned_data['pmids']
         if len(pmids):
             pm_loader = PubmedLoader()
-            pm_loader.load_records(PMIDs=pmids)
+
+            try:
+                pm_loader.load_records(PMIDs=pmids)
+            except PMIDLoaderError as e:
+                messages.error(self.request, e)
+                return self.form_invalid(form)
+
             pm_loader.save_records()
 
         # DOIs
         dois = form.cleaned_data['dois']
         if len(dois):
             doi_loader = DOILoader()
-            doi_loader.load_records(DOIs=dois)
+
+            try:
+                doi_loader.load_records(DOIs=dois)
+            except DOILoaderError as e:
+                messages.error(self.request, e)
+                return self.form_invalid(form)
+
             doi_loader.save_records()
 
         messages.success(