Merge pull request #366 from mouse-reeve/tests

Fixes default field values in ActivityPub dataclasses

bookwyrm/activitypub/base_activity.py

@@ -74,7 +74,8 @@ class ActivityObject:
             try:
                 value = kwargs[field.name]
             except KeyError:
-                if field.default == MISSING:
+                if field.default == MISSING and \
+                        field.default_factory == MISSING:
                     raise ActivitySerializerError(\
                             'Missing required field: %s' % field.name)
                 value = field.default
@@ -143,6 +144,8 @@ class ActivityObject:
 
         # add images
         for (model_key, value) in image_fields.items():
+            if not value:
+                continue
             getattr(instance, model_key).save(*value, save=True)
 
         # add one to many fields
@@ -188,6 +191,8 @@ def resolve_foreign_key(model, remote_id):
 
 def tag_formatter(tags, tag_type):
     ''' helper function to extract foreign keys from tag activity json '''
+    if not isinstance(tags, list):
+        return []
     items = []
     types = {
         'Book': models.Book,
@@ -207,9 +212,9 @@ def tag_formatter(tags, tag_type):
 
 def image_formatter(image_json):
     ''' helper function to load images and format them for a model '''
-    url = image_json.get('url')
+    if not image_json or not hasattr(image_json, 'url'):
-    if not url:
         return None
+    url = image_json.get('url')
 
     try:
         response = requests.get(url)
@@ -230,6 +235,8 @@ def image_attachments_formatter(images_json):
         caption = image.get('name')
         attachment = models.Attachment(caption=caption)
         image_field = image_formatter(image)
+        if not image_field:
+            continue
         attachment.image.save(*image_field, save=False)
         attachments.append(attachment)
     return attachments

bookwyrm/activitypub/book.py

@@ -25,7 +25,7 @@ class Book(ActivityObject):
     librarything_key: str
     goodreads_key: str
 
-    attachment: List[Image] = field(default=lambda: [])
+    attachment: List[Image] = field(default_factory=lambda: [])
     type: str = 'Book'
 
 

bookwyrm/activitypub/note.py

@@ -24,8 +24,8 @@ class Note(ActivityObject):
     cc: List[str]
     content: str
     replies: Dict
-    tag: List[Link] = field(default=lambda: [])
+    tag: List[Link] = field(default_factory=lambda: [])
-    attachment: List[Image] = field(default=lambda: [])
+    attachment: List[Image] = field(default_factory=lambda: [])
     sensitive: bool = False
     type: str = 'Note'
 

bookwyrm/activitypub/person.py

@@ -15,7 +15,7 @@ class Person(ActivityObject):
     summary: str
     publicKey: PublicKey
     endpoints: Dict
-    icon: Image = field(default=lambda: {})
+    icon: Image = field(default_factory=lambda: {})
     bookwyrmUser: bool = False
     manuallyApprovesFollowers: str = False
     discoverable: str = True

bookwyrm/models/base_model.py

@@ -59,24 +59,31 @@ class ActivitypubMixin:
     def to_activity(self, pure=False):
         ''' convert from a model to an activity '''
         if pure:
+            # works around bookwyrm-specific fields for vanilla AP services
             mappings = self.pure_activity_mappings
         else:
+            # may include custom fields that bookwyrm instances will understand
             mappings = self.activity_mappings
 
         fields = {}
         for mapping in mappings:
             if not hasattr(self, mapping.model_key) or not mapping.activity_key:
+                # this field on the model isn't serialized
                 continue
             value = getattr(self, mapping.model_key)
             if hasattr(value, 'remote_id'):
+                # this is probably a foreign key field, which we want to
+                # serialize as just the remote_id url reference
                 value = value.remote_id
-            if isinstance(value, datetime):
+            elif isinstance(value, datetime):
                 value = value.isoformat()
+
+            # run the custom formatter function set in the model
             result = mapping.activity_formatter(value)
             if mapping.activity_key in fields and \
                     isinstance(fields[mapping.activity_key], list):
-                # there are two database fields that map to the same AP list
+                # there can be two database fields that map to the same AP list
-                # this happens in status, which combines user and book tags
+                # this happens in status tags, which combines user and book tags
                 fields[mapping.activity_key] += result
             else:
                 fields[mapping.activity_key] = result
@@ -265,7 +272,7 @@ def tag_formatter(items, name_field, activity_type):
 
 def image_formatter(image, default_path=None):
     ''' convert images into activitypub json '''
-    if image:
+    if image and hasattr(image, 'url'):
         url = image.url
     elif default_path:
         url = default_path

bookwyrm/tests/connectors/test_self_connector.py

@@ -57,10 +57,9 @@ class SelfConnector(TestCase):
 
     def test_search_rank(self):
         results = self.connector.search('Anonymous')
-        self.assertEqual(len(results), 3)
+        self.assertEqual(len(results), 2)
-        self.assertEqual(results[0].title, 'Edition of Example Work')
+        self.assertEqual(results[0].title, 'More Editions')
-        self.assertEqual(results[1].title, 'More Editions')
+        self.assertEqual(results[1].title, 'Edition of Example Work')
-        self.assertEqual(results[2].title, 'Another Edition')
 
 
     def test_search_default_filter(self):

bookwyrm/tests/test_remote_user.py

@@ -21,50 +21,7 @@ class RemoteUser(TestCase):
         self.user_data = json.loads(datafile.read_bytes())
 
 
-
     def test_get_remote_user(self):
         actor = 'https://example.com/users/rat'
         user = remote_user.get_or_create_remote_user(actor)
         self.assertEqual(user, self.remote_user)
-
-
-    def test_create_remote_user(self):
-        user = remote_user.create_remote_user(self.user_data)
-        self.assertFalse(user.local)
-        self.assertEqual(user.remote_id, 'https://example.com/user/mouse')
-        self.assertEqual(user.username, 'mouse@example.com')
-        self.assertEqual(user.name, 'MOUSE?? MOUSE!!')
-        self.assertEqual(user.inbox, 'https://example.com/user/mouse/inbox')
-        self.assertEqual(user.outbox, 'https://example.com/user/mouse/outbox')
-        self.assertEqual(user.shared_inbox, 'https://example.com/inbox')
-        self.assertEqual(
-            user.public_key,
-            self.user_data['publicKey']['publicKeyPem']
-        )
-        self.assertEqual(user.local, False)
-        self.assertEqual(user.bookwyrm_user, True)
-        self.assertEqual(user.manually_approves_followers, False)
-
-
-    def test_create_remote_user_missing_inbox(self):
-        del self.user_data['inbox']
-        self.assertRaises(
-            TypeError,
-            remote_user.create_remote_user,
-            self.user_data
-        )
-
-
-    def test_create_remote_user_missing_outbox(self):
-        del self.user_data['outbox']
-        self.assertRaises(
-            TypeError,
-            remote_user.create_remote_user,
-            self.user_data
-        )
-
-
-    def test_create_remote_user_default_fields(self):
-        del self.user_data['manuallyApprovesFollowers']
-        user = remote_user.create_remote_user(self.user_data)
-        self.assertEqual(user.manually_approves_followers, False)