csv import and export fixes

[hughrun]

Adds shelved and published dates for books and their imported reviews. Provides option to create new (custom) shelves when importing books.

fixes #3004
fixes #2411

@bookwyrm-social/code-review I'm looking for some clues as to why my tests fail when run sequentially but pass when run individually.

[mouse-reeve]

I'm not sure how to handle the merge conflicts on this one

[mouse-reeve]

I wasn't sure how to resolve the merge conflicts on this

[hughrun]

yeah I got distracted with other things, I need to come back to this.

[hughrun]

There's something wrong with my tests here (test_create_new_shelf and test_handle_imported_book_review) - they pass when run in isolation but not when running the whole TestCase. Any ideas @bookwyrm-social/code-review ?

[dato]

Hello @hughrun!

I took a look at the failures, here's the root cause (fwiw took a while to figure out!):

The __init__ method of BookwyrmBooksImporter is used to extend the row_mappings_guesses attribute:

bookwyrm/bookwyrm/importers/bookwyrm_import.py

Lines 38 to 40 in 06d6360

	def __init__(self, *args: Any, **kwargs: Any):
	self.row_mappings_guesses.append(("shelf_name", ["shelf_name"]))
	super().__init__(*args, **kwargs)

But row_mappings_guesses is a class level attribute, not instance level; so what happens is that the pair ("shelf_name", ["shelf_name"]) is appended once per created object.

The tests fail when such __init__ runs more than once (more than one importer created), because the duplication results in shelf_name=None in the import item.

To fix the issue, you can remove the __init__ method in favor of, for example:

class BookwyrmBooksImporter(Importer):
    # ...
    row_mappings_guesses = Importer.row_mappings_guesses + [
        ("shelf_name", ["shelf_name"]),
    ]

to avoid the bogus duplication.

[hughrun]

Ok a couple of things here:

1. I amended the test for retry jobs in bookwyrm/tests/importers/test_importer.py to order the jobs, because they were running in the reverse order when running multiple tests (but not a single test). My assumption is that this happened because pylint runs tests in parallel, and that ultimately it doesn't matter which order they run in, but it may have been indicating an actual problem - so some eyes on that would be helpful.
2. All tests pass locally, but there seems to be some kind of issue with the GitHub runners. Not sure what I can do about that.

@dato thanks for your advice.

[mouse-reeve]

I think this just needs a merge migration in order to be ready to go

[hughrun]

Ok there seems to be a conflict in the tests (causing some imports to finish out of order) because I'm using the same books as in generic.csv. Presumably this is because tests run in parallel? I'll adjust the test data to fix this.

[hughrun]

@bookwyrm-social/code-review I think this is ready.

[mouse-reeve]

Worked neatly for me! Thank you so much

[eldang]

Thank you! I'm excited for when this makes out to instances in the wild.

[mouse-reeve]

It seems like since merging this tests have been failing on new pull requests:

FAILED bookwyrm/tests/importers/test_importer.py::GenericImporter::test_create_job - AssertionError: 1 != 0

[hughrun]

ugggghhhh that's the error I was getting originally, I thought I'd fixed it by not using the same books to test importing.

@mouse-reeve from my own troubleshooting on this it seems that the tests sometimes return books in a slightly different order, which is what leads to this assertion error.

All tests pass individually but not together, so there's something I'm not understanding that leads to this difference: perhaps tests running concurrently leading to celery tasks returning in different orders?

[mouse-reeve]

Ah how frustrating! I tried re-writing that test to ignore the order of the items: #3419. It's hacky, but if it works...?