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

Let a user search books within their shelves #3118

Conversation

rosschapman
Copy link
Contributor

@rosschapman rosschapman commented Nov 17, 2023

Description

Search within your own books! 🔍 👤 📚
For issue background and details: #2079

Goals

  • Users should be able to search their own shelves, including All Books
  • Users should expect that this search control behaves similar to the global book search

What's changed?

  1. Adds shelf search query handling to Shelf view
    In preparation for this work, the assumption was that we might reuse the /search route and update the Search view with the new code. But it quickly became obvious that the Shelf view was a better place to add search behavior for the Your Books pages.
  2. Extends search()
    To leverage existing system search behavior to search within a user's shelves, the search() function parameters have been extended with a kwarg for books of type QuerySet. Now book search can be invoked on a preloaded query or not.
  3. Uses filtering components for shelf book search*
    To differentiate from the global, federated book search and be consistent with the filtering controls that exist for other list interfaces, I've reused the filter_panels components

Tests

View cases are being worked out over at: rosschapman#1

Screenshots

image

image

image


PRD and implementation notes (for posterity)

- 🙋 Questions: 
	- Do we want to allow users to search across
		1. All their books
		4. All their books *within* a shelf identifier (eg: just *Read* books)
		5. Both
		- Implementing # 1 is probably a good MVP
- Presumably we don't have to touch the core book search `search` method ([`bookwyrm/book_search.py`](https://github.com/bookwyrm-social/bookwyrm/blob/main/bookwyrm/book_search.py#L40))
	- 🙋 Questions:
		- What is a `search identifier` and why do we search on that first?
		```python
		# first, try searching unique identifiers
	    # unique identifiers never have spaces, title/author usually do
    	if not " " in query:
        	results = search_identifiers(query, *filters, return_first=return_first)
		```

### Speculative task list (large, can be broken up)
- [ ] [BE] Create receiver (view class method) for new user books search request
	- **Option 1:** Modify the existing Search View [`views/search#book_search`](https://github.com/bookwyrm-social/bookwyrm/blob/main/bookwyrm/views/search.py) with additional logic to restrict the search query to the current user's books
	- **Option 2:** Create a separate view method - named something like `def book_search_user` or `def book_search_own` etc... -- which might reuse some of the existing view logic but will give 
	- My preference is Option # 2 to start. We can always consolidate after if collaborators determine the branching logic doesn't create more headaches than it's worth. 
		- For example, we don't need the `remote_search` capability. Perhaps we forego ISBN check?
- [ ] [FE] Add search control to User's Shelf page ([`bookwyrm/templates/shelf/shelf.html`](https://github.com/bookwyrm-social/bookwyrm/blob/main/bookwyrm/templates/shelf/shelf.html))
	- **Option 1:** Duplicate the search control code from the chrome header ([`bookwyrm/templates/layout.html`]([bookwyrm/templates/layout.html](https://github.com/bookwyrm-social/bookwyrm/blob/main/)))
	- **Option 2:** Create a reusable Search control component
		- My preference is option # 1. We can always consolidate after if collaborators determine the branching logic doesn't create more headaches than it's worth>)

@hughrun
Copy link
Contributor

hughrun commented Nov 17, 2023

Great stuff @rosschapman awesome to have someone working on this.

What is a search identifier and why do we search on that first?

The search_identifiers function searches for anything that might be an ISBN or a deduplication_field on an Edition.

So what is a deduplication_field? If you check models/book.py you'll see that pretty much everything in BookDataModel, plus a few other fields in models that inherit it, are deduplication_fields. These are standard identifiers like ISBNs, librarything_key, wikidata identifiers, and so on. What they have in common is that they generally don't duplicate - i.e. a librarything_key will only ever refer to one particular edition of a particular book. So if the term someone searches by is a librarything_key for example, if you get a match on that you pretty much know that's the book they meant. This is not the case for an author or a title, where many books can have the same title, and multiple authors can have the same name. So we look for a match on a deduplication_field first and if we find one we just return that because it's very likely to be the match, but if we don't find one, we keep searching knowing we'll return stuff that may or may not be what they were looking for.

Hopefully that makes sense.

@mouse-reeve
Copy link
Member

(I just went in and added spaces in places where you mentioned an option number and GitHub thought it was a reference to an issue, it had me baffled for a second 😂)

@rosschapman
Copy link
Contributor Author

(@mouse-reeve 🙈 😅 githuh markdown fail)

@rosschapman
Copy link
Contributor Author

@hughrun Thank you, that's the context I needed to understand that part. 💪

@rosschapman rosschapman marked this pull request as ready for review December 6, 2023 04:18
@rosschapman rosschapman changed the title [WIP] Let a user search books within their shelves Let a user search books within their shelves Dec 6, 2023
@rosschapman
Copy link
Contributor Author

@hughrun @mouse-reeve this is ready for an initial look, thanks! Going to think through some baseline test coverage in the meantime.

Copy link
Contributor

@hughrun hughrun left a comment

Choose a reason for hiding this comment

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

This is really nice!

Works well, just a small change needed, and a question about terminology, as noted.

bookwyrm/templates/shelf/shelf.html Outdated Show resolved Hide resolved
bookwyrm/templates/shelf/search_filter_field.html Outdated Show resolved Hide resolved
@rosschapman
Copy link
Contributor Author

rosschapman commented Dec 14, 2023

@hughrun I tried a different approach of showing a hint when the results are empty. See: 4a4046a. (Also renamed things from "search" to "filter" so it's more obvious to developers looking at this code that we are filtering -- as in, search within my books -- rather than searching -- search for a book.

Here's a screenshot of new hint:
image

And this should play well if we show the other pages text:
image

@hughrun
Copy link
Contributor

hughrun commented Dec 14, 2023

@hughrun I tried a different approach of showing a hint when the results are empty.

This is a much better idea than my suggestion 😄 Looks great!

My only suggestion at this point is that in some ways you now have the opposite problem. Previously there was no way to clear the filter if the results were empty. Now it tells the user two opposing things: that we couldn't find any books using their filter, and that the filter has been applied - but then we're showing them an unfiltered list (i.e. the filter has NOT been applied, which is deliberate).

I can see that this may confuse some users. Is there a way to do this where the filters/search term isn't passed back through to the view if the search set is empty, so we can say "we couldn't find anything" and automatically clear the filter?

@rosschapman
Copy link
Contributor Author

@hughrun for some reason it made sense in my head to still show all books without a match. But yeah, with only the placement of a small hint above it seems confusing. What about my latest commit which updates the template to show the not found message in the same place as the books list, while also keeps the Filters panel and Clear Filters option in view:

image

@rosschapman
Copy link
Contributor Author

Btw, it seems the template logic for setting the panel to "open" is a little wonky since it depends a filters_applied context variable being set. But if I set this variable it actually doubles up the Filters Applied token on the page. I was thinking about keeping the panel open in the case of no matches found.

@hughrun
Copy link
Contributor

hughrun commented Dec 14, 2023

I was thinking about keeping the panel open in the case of no matches found.

That would be nice, but if it's too hard I don't think it matters too much. Your latest screenshot looks good, let me know when you're happy to call it "ready" and I'll take a proper final look later.

@rosschapman
Copy link
Contributor Author

rosschapman commented Dec 14, 2023

That would be nice, but if it's too hard I don't think it matters too much.

filters_applied is only used for the Feed view (AFAICT) but I wonder if it was a bit of a hack to hide the "Clear Filters" button. Anywho, I don't think it would be that hard to suss out and fix the logic to work more generically for different kinds of filter fields. But out of scope of this PR 😅

Copy link
Contributor

@hughrun hughrun left a comment

Choose a reason for hiding this comment

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

Looks great other than a teeny change as per my comment.

bookwyrm/views/shelf/shelf.py Show resolved Hide resolved
Copy link
Member

@mouse-reeve mouse-reeve left a comment

Choose a reason for hiding this comment

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

Amazing! Thank you, looks good to me

@mouse-reeve mouse-reeve merged commit 9c3e638 into bookwyrm-social:main Jan 3, 2024
10 checks passed
@rosschapman rosschapman deleted the let-a-user-search-within-their-books branch February 22, 2024 03:11
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.

None yet

3 participants