Merge branch 'feature/3570_incorrect_issns' into develop

doajtest/matrices/article_create_article/issn_validation_against_journal.matrix.csv

@@ -0,0 +1,17 @@
+test_id,eissn,pissn,validated
+1,eissn_in_doaj,pissn_in_doaj,yes
+2,eissn_in_doaj,eissn_not_in_doaj,
+3,eissn_in_doaj,pissn_not_in_doaj,
+4,eissn_in_doaj,!eissn_in_doaj,
+5,pissn_in_doaj,eissn_in_doaj,
+6,pissn_in_doaj,eissn_not_in_doaj,
+7,pissn_in_doaj,pissn_not_in_doaj,
+8,pissn_in_doaj,!pissn_in_doaj,
+9,eissn_not_in_doaj,eissn_in_doaj,
+10,eissn_not_in_doaj,pissn_in_doaj,
+11,eissn_not_in_doaj,pissn_not_in_doaj,
+12,eissn_not_in_doaj,!eissn_not_in_doaj,
+13,pissn_not_in_doaj,eissn_in_doaj,
+14,pissn_not_in_doaj,pissn_in_doaj,
+15,pissn_not_in_doaj,eissn_not_in_doaj,
+16,pissn_not_in_doaj,!pissn_not_in_doaj,

doajtest/matrices/article_create_article/issn_validation_against_journal.settings.csv

@@ -0,0 +1,19 @@
+field,test_id,eissn,pissn,validated
+type,index,generated,generated,conditional
+deafult,,,,no
+,,,,
+values,,eissn_in_doaj,eissn_in_doaj,yes
+values,,pissn_in_doaj,pissn_in_doaj,no
+values,,eissn_not_in_doaj,eissn_not_in_doaj,
+values,,pissn_not_in_doaj,pissn_not_in_doaj,
+,,,,
+,,,,
+conditional validated,,eissn_in_doaj,pissn_in_doaj,yes
+constraint eissn,,eissn_in_doaj,!eissn_in_doaj,
+constraint eissn,,eissn_not_in_doaj,!eissn_not_in_doaj,
+constraint eissn,,pissn_not_in_doaj,!pissn_not_in_doaj,
+constraint eissn,,pissn_in_doaj,!pissn_in_doaj,
+constraint pissn,,eissn_in_doaj,!eissn_in_doaj,
+constraint pissn,,eissn_not_in_doaj,!eissn_not_in_doaj,
+constraint pissn,,pissn_not_in_doaj,!pissn_not_in_doaj,
+constraint pissn,,pissn_in_doaj,!pissn_in_doaj,

doajtest/matrices/article_create_article/issn_validation_against_journal.settings.json

@@ -0,0 +1,119 @@
+{
+  "parameters": [
+    {
+      "name": "test_id",
+      "type": "index"
+    },
+    {
+      "name": "eissn",
+      "type": "generated",
+      "values": {
+        "eissn_in_doaj": {
+          "constraints": {
+            "pissn": {
+              "nor": [
+                "eissn_in_doaj"
+              ]
+            }
+          }
+        },
+        "pissn_in_doaj": {
+          "constraints": {
+            "pissn": {
+              "nor": [
+                "pissn_in_doaj"
+              ]
+            }
+          }
+        },
+        "eissn_not_in_doaj": {
+          "constraints": {
+            "pissn": {
+              "nor": [
+                "eissn_not_in_doaj"
+              ]
+            }
+          }
+        },
+        "pissn_not_in_doaj": {
+          "constraints": {
+            "pissn": {
+              "nor": [
+                "pissn_not_in_doaj"
+              ]
+            }
+          }
+        }
+      }
+    },
+    {
+      "name": "pissn",
+      "type": "generated",
+      "values": {
+        "eissn_in_doaj": {},
+        "pissn_in_doaj": {},
+        "eissn_not_in_doaj": {},
+        "pissn_not_in_doaj": {},
+        "!eissn_in_doaj": {
+          "constraints": {
+            "eissn": {
+              "or": [
+                "eissn_in_doaj"
+              ]
+            }
+          }
+        },
+        "!eissn_not_in_doaj": {
+          "constraints": {
+            "eissn": {
+              "or": [
+                "eissn_not_in_doaj"
+              ]
+            }
+          }
+        },
+        "!pissn_not_in_doaj": {
+          "constraints": {
+            "eissn": {
+              "or": [
+                "pissn_not_in_doaj"
+              ]
+            }
+          }
+        },
+        "!pissn_in_doaj": {
+          "constraints": {
+            "eissn": {
+              "or": [
+                "pissn_in_doaj"
+              ]
+            }
+          }
+        }
+      }
+    },
+    {
+      "name": "validated",
+      "type": "conditional",
+      "values": {
+        "yes": {
+          "conditions": [
+            {
+              "eissn": {
+                "or": [
+                  "eissn_in_doaj"
+                ]
+              },
+              "pissn": {
+                "or": [
+                  "pissn_in_doaj"
+                ]
+              }
+            }
+          ]
+        },
+        "no": {}
+      }
+    }
+  ]
+}

doajtest/unit/test_article_acceptable_and_permissions.py

@@ -14,6 +14,11 @@ def is_acceptable_load_cases():
                                "test_id",
                                {"test_id": []})
 
+def issn_validation_against_journal_load_sets():
+    return load_parameter_sets(rel2abs(__file__, "..", "matrices", "article_create_article"), "issn_validation_against_journal",
+                               "test_id",
+                               {"test_id": []})
+
 
 class TestBLLPrepareUpdatePublisher(DoajTestCase):
 
@@ -110,4 +115,73 @@ def test_has_permissions(self):
         assert failed_result["unowned"].sort() == [pissn, eissn].sort()
         # assert failed_result == {'success': 0, 'fail': 1, 'update': 0, 'new': 0, 'shared': [],
         #                          'unowned': [pissn, eissn],
-        #                          'unmatched': []}, "received: {}".format(failed_result)
+        #                          'unmatched': []}, "received: {}".format(failed_result)
+
+
+    @parameterized.expand(issn_validation_against_journal_load_sets)
+    def test_issn_validation_against_journal_load_sets(self, value, kwargs):
+        kwpissn = kwargs.get("pissn")
+        kweissn = kwargs.get("eissn")
+        validated = kwargs.get("validated")
+
+        js = JournalFixtureFactory.make_many_journal_sources(2)
+        journal_in_doaj = Journal(**js[0])
+        journal_in_doaj.set_in_doaj(True)
+        journal_in_doaj.bibjson().pissn = "1111-1111"
+        journal_in_doaj.bibjson().eissn = "2222-2222"
+        journal_in_doaj.save(blocking=True)
+
+        journal_not_in_doaj = Journal(**js[1])
+        journal_not_in_doaj.set_in_doaj(False)
+        journal_not_in_doaj.bibjson().pissn = "3333-3333"
+        journal_not_in_doaj.bibjson().eissn = "4444-4444"
+        journal_not_in_doaj.save(blocking=True)
+
+        if (kwpissn == "pissn_in_doaj"):
+            pissn = journal_in_doaj.bibjson().pissn
+        elif (kwpissn == "eissn_in_doaj"):
+            pissn = journal_in_doaj.bibjson().eissn
+        elif (kwpissn == "pissn_not_in_doaj"):
+            pissn = journal_not_in_doaj.bibjson().pissn
+        else:
+            pissn = journal_not_in_doaj.bibjson().eissn
+
+        if (kweissn == "pissn_in_doaj"):
+            eissn = journal_in_doaj.bibjson().pissn
+        elif (kweissn == "eissn_in_doaj"):
+            eissn = journal_in_doaj.bibjson().eissn
+        elif (kweissn == "pissn_not_in_doaj"):
+            eissn = journal_not_in_doaj.bibjson().pissn
+        else:
+            eissn = journal_not_in_doaj.bibjson().eissn
+
+
+        art_source = ArticleFixtureFactory.make_article_source(pissn=pissn, eissn=eissn)
+        article = Article(**art_source)
+
+        if validated:
+            self.assertIsNone(self.svc.is_acceptable(article))
+
+        else:
+            with self.assertRaises(exceptions.ArticleNotAcceptable):
+                self.svc.is_acceptable(article)
+
+    def test_check_validation_for_2_journals(self):
+
+        js = JournalFixtureFactory.make_many_journal_sources(2, in_doaj=True)
+        journal_in_doaj = Journal(**js[0])
+        journal_in_doaj.bibjson().pissn = "1111-1111"
+        journal_in_doaj.bibjson().eissn = "2222-2222"
+        journal_in_doaj.save(blocking=True)
+
+        journal_not_in_doaj = Journal(**js[1])
+        journal_not_in_doaj.bibjson().pissn = "3333-3333"
+        journal_not_in_doaj.bibjson().eissn = "4444-4444"
+        journal_not_in_doaj.save(blocking=True)
+
+
+        art_source = ArticleFixtureFactory.make_article_source(pissn="1111-1111", eissn="4444-4444")
+        article = Article(**art_source)
+
+        with self.assertRaises(exceptions.ArticleNotAcceptable):
+            self.svc.is_acceptable(article)

doajtest/unit/test_models.py

@@ -1661,3 +1661,30 @@ def test_get_name_safe(self):
         # account does not exist
         assert models.Account.get_name_safe('not existing account id') == ''
 
+    def test_11_find_by_issn(self):
+        js = JournalFixtureFactory.make_many_journal_sources(2, in_doaj=True)
+        j1 = models.Journal(**js[0])
+        j1.bibjson().pissn = "1111-1111"
+        j1.bibjson().eissn = "2222-2222"
+        j1.save(blocking=True)
+
+        j2 = models.Journal(**js[1])
+        j2.bibjson().pissn = "3333-3333"
+        j2.bibjson().eissn = "4444-4444"
+        j2.save(blocking=True)
+
+        journals = models.Journal.find_by_issn(["1111-1111", "2222-2222"], True)
+        assert len(journals) == 1
+        assert journals[0].id == j1.id
+
+        journals = models.Journal.find_by_issn(["1111-1111", "3333-3333"], True)
+        assert len(journals) == 2
+        assert journals[0].id == j1.id
+        assert journals[1].id == j2.id
+
+        journals = models.Journal.find_by_issn_exact(["1111-1111", "2222-2222"], True)
+        assert len(journals) == 1
+        assert journals[0].id == j1.id
+
+        journals = models.Journal.find_by_issn_exact(["1111-1111", "3333-3333"], True)
+        assert len(journals) == 0

portality/bll/services/article.py

@@ -159,9 +159,6 @@ def _validate_issns(article_bibjson: models.ArticleBibJSON):
         if len(pissn) > 1 or len(eissn) > 1:
             raise exceptions.ArticleNotAcceptable(message=Messages.EXCEPTION_TOO_MANY_ISSNS)
 
-        pissn = article_bibjson.get_one_identifier("pissn")
-        eissn = article_bibjson.get_one_identifier("eissn")
-
         # no pissn or eissn
         if not pissn and not eissn:
             raise exceptions.ArticleNotAcceptable(message=Messages.EXCEPTION_NO_ISSNS)
@@ -252,7 +249,8 @@ def has_permissions(self, account, article, limit_to_account):
     def is_acceptable(self, article: models.Article):
         """
         conduct some deep validation on the article to make sure we will accept it
-        or the moment, this just means making sure it has a DOI and a fulltext
+        this just means making sure it has a DOI and a fulltext, and that its ISSNs
+        match a single journal
         """
         try:
             bj = article.bibjson()
@@ -266,12 +264,39 @@ def is_acceptable(self, article: models.Article):
             raise exceptions.ArticleNotAcceptable(message=Messages.EXCEPTION_NO_DOI_NO_FULLTEXT)
 
         self._validate_issns(bj)
+        journal = self.match_journal_with_validation(bj)
 
         # is journal in doaj (we do this check last as it has more performance impact)
-        journal = article.get_journal()
         if journal is None or not journal.is_in_doaj():
             raise exceptions.ArticleNotAcceptable(message=Messages.EXCEPTION_ADDING_ARTICLE_TO_WITHDRAWN_JOURNAL)
 
+    @staticmethod
+    def match_journal_with_validation(article_bibjson: models.ArticleBibJSON):
+        pissn = article_bibjson.get_one_identifier("pissn")
+        eissn = article_bibjson.get_one_identifier("eissn")
+
+        issns = []
+
+        if pissn is not None:
+            issns.append(pissn)
+        if eissn is not None:
+            issns.append(eissn)
+
+        journal = models.Journal.find_by_issn_exact(issns, True)
+
+        # check if only one journal matches pissn and eissn and if they are in the correct fields
+        # no need to check eissn, if pissn matches, pissn and eissn are different and only 1 journal has been found - then eissn matches too
+        if len(journal) != 1:
+            raise exceptions.ArticleNotAcceptable(message=Messages.EXCEPTION_MISMATCHED_ISSNS)
+        if pissn is not None:
+            if journal[0].bibjson().pissn != pissn:
+                raise exceptions.ArticleNotAcceptable(message=Messages.EXCEPTION_MISMATCHED_ISSNS)
+        if eissn is not None:
+            if journal[0].bibjson().eissn != eissn:
+                raise exceptions.ArticleNotAcceptable(message=Messages.EXCEPTION_MISMATCHED_ISSNS)
+
+        return journal[0]
+
     @staticmethod
     def is_legitimate_owner(article, owner):
         """

portality/models/v2/journal.py

@@ -70,6 +70,22 @@ def find_by_issn(cls, issns, in_doaj=None, max=10):
         records = [cls(**r.get("_source")) for r in result.get("hits", {}).get("hits", [])]
         return records
 
+    @classmethod
+    def find_by_issn_exact(cls, issns, in_doaj=None, max=2):
+        """
+        Finds journal that matches given issns exactly - if no data problems should always be only 1
+        """
+        if not isinstance(issns, list):
+            issns = [issns]
+        if len(issns) > 2:
+            return []
+        q = JournalQuery()
+        q.find_by_issn_exact(issns, in_doaj=in_doaj, max=max)
+        result = cls.query(q=q.query)
+        # create an array of objects, using cls rather than Journal, which means subclasses can use it too
+        records = [cls(**r.get("_source")) for r in result.get("hits", {}).get("hits", [])]
+        return records
+
     @classmethod
     def issns_by_owner(cls, owner, in_doaj=None):
         q = IssnQuery(owner, in_doaj=in_doaj)
@@ -922,6 +938,16 @@ class JournalQuery(object):
         }
     }
 
+    must_query = {
+        "track_total_hits": True,
+        "query": {
+            "bool": {
+                "must": [
+                ]
+            }
+        }
+    }
+
     all_doaj = {
         "track_total_hits": True,
         "query": {
@@ -947,6 +973,14 @@ def find_by_issn(self, issns, in_doaj=None, max=10):
             self.query["query"]["bool"]["must"].append({"term": {"admin.in_doaj": in_doaj}})
         self.query["size"] = max
 
+    def find_by_issn_exact(self, issns, in_doaj=None, max=10):
+        self.query = deepcopy(self.must_query)
+        for issn in issns:
+            self.query["query"]["bool"]["must"].append({"term": {"index.issn.exact": issn}})
+        if in_doaj is not None:
+            self.query["query"]["bool"]["must"].append({"term": {"admin.in_doaj": in_doaj}})
+        self.query["size"] = max
+
     def all_in_doaj(self):
         q = deepcopy(self.all_doaj)
         if self.minified: