book_loader was calling a merge_works more than once on the same pair of works, causing an integrity exception in WasWorks.

Changes made in add_related so that merge_works called only once for a given pair of works.   Also put in some safeguards in merge_works to prevent from self-merging and from merging when either works is missing an id (which should happen if a work has been deleted.)  Maybe we should check also to see whether the id for was is already in WasWorks

core/bookloader.py

@@ -348,6 +348,7 @@ def add_related(isbn):
     other_editions = {}
     for other_isbn in thingisbn(isbn):
         # 979's come back as 13
+        logger.debug("other_isbn: %s", other_isbn)
         if len(other_isbn)==10:
             other_isbn = regluit.core.isbn.convert_10_to_13(other_isbn)
         related_edition = add_by_isbn(other_isbn, work=work)
@@ -357,6 +358,7 @@ def add_related(isbn):
             if edition.work.language == related_language:
                 new_editions.append(related_edition)
                 if related_edition.work != edition.work:
+                    logger.debug("merge_works path 1 %s %s", edition.work.id, related_edition.work.id )
                     merge_works(edition.work, related_edition.work)
             else:
                 if other_editions.has_key(related_language):
@@ -366,11 +368,15 @@ def add_related(isbn):
 
     # group the other language editions together
     for lang_group in other_editions.itervalues():
+        logger.debug("lang_group (ed, work): %s", [(ed.id, ed.work.id) for ed in lang_group])
         if len(lang_group)>1:
             lang_edition = lang_group[0]
-            for related_edition in lang_group[1:]:
-                if lang_edition.work != related_edition.work:
-                    merge_works(lang_edition.work, related_edition.work)
+            logger.debug("lang_edition.id: %s", lang_edition.id)
+            # compute the distinct set of works to merge into lang_edition.work
+            works_to_merge = set([ed.work for ed in lang_group[1:]]) - set([lang_edition.work])
+            for w in works_to_merge:
+                logger.debug("merge_works path 2 %s %s", lang_edition.work.id, w.id )
+                merge_works(lang_edition.work, w)
         
     return new_editions
     
@@ -390,6 +396,10 @@ def merge_works(w1, w2):
     """will merge the second work (w2) into the first (w1)
     """
     logger.info("merging work %s into %s", w2, w1)
+    # don't merge if the works are the same or at least one of the works has no id (for example, when w2 has already been deleted)
+    if w1 == w2 or w1.id is None or w2.id is None:
+        return
+    
     for identifier in w2.identifiers.all():
         identifier.work = w1
         identifier.save()
@@ -411,6 +421,7 @@ def merge_works(w1, w2):
     for ww in models.WasWork.objects.filter(work = w2):
         ww.work = w1
         ww.save()
+        
     w2.delete()
 
 

core/tests.py

@@ -12,7 +12,7 @@ from django.contrib.contenttypes.models import ContentType
 from django.contrib.sites.models import Site
 
 from regluit.payment.models import Transaction
-from regluit.core.models import Campaign, Work, UnglueitError
+from regluit.core.models import Campaign, Work, UnglueitError, Edition
 from regluit.core import bookloader, models, search, goodreads, librarything
 from regluit.core import isbn
 from regluit.payment.parameters import PAYMENT_TYPE_AUTHORIZATION
@@ -97,6 +97,57 @@ class BookLoaderTests(TestCase):
         edition.publication_date = None
         self.assertTrue(edition.work.publication_date)
 
+    def test_merge_works_mechanics(self):
+        """Make sure then merge_works is still okay when we try to merge works with themselves and with deleted works"""
+        w1 = Work(title="Work 1")
+        w1.save()
+        
+        w2 = Work(title="Work 2")
+        w2.save()
+        
+        e1 = Edition(work=w1)
+        e1.save()
+        
+        e2 = Edition(work=w2)
+        e2.save()
+        
+        e2a = Edition(work=w2)
+        e2a.save()
+        
+        self.assertTrue(e1)
+        self.assertTrue(e2)
+        self.assertTrue(e2a)
+        self.assertTrue(e1.work)
+        self.assertTrue(e2.work)
+        self.assertEqual(models.Work.objects.count(), 2)
+ 
+        w1_id = w1.id
+        w2_id = w2.id
+        
+        # first try to merge work 1 into itself -- should not do anything
+        bookloader.merge_works(w1,w1)
+        self.assertEqual(models.Work.objects.count(), 2)
+        
+        # merge the second work into the first
+        bookloader.merge_works(e1.work, e2.work)
+        self.assertEqual(models.Work.objects.count(),1)
+        self.assertEqual(models.WasWork.objects.count(),1)
+        
+        # getting proper view?
+        anon_client = Client()
+        r = anon_client.get("/work/%s/" % w1_id)
+        self.assertEqual(r.status_code, 200)
+        r = anon_client.get("/work/%s/" % w2_id)
+        self.assertEqual(r.status_code, 200)        
+        
+        # try to do it twice -- nothing should happen
+        bookloader.merge_works(e1.work, e2a.work)
+        r = anon_client.get("/work/%s/" % w1_id)
+        self.assertEqual(r.status_code, 200)
+        r = anon_client.get("/work/%s/" % w2_id)
+        self.assertEqual(r.status_code, 200)               
+        
+
     def test_merge_works(self):
         # add two editions and see that there are two stub works
         e1 = bookloader.add_by_isbn('0465019358')