From b8fc5c9b7a6ee796d8f7629270bd7a19b3da26c1 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Mon, 23 Oct 2023 20:42:56 +1100 Subject: [PATCH 1/3] fix tests --- .../tests/models/test_bookwyrm_import_job.py | 64 +++++++++++-------- bookwyrm/tests/utils/test_tar.py | 7 +- 2 files changed, 40 insertions(+), 31 deletions(-) diff --git a/bookwyrm/tests/models/test_bookwyrm_import_job.py b/bookwyrm/tests/models/test_bookwyrm_import_job.py index 78a8ec160..249160481 100644 --- a/bookwyrm/tests/models/test_bookwyrm_import_job.py +++ b/bookwyrm/tests/models/test_bookwyrm_import_job.py @@ -62,13 +62,12 @@ class BookwyrmImport(TestCase): parent_work=self.work, ) - archive_file = pathlib.Path(__file__).parent.joinpath( + self.archive_file = pathlib.Path(__file__).parent.joinpath( "../data/bookwyrm_account_export.tar.gz" ) - self.tarfile = BookwyrmTarFile.open( - mode="r:gz", fileobj=open(archive_file, "rb") - ) - self.import_data = json.loads(self.tarfile.read("archive.json").decode("utf-8")) + with open(self.archive_file, "rb") as fileobj: + tarfile = BookwyrmTarFile.open(mode="r:gz", fileobj=fileobj) + self.import_data = json.loads(tarfile.read("archive.json").decode("utf-8")) def test_update_user_profile(self): """Test update the user's profile from import data""" @@ -77,19 +76,22 @@ class BookwyrmImport(TestCase): "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" ): - models.bookwyrm_import_job.update_user_profile( - self.local_user, self.tarfile, self.import_data.get("user") - ) - self.local_user.refresh_from_db() + with open(self.archive_file, "rb") as fileobj: + tarfile = BookwyrmTarFile.open(mode="r:gz", fileobj=fileobj) - self.assertEqual( - self.local_user.username, "mouse" - ) # username should not change - self.assertEqual(self.local_user.name, "Rat") - self.assertEqual( - self.local_user.summary, - "I love to make soup in Paris and eat pizza in New York", - ) + models.bookwyrm_import_job.update_user_profile( + self.local_user, tarfile, self.import_data.get("user") + ) + self.local_user.refresh_from_db() + + self.assertEqual( + self.local_user.username, "mouse" + ) # username should not change + self.assertEqual(self.local_user.name, "Rat") + self.assertEqual( + self.local_user.summary, + "I love to make soup in Paris and eat pizza in New York", + ) def test_update_user_settings(self): """Test updating the user's settings from import data""" @@ -248,14 +250,16 @@ class BookwyrmImport(TestCase): find or create the editions in the database and return a list of edition instances""" - self.assertEqual(models.Edition.objects.count(), 1) self.assertEqual(models.Edition.objects.count(), 1) - bookwyrm_import_job.get_or_create_edition( - self.import_data["books"][1], self.tarfile - ) # Sand Talk + with open(self.archive_file, "rb") as fileobj: + tarfile = BookwyrmTarFile.open(mode="r:gz", fileobj=fileobj) - self.assertEqual(models.Edition.objects.count(), 1) + bookwyrm_import_job.get_or_create_edition( + self.import_data["books"][1], tarfile + ) # Sand Talk + + self.assertEqual(models.Edition.objects.count(), 1) def test_get_or_create_edition_not_existing(self): """Test take a JSON string of books and editions, @@ -264,12 +268,16 @@ class BookwyrmImport(TestCase): self.assertEqual(models.Edition.objects.count(), 1) - bookwyrm_import_job.get_or_create_edition( - self.import_data["books"][0], self.tarfile - ) # Seeing like a state + with open(self.archive_file, "rb") as fileobj: + tarfile = BookwyrmTarFile.open(mode="r:gz", fileobj=fileobj) + bookwyrm_import_job.get_or_create_edition( + self.import_data["books"][0], tarfile + ) # Seeing like a state - self.assertTrue(models.Edition.objects.filter(isbn_13="9780300070163").exists()) - self.assertEqual(models.Edition.objects.count(), 2) + self.assertTrue( + models.Edition.objects.filter(isbn_13="9780300070163").exists() + ) + self.assertEqual(models.Edition.objects.count(), 2) def test_clean_values(self): """test clean values we don't want when creating new instances""" @@ -373,7 +381,7 @@ class BookwyrmImport(TestCase): self.assertEqual( models.Review.objects.filter(book=self.book).first().name, "great book" ) - self.assertEqual( + self.assertAlmostEqual( models.Review.objects.filter(book=self.book).first().rating, 5.00 ) diff --git a/bookwyrm/tests/utils/test_tar.py b/bookwyrm/tests/utils/test_tar.py index cb4e738d7..be5257542 100644 --- a/bookwyrm/tests/utils/test_tar.py +++ b/bookwyrm/tests/utils/test_tar.py @@ -1,5 +1,6 @@ -from bookwyrm.utils.tar import BookwyrmTarFile +import os import pytest +from bookwyrm.utils.tar import BookwyrmTarFile @pytest.fixture @@ -15,10 +16,10 @@ def write_tar(): archive_path = "/tmp/test.tar.gz" with open(archive_path, "wb") as archive_file: with BookwyrmTarFile.open(mode="w:gz", fileobj=archive_file) as tar: - return tar + yield tar os.remove(archive_path) def test_write_bytes(write_tar): - write_tar.write_bytes(b"ABCDEF", filename="example.txt") + write_tar.write_bytes(b"ABCDEF") From ddec2dbaa98b6f3a9a8fedeacefeb40508aa5ab2 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Mon, 23 Oct 2023 20:43:49 +1100 Subject: [PATCH 2/3] fix tar types notification docstring --- bookwyrm/models/notification.py | 2 +- bookwyrm/utils/tar.py | 12 +++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/bookwyrm/models/notification.py b/bookwyrm/models/notification.py index 98d20a3cb..d62043845 100644 --- a/bookwyrm/models/notification.py +++ b/bookwyrm/models/notification.py @@ -255,7 +255,7 @@ def notify_user_on_user_import_complete( def notify_user_on_user_export_complete( sender, instance, *args, update_fields=None, **kwargs ): - """we imported your user details! aren't you proud of us""" + """we exported your user details! aren't you proud of us""" update_fields = update_fields or [] if not instance.complete or "complete" not in update_fields: print("RETURNING", instance.status) diff --git a/bookwyrm/utils/tar.py b/bookwyrm/utils/tar.py index 6aec88b42..044a47404 100644 --- a/bookwyrm/utils/tar.py +++ b/bookwyrm/utils/tar.py @@ -1,7 +1,7 @@ """manage tar files for user exports""" import io import tarfile -from typing import Any +from typing import Any, Optional from uuid import uuid4 from django.core.files import File @@ -16,7 +16,9 @@ class BookwyrmTarFile(tarfile.TarFile): info.size = len(data) self.addfile(info, fileobj=buffer) - def add_image(self, image: Any, filename: str = None, directory: Any = "") -> None: + def add_image( + self, image: Any, filename: Optional[str] = None, directory: Any = "" + ) -> None: """ Add an image to the tar archive :param str filename: overrides the file name set by image @@ -35,12 +37,12 @@ class BookwyrmTarFile(tarfile.TarFile): def read(self, filename: str) -> Any: """read data from the tar""" - with self.extractfile(filename) as reader: + if reader := self.extractfile(filename): return reader.read() def write_image_to_file(self, filename: str, file_field: Any) -> None: """add an image to the tar""" extension = filename.rsplit(".")[-1] - with self.extractfile(filename) as reader: + if buf := self.extractfile(filename): filename = f"{str(uuid4())}.{extension}" - file_field.save(filename, File(reader)) + file_field.save(filename, File(buf)) From e29c93a1e90155bdc186c09502453d7b95f51240 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Mon, 23 Oct 2023 20:44:52 +1100 Subject: [PATCH 3/3] complete jobs more sensibly - fix tuple in tar export I accidentally broke by following pylint blindly - just use job.set_status to complete jobs since it does everything we need - fix/avoid Celery "not JSON deserializable" error by not saving whole job including user value --- bookwyrm/models/bookwyrm_export_job.py | 9 ++++----- bookwyrm/models/bookwyrm_import_job.py | 3 +-- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/bookwyrm/models/bookwyrm_export_job.py b/bookwyrm/models/bookwyrm_export_job.py index e4a6e314f..7c3d3ac2a 100644 --- a/bookwyrm/models/bookwyrm_export_job.py +++ b/bookwyrm/models/bookwyrm_export_job.py @@ -42,13 +42,12 @@ def start_export_task(**kwargs): job.export_data = ContentFile(b"", str(uuid4())) json_data = json_export(job.user) tar_export(json_data, job.user, job.export_data) + job.save(update_fields=["export_data"]) except Exception as err: # pylint: disable=broad-except logger.exception("User Export Job %s Failed with error: %s", job.id, err) job.set_status("failed") - job.set_status( - "complete" - ) # need to explicitly set this here to trigger notifications - job.save(update_fields=["export_data"]) + + job.set_status("complete") def tar_export(json_data: str, user, file): @@ -61,7 +60,7 @@ def tar_export(json_data: str, user, file): if getattr(user, "avatar", False): tar.add_image(user.avatar, filename="avatar") - editions = get_books_for_user(user) + editions, books = get_books_for_user(user) # pylint: disable=unused-argument for book in editions: if getattr(book, "cover", False): tar.add_image(book.cover) diff --git a/bookwyrm/models/bookwyrm_import_job.py b/bookwyrm/models/bookwyrm_import_job.py index 32c1a037a..16dad1bfc 100644 --- a/bookwyrm/models/bookwyrm_import_job.py +++ b/bookwyrm/models/bookwyrm_import_job.py @@ -61,8 +61,7 @@ def start_import_task(**kwargs): process_books(job, tar) - job.set_status("complete") # set here to trigger notifications - job.save() + job.set_status("complete") archive_file.close() except Exception as err: # pylint: disable=broad-except