Skip to content

filter books by scraper and issues#390

Open
elfkuzco wants to merge 2 commits into
mainfrom
filter-books-by-scraper-and-issues
Open

filter books by scraper and issues#390
elfkuzco wants to merge 2 commits into
mainfrom
filter-books-by-scraper-and-issues

Conversation

@elfkuzco

@elfkuzco elfkuzco commented Jul 1, 2026

Copy link
Copy Markdown
Contributor
Screenshot_20260701_210236

Changes

  • make param to filter books a pydantic model
  • filter books by issue or scraper name
  • add UI filters to filter books by issue and/or scraper name
  • make inbox book table to always display as card instead of normal table since it has so many columns

This closes #388

@elfkuzco elfkuzco self-assigned this Jul 1, 2026
@elfkuzco elfkuzco requested a review from benoit74 July 1, 2026 20:19
@elfkuzco

elfkuzco commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

@benoit74 , there are other places where the BookTable is used. For example in books view and in title detail view. Should we make those cards too? And should we extend the filters to teh normal books view?

@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.42857% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.84%. Comparing base (738bf66) to head (19f1aa1).
⚠️ Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
backend/src/cms_backend/db/books.py 54.54% 7 Missing and 8 partials ⚠️
...ms_backend/mill/mark_staging_books_for_deletion.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #390      +/-   ##
==========================================
+ Coverage   80.83%   80.84%   +0.01%     
==========================================
  Files          62       62              
  Lines        3454     3503      +49     
  Branches      377      385       +8     
==========================================
+ Hits         2792     2832      +40     
- Misses        547      554       +7     
- Partials      115      117       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@benoit74 benoit74 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you

  • Seeing it in action, I don't feel like displaying the whole scraper metadata and allowing to search inside it in free text is the good approach. I suggest we rather replace (in the UI) the "Scraper" filter with an "Offliner" filter which is a list of offliners as currently defined in Zimfarm (I suggest UI call Zimfarm API to retrieve list of offliners).
  • The UI should then not show the "Scraper" metadata in the card, but the "Offliner" (just matching the scraper metadata, and saying "Unknown" if there is no match)
  • The card width can then be made much smaller, with at least 3 or maybe even 4 cards side-by-side
  • The card UI should be polished: all "lines" should have the same height (except the list of issues where we want one issue per "line" + one line for the "view issues" button), data should not overflow on two lines but be wrapped with ellipsis, elements of "status" should be vertically aligned.

@elfkuzco

elfkuzco commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

I suggest we rather replace (in the UI) the "Scraper" filter with an "Offliner" filter which is a list of offliners as currently defined in Zimfarm (I suggest UI call Zimfarm API to retrieve list of offliners).

This will require zimfarm to whitelist cms UI due to CORS

@benoit74

benoit74 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

This will require zimfarm to whitelist cms UI due to CORS

OK, we will need to do this anyway for #333

@elfkuzco

elfkuzco commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Seeing it in action, I don't feel like displaying the whole scraper metadata and allowing to search inside it in free text is the good approach. I suggest we rather replace (in the UI) the "Scraper" filter with an "Offliner" filter which is a list of offliners as currently defined in Zimfarm (I suggest UI call Zimfarm API to retrieve list of offliners).

Should we also show "unknown" when user doesn't pass any offliner? Also, the book full details inherits this attribute, should it also show "Unknown" instead of the underlying "scraper"?

@elfkuzco

elfkuzco commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author
Screenshot_20260702_133818

@elfkuzco elfkuzco requested a review from benoit74 July 2, 2026 12:48
@benoit74

benoit74 commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Thank you.

  • I see that "Status" line is still higher than other lines. While I do understand "Issues" line must be de-facto higher when we have multiple issues, I would prefer we have the same height on all lines when there is no issue. Probably worth to set a min-height on lines so that "Status" line content "fits" inside.

  • I feel the "View Issues" button very confusing, it is weird that clicking anywhere on the card brings you to details but clicking on this specific area brings you to Issues. Please rather add two buttons at the bottom of the card, only to view details and one to view issues (not show when there is no issues), and make the card not clickable.

  • Flavour is the only field to not have a "dash" when there is no value ; this is weird, please put a "dash" when there is no flavour set

All that being said, looking back at Zimfarm files card, I feel like we should adopt a very similar design for consistency and because I like it:

  • no unneeded "field name" repeated on all cards when not needed (we probably never need it on CMS, see suggestions below)
  • no text on buttons, make icon self-explanatory + tooltip
image

This could even solve the issue of "Status line" height ... by not have a status line anymore, but just displaying the status since it is kinda obvious it is the status. Same for issues, the fact it is a badge in red make it quite clear it is an issue. Deletion date could be shown just like "date" but with an different and explicit icon. Offliner could also have its own badge with an explicit icon (everyone will know it is the offliner when value text will be correct anyway). And since buttons will have no text, we can add the "view" + "download" buttons directly on the card just like on ZF which is a big plus.

Should we also show "unknown" when user doesn't pass any offliner? Also, the book full details inherits this attribute, should it also show "Unknown" instead of the underlying "scraper"?

We should compute offliner based on scraper value + matching with the current list of known offliner. So "Unknown" should not be shown in your screenshot, we should see "mwoffliner" (and maybe "zimit", I don't recall ^^).

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.

Book inbox should display scraper and allow to filter by scraper and book issue type

2 participants