Conversation
update templates add migration
also adds tests
- match to existing series using a search vector instead of exact matching - add series listing to author pages - clean up a few other details
|
hmm. Not sure why this |
I didn't manage to reproduce the failure locally either. |
ilkka-ollakka
left a comment
There was a problem hiding this comment.
I think this looks good. The failing test is something that could be tried to reproduce but shouldn't block merging in my opinion.
|
I can replicate this test failure locally if I run |
Resolve conflicts.
Q object query was wrong. Also fixed an except block I messed up when merging.
|
I have some small tweaks, but I ran into two bugs that weren't obvious to me: No. 1: If I add a new series with the same name as an existing series, I get this error: Which traces to: No. 2: If I try to delete a series using the checkbox on the edit book page, I get this error: Which traces to: |
mouse-reeve
left a comment
There was a problem hiding this comment.
I submitted fixes for all these comments on your repo, hopefully that helps??? But the two errors I found unfortunately remain
* Adds migration files needed for merge * Sorts series by number Also fixes a blocktrans error * Small wording and formatting tweaks to seris view
dato
left a comment
There was a problem hiding this comment.
I took a look at this PR, I have some minor comments and questions (all suggestions are optional).
Thanks!!
- turns out we need 'user' on Series as well - fix reverse values for activitypub
- fix upgrade_series command and add test - make series a proper ordered collection - fix various test problems - prevent duplicate seriesbooks - fix problems in edit_book template due to series management - fix issues with series pages
mouse-reeve
left a comment
There was a problem hiding this comment.
I'm sorry to be the bearer of this news, but now when I import a book without a series from Inventaire, it creates a blank string series that has a url linked from the book page, but the link leads to a 404
|
hmm, that seems suboptimal! I must have broken the logic somewhere. |
- fixes issue where imported books from Inventaire created series with no name - uses django's get_or_create() for simpler code - rationalises migrations - adds tests
Description
This PR moves book series to a pair of new models:
SeriesandSeriesBook. This will allow more consistency around series, and for books to be matched to series regardless of whether they share an author. Basically it makes series "things, not strings".A
Serieshas basicBookDatato better enable us to de-duplicate them, although only series imported from Inventaire are likely to have anything beyond a name. ASeriesBookconnects a Series to aBook, including aseries_number: it's the instantiation of a Book being part of a series.When saving a book, users will be prompted to confirm whether it is part of an existing series, in the same way we check for existing authors.
All importers will now convert series information to a
SeriesBookandSerieswhere possible. If there is a matching series but no other book in the series with a matching author, the series information is left on the incoming book and users will be prompted to confirm the series information manually on the book view page.Series can be edited, including updating the
seriesnumberon one or moreseriesbooks- users need to have permission to edit books to do this.Creating a data migration proved to be challenging, so I have instead created an admin command to convert "legacy" series to Series and SeriesBooks where possible. This will need to be run by admins manually.
I have tried to make this backwards-compatible but it's entirely possibly that I've missed something.
I also added an atomic block to the user export model which is entirely unrelated except that I made a mistake when I was testing, and without this block I got a bunch of obscure errors.
This was a bit of a beast to put together so there may well be parts that could be more efficient or that I've missed - happy for any feedback!
What type of Pull Request is this?
Does this PR change settings or dependencies, or break something?
Details of breaking or configuration changes (if any of above checked)
Documentation
Tests