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

Add ability to search for books in a vl (vl:foo). #741

Merged
merged 3 commits into from Aug 20, 2017
Merged

Add ability to search for books in a vl (vl:foo). #741

merged 3 commits into from Aug 20, 2017

Conversation

cbhaley
Copy link
Contributor

@cbhaley cbhaley commented Aug 19, 2017

Also fix not recomputing the search cache when vls are edited.

There is one backwards incompatibility that I found. If a user has created a grouped search term with the name "vl" then the new vl search location will hide that grouped term.

@kovidgoyal
Copy link
Owner

What's the motivation for this?

@cbhaley
Copy link
Contributor Author

cbhaley commented Aug 19, 2017

Two things.

  1. I frequently get queries about the content server and CC, asking how someone can use the search expression attached to the username to select one or more VLs. One answer, use the searches from the VLs, is correct but unsatisfactory because it won't keep track of changes to the VLs. The other answer, convert the VL searches into saved searches and use those is equally unsatisfactory because many people have no idea what a saved search is. The same questions arise in the calibre forum: how to limit a user to a particular VL. The change makes doing that easy and intuitive.

  2. From time to time I see queries on mobileread asking how to see things like

  • what books are in two virtual libraries
  • what books are in a combined virtual library
    and the like. Again, the current answer is to tell them to use saved searches in the VLs and then compose these however they want. The same problem applies; either they don't know what saved searches are or they don't want to use them.

The change to the mixin fixed a bug. I found that if I changed a VL search, that change didn't get reflected in the results until I restarted calibre.

@kovidgoyal
Copy link
Owner

Sorry, I've injured my wrist so I have been mostly staying away from the keyboard. As a result I've skimmed over/skipped quite a few posts on MR in the last couple of weeks. So...

  1. Not sure what "selecting VLs based on search queries associated with a username" means. Is that something CC specific? Also isn't the way to limit a user to a VL, to just specify that VL in the server preferences? How does adding a vl prefix help with that?

  2. This makes sense to me.

I've already merged the change to the mixin, since that is obviosuly a bug.

@cbhaley
Copy link
Contributor Author

cbhaley commented Aug 19, 2017

Re selecting VL in the content server via username: no, this is a content server thing, not just a CC thing. I see the question a lot because of CC's content server connection. People want to see books from only one VL.

You said "Also isn't the way to limit a user to a VL, to just specify that VL in the server preferences?" How do you do that? The only way I see is to enter a search expression. That search expression can't (currently) be a VL name (AFAIK), but can refer to a saved search. Is there something I am not seeing?

Hope your wrist improves. Pain is not fun.

@kovidgoyal
Copy link
Owner

Oh sorry, I thought I had implemented a VL dropdown for filling in the search expression from current VL definitions, but I guess I was mis-remembering. OK, I'm ready to merge, but could you rebase and also add some documentation about the new prefix somewhere, probably in virtual_libraries.rst

Thanks, there's not much pain, but I was advised to rest the wrist, so I have been taking it easy.

@kovidgoyal
Copy link
Owner

Another thing that occurs to me is, since one of the purposes of the vl: prefix is to be used as a per user restriction, we need to handle the case of the restriction raising a ParseException gracefully (for example when a vl the restiction refers to is deleted).

@cbhaley
Copy link
Contributor Author

cbhaley commented Aug 19, 2017

Are you talking about in the content server? I agree that ParseException needs to be handled gracefully for all the ParseExceptions.

Do you want me to look into doing that? I can certainly ensure that nothing bad happens (probably nothing happens :) ), but I don't know how I can tell the user about the exception, especially in OPDS.

@kovidgoyal
Copy link
Owner

Essentially, look in srv/handler.py for all calls to self.restriction_for() whereever the restriction expression is used, if it raises a PArseException it should return the empty set for the allowed books.

@cbhaley
Copy link
Contributor Author

cbhaley commented Aug 19, 2017

I added some documentation.

I also added some code to (try to) handle ParseExceptions intelligently. The problem with always ignoring them is that the user will not understand why s/he is getting those results. What I did should produce an exception message on initial connect but not break the other places restriction_for is used.

@kovidgoyal
Copy link
Owner

Looks good, but I decided to implement the error reporting slightly differently, because the browser client supports offline mode, there is no way to guarantee which function will be called first in any given session. So instead, the javascript function used to populate the book list will report a ParseError in the restriction expression directly.

If you rebase one more time, I'll merge the changes for the vl: prefix.

@cbhaley
Copy link
Contributor Author

cbhaley commented Aug 20, 2017

I see what you did and it makes sense for the standard browser. Unfortunately it doesn't handle the OPDS case. What you committed makes OPDS return an empty book list with no error or indication what went wrong. I pushed some changes in the same spirit as yours to make OPDS (http:/foo.bar.com/opds) return a HTTP 500 error when it sees a ParseException. I didn't change the behavior of other OPDS endpoints because a) the user shouldn't get there, and b) why take the risk.

@kovidgoyal kovidgoyal merged commit 9dcc416 into kovidgoyal:master Aug 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants