Fix parse_doi: DOI lost when a non-doi ELocationID (e.g. pii) follows it

pubmed_parser/medline_parser.py

@@ -62,20 +62,23 @@ def parse_doi(pubmed_article):
     article = medline.find("Article")
     elocation_ids = article.findall("ELocationID")
 
-    if len(elocation_ids) > 0:
-        for e in elocation_ids:
-            doi = e.text.strip() or "" if e.attrib.get("EIdType", "") == "doi" else ""
-    else:
+    doi = ""
+    # Prefer the DOI from ELocationID. Stop at the first one whose
+    # EIdType is "doi"; otherwise a later ELocationID of a different
+    # type (e.g. "pii") would overwrite an already-found DOI with "".
+    for e in elocation_ids:
+        if e.attrib.get("EIdType", "") == "doi" and e.text is not None:
+            doi = e.text.strip()
+            if doi:
+                break
+    # Fall back to PubmedData/ArticleIdList when no DOI is present in
+    # the ELocationID list (this also covers records with no ELocationID).
+    if not doi:
         article_ids = pubmed_article.find("PubmedData/ArticleIdList")
         if article_ids is not None:
-            doi = article_ids.find('ArticleId[@IdType="doi"]')
-            doi = (
-                (doi.text.strip() if doi.text is not None else "")
-                if doi is not None
-                else ""
-            )
-        else:
-            doi = ""
+            doi_node = article_ids.find('ArticleId[@IdType="doi"]')
+            if doi_node is not None and doi_node.text is not None:
+                doi = doi_node.text.strip()
     return doi
 
 

tests/test_medline_parser.py

@@ -24,6 +24,13 @@ def fetch_compressed_medline_xml(pubmed_id):
 article_36400559 = parsed_medline[0]
 article_28786991 = parsed_medline[1]
 
+# Record whose <Article> lists a "doi" ELocationID *before* a "pii" one.
+# Used to guard against a trailing non-doi ELocationID overwriting the DOI.
+parsed_medline_doi_then_pii = list(
+    pp.parse_medline_xml(fetch_compressed_medline_xml(['24746000']))
+)
+article_24746000 = parsed_medline_doi_then_pii[0]
+
 
 def test_abstract():
     """This is a test for the abstract field."""
@@ -70,6 +77,19 @@ def test_doi():
     assert article_28786991['doi'] == '10.1371/journal.pone.0180707'
 
 
+def test_doi_with_trailing_pii_elocation_id():
+    """DOI must survive a later non-doi ELocationID (e.g. pii).
+
+    PMID 24746000 lists::
+
+        <ELocationID EIdType="doi" ...>10.1016/S2213-2600(14)70019-0</ELocationID>
+        <ELocationID EIdType="pii" ...>S2213-2600(14)70019-0</ELocationID>
+
+    The trailing "pii" entry must not overwrite the parsed DOI with "".
+    """
+    assert article_24746000['doi'] == '10.1016/S2213-2600(14)70019-0'
+
+
 def test_grant_ids():
     """This is a test for the grant_ids field."""
     assert article_36400559['grant_ids'] == []