Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Kobo] Add Shelf/Collection support. #1266

Merged
merged 4 commits into from
Apr 26, 2020

Conversation

shavitmichael
Copy link
Contributor

Implements the v1/library/tags set of endpoints to sync CalibreWeb
shelves with the Kobo device.

Drive-by: Refactors shelf.py to consolidate how user permissions are checked.
Drive-by: Fix issue with the sync endpoint that arrises when a book is
hard-deleted.

Tested: I've been using this for the past week without noticable issues, although I'll admit I don't use Shelves as much now that Kobo has added the Series tab.

Implements the v1/library/tags set of endpoints to sync CalibreWeb
shelves with the Kobo device.
Drive-by: Refactors shelf.py to consolidate how user permissions are checked.
Drive-by: Fix issue with the sync endpoint that arrises when a book is
hard-deleted.
@shavitmichael
Copy link
Contributor Author

Bump on this PR?
This is the one remaining large features that I intend to implement.

@OzzieIsaacs
Copy link
Collaborator

I haven't forgotten you. I'm currently working on the LDAP integration, wrote some test, and I'm almost finished, I hope I can start working on this one end of the week. But it will take some time to merge. I'll write some tests(https://github.com/OzzieIsaacs/calibre-web-test), to ensure everything works well and I will start a little documentation in the wiki.

@shavitmichael
Copy link
Contributor Author

No worries. I actually just added a first revision for some documentation to the wiki a few hours ago :)

cps/kobo.py Outdated Show resolved Hide resolved
cps/shelf.py Outdated Show resolved Hide resolved
cps/kobo.py Outdated
log.debug("Received malformed v1/library/tags request.")
abort(400, description="Malformed tags POST request. Data is missing 'Name' or 'Items' field")

shelf = ub.session.query(ub.Shelf).filter(and_(ub.Shelf.name) == name, ub.Shelf.user_id ==
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big little problem, the name is not/no longer unique per user:
grafik
Every user can generate a public and private shelf with the same name, so we should check it during loading shelf name. I think we should prefer the private shelf over the public one with the same name. ( We are having 2 cases to consider

  1. name is not valid or not belonging to user
  2. name is unique private or unique public shelf owned by user
  3. name is used for private and public shelf of user
    This problem occurs for ever shelf request with filtering from name

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another idea would be: In the UI to the public shelfs the text 'public' is appended, so maybe doing this here would also be possible. But this text is translatable, so it depends on the user interface language.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be misunderstanding, but I don't think this is an issue for us.

We only use the shelf name during shelf creation. Other API calls (such as adding/removing books from a shelf, or deleting a shelf) rely on the ub.Shelf.uuid field to identify the shelf they operate on.
The only reason we check the shelf name here is to prevent the user from creating multiple shelves with the same name. However if shelves with the same name do exist, that won't affect the Kobo sync protocol in any way.

@OzzieIsaacs
Copy link
Collaborator

I'm currently working on the PR (as you might have seen):

  • Question regarding the missing feature of multiple authors: If there would be more than one author the filed would return a list of dicts similar to the DownloadUrls list, right?

  • Regarding "changing the pubdate also to a timestamp". From my point of view, it would be okay, I could modifiy the other parts in the code to handle the condequences, or is there a big issue you already know about?

@OzzieIsaacs
Copy link
Collaborator

OzzieIsaacs commented Apr 19, 2020

Start with fresh installation (means app.db is deleted before calibre-web starts and is freshly build).
Activate upload feature in admin -> basic configuration -> feature configuration -> enable upload
Upload one book (in my case a epub file)
Edit the recently uploaded book
Push on delete button, you get this traceback (the line numbers may differ, as I did some code cosmetics):

 return func(*args, **kwargs)
File "/home/user/Entwicklung/calibre-web/cps/editbooks.py", line 182, in delete_book
ub.session.query(ub.ArchivedBook).filter(ub.ReadBook.book_id == book_id).delete()
File "/home/user/.local/lib/python2.7/site-packages/sqlalchemy/orm/query.py", line 3689, in delete
delete_op.exec_()
File "/home/user/.local/lib/python2.7/site-packages/sqlalchemy/orm/persistence.py", line 1672, in exec_
self._do_pre_synchronize()
File "/home/user/.local/lib/python2.7/site-packages/sqlalchemy/orm/persistence.py", line 1751, in _do_pre_synchronize
"synchronize_session parameter." % err
InvalidRequestError: Could not evaluate current criteria in Python: "Can't evaluate criteria against alternate class <class 'cps.ub.ReadBook'>". Specify 'fetch' or False for the synchronize_session parameter.

The archived books table looks like this:
grafik

new_tags_last_modified = sync_token.tags_last_modified

for shelf in ub.session.query(ub.ShelfArchive).filter(func.datetime(ub.ShelfArchive.last_modified) > sync_token.tags_last_modified, ub.ShelfArchive.user_id == current_user.id):
new_tags_last_modified = max(shelf.last_modified, new_tags_last_modified)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't fully understood what we are doing there.
I did the following: Generate shelf on UI (private one), sync it to kobo (New Tag is synced), add book to shelf, try to sync it again.
Expected result: Changed tag is synced, but I get empty response.
Upon looking into ShelfArchive table I find this:
grafik
User ID is identical to the uuid but it's compared to the internal Calibre-Web user iD, which is a number from 1 to x, so comparsion fails, and the book in shelf isn't synced.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This particular section is propagating deletions of shelves from CalibreWeb to the device. And indeed it looks like there's a bug in the delete_shelf_helper() I added to shelf.py; specifically line 286

    ub.session.add(ub.ShelfArchive(uuid = cur_shelf.uuid, user_id = cur_shelf.uuid))

With this bug, deleting a shelf from CalibreWeb and syncing the device doesn't remove the shelf form the device.
With 9296d35 , deleting a shelf from CalibreWeb will create an entry in the ShelfArchive table with the correct user_id field. Deleting a shelf form CalibreWeb also removes it from the device after syncing.

@shavitmichael
Copy link
Contributor Author

Wow lot's of great comments, thanks for taking a look!

  • Question regarding the missing feature of multiple authors: If there would be more than one author the filed would return a list of dicts similar to the DownloadUrls list, right?

Here's an example from the official Kobo store:

"BookMetadata" : {
  "ContributorRoles": [
    {
      "Name": "Garth Nix"
    },
    {
      "Name": "Leo and Diane Dillon"
    }],
  "Contributors": [ "Garth Nix", "Leo and Diane Dillon"],

Both authors show up under the Book tittle on my device, and both authors have an entry on my device's Author tab.

  • Regarding "changing the pubdate also to a timestamp". From my point of view, it would be okay, I could modifiy the other parts in the code to handle the condequences, or is there a big issue you already know about?

I don't think there should be any issues, the usages are pretty spread out throughout the code-base so it's some work to update everything.

@shavitmichael
Copy link
Contributor Author

Start with fresh installation (means app.db is deleted before calibre-web starts and is freshly build).
Activate upload feature in admin -> basic configuration -> feature configuration -> enable upload
Upload one book (in my case a epub file)
Edit the recently uploaded book
Push on delete button, you get this traceback (the line numbers may differ, as I did some code cosmetics):

 return func(*args, **kwargs)
File "/home/user/Entwicklung/calibre-web/cps/editbooks.py", line 182, in delete_book
ub.session.query(ub.ArchivedBook).filter(ub.ReadBook.book_id == book_id).delete()
File "/home/user/.local/lib/python2.7/site-packages/sqlalchemy/orm/query.py", line 3689, in delete
delete_op.exec_()
File "/home/user/.local/lib/python2.7/site-packages/sqlalchemy/orm/persistence.py", line 1672, in exec_
self._do_pre_synchronize()
File "/home/user/.local/lib/python2.7/site-packages/sqlalchemy/orm/persistence.py", line 1751, in _do_pre_synchronize
"synchronize_session parameter." % err
InvalidRequestError: Could not evaluate current criteria in Python: "Can't evaluate criteria against alternate class <class 'cps.ub.ReadBook'>". Specify 'fetch' or False for the synchronize_session parameter.

The archived books table looks like this:
grafik

Good catch! Fixed in 742ec2b .

@@ -326,12 +326,12 @@ def feed_shelfindex():
def feed_shelf(book_id):
off = request.args.get("offset") or 0
if current_user.is_anonymous:
shelf = ub.session.query(ub.Shelf).filter(ub.Shelf.is_public == 1, ub.Shelf.id == book_id).first()
shelf = ub.session.query(ub.Shelf).filter(ub.Shelf.is_public == 1, ub.Shelf.id == book_id, not ub.Shelf.deleted).first()
else:
Copy link
Collaborator

@OzzieIsaacs OzzieIsaacs Apr 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We almost got it:
grafik
But there is one sideeffect: This line and line 334 causes a crash upon access. Because there is no column deleted in Shelf. I currently can't oversee what the intention of the change was. I would delete it? I don't need an updated PR, just short feedback.

@OzzieIsaacs
Copy link
Collaborator

During testing found some more or less big problems and fixed them by myself:
Creating a tag via Kobo interfaces failed, because uuid() doesn't return a string
Updating Shelfs from the UI didn't trigger a sync, because the last_modified time wasn't updated.
I improved error handling to catch empty post requests missing elements in every request and NaN's in the reading status
I removed the 404 errorhandler, because it hijacked the general 404 handler of the project and so every invalid request would have proxied to kobo, or would have been tranfered to a empty "Code 200 {}" request
Empty shelf names are prevented to create over the sync interface

@OzzieIsaacs OzzieIsaacs merged commit e29f17a into janeczku:Develop Apr 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants