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

EKIRJASTO-88 Add selected books #187

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

natlibfi-burger
Copy link
Contributor

@natlibfi-burger natlibfi-burger commented Feb 6, 2025

What's this do?
This code adds the possibility for a user to add and remove books from a list of favorites. to both single book view and list views a button is added from which the adding and removing a book from favorites. Successfully adding a book creates a toast telling the user about successful adding or removal of a book from favorites.

The favorites are shown on their own tab, Favorites, while loans and reserves are moved under one tab, My books. In the future, The favorites page can be expanded upon when more lists are added. The loans and reserves in the same view are differentiated with the same kind of tab than audiobook-ebook-all in the catalog. All books have also a new field called selected. This information is used to show different buttons based on if book is selected as a favorite or not.

As opposed to the old solution where we get the data only from the loans feed, we now have another feed that carries the information of selected books. These feed entries are combined and stored to the database and register. It is planned that all other information we need to add (like information if a book is read, or is graded) is added to the selected feed.

Handling the adding and removing of favorites is handled similarly to the handling of the loan limit reached -popup. Two book statuses have been added, selected and unselected, that trigger the wanted handling for both the behaviours, respectively. The book does not spend a long time in this state, instead, the old state is reset when the book information has been updated to the database and the register, as well as the popup has been shown. To note: This can cause possible audio descriptors to call out incorrect information, as the status is reset.

The code also handles 401 refreshes, both on select and unselect. If a user is not logged in and they attempt to select/unselect a book, it shows the login screen.

Old translations need to be adjusted. Lainat -> Omat kirjat (ex).

Why are we doing this? (w/ JIRA link if applicable)
https://jira.it.helsinki.fi/browse/EKIRJASTO-88

How should this be tested? / Do these changes have associated tests?
The code can be tested by logging in and going to catalog, and adding books to the list. There should be a popup informing the user of what book has been added or removed. The added books should be seen on their own tab "Favorites" tab on the bottom tab bar. Removing books from the list should refresh the list, so that the information remains fresh. If not logged in, the My books tab should show tabs even when not logged in, and there should be a text suggesting logging in. A new text is also added to favorites page. When logged in and there is no favorites, there should be a text informing how to add books from catalog into the list.

If logged out and trying to select a book, the login page should open.

The books should also be able to be added from the single book view, with a toast popping up there as well.

Refresh can be tested by logging in and waiting 10 min, and then trying to select or unselect a book.

Dependencies for merging? Releasing to production?
Should not be released before also done in IOS.

Does this require updates to old Transifex strings? Have the translators been informed?
In its current form, this does require updates to old strings. There is also new strings.

  • Have informed translators

natlibfi-burger and others added 18 commits January 3, 2025 17:11
Added a pickup of the possibly excisting selected Datetime into the OBDS
feed and parser, so it can be used to change what user sees. Added
handling for the selectedURI available in AuthenticationDocument.
Current returns the loans.
Added test button to load selected from settings page.
Added the select and unselect functions to the controller.
Added an icon that changes depending on if the user has the book
selected or not to the catalog views.

Needs to be added still:
Button for single book view
401 handling
Move seeing selected books to a different place
Add little toast to tell that a book has been added or deleted
Fix some error handling

Ref: EKIR-88
Added the same button into the single page view and added the
functionality.
Fixed both the select and unselect tasks to handle the database and book
register correctly, as well as added comments to the code.
Added a toast that informs user about selecting and unselecting a book
being successful. Added functions that handle the toasts based on book
status.
Added functions to reset a book status after the toast has been showed.
Added accessibility strings and descriptions, though unsure if they are
used. Also added conditions to when a book is shown as revokes in the
accessibility section.

TODO:
- Change the location of select button in app + rename
- Reload selected on remove from the list
- Making into local feed
- Handling of 401
- Show selected only in selected and book info page
Moved the selected feed to now be shown in the same bottom tab as loans.
Removing a loan removed the entry from database and register, so added a
selected check, so the book is only removed if it's also not selected.
Added translatable strings for the different tabs and ensured correct
functioning even when feed is empty (used to not show the nvigation tabs
if feed was empty).
Added descriptions to functions.
Removed the seected button from the settings page.
Ensured that foltering and sorting in the new views works correctly.

NOTE: All favorites are now stored in the local database which might
cause slowness and storage spage filling if there are alot of them.
The link being not null in some cases when it was expected to be null in
some tests caused failures.
Added a link that s a copy of loans link to the
mock account, as otherwise tests that use syncBook and
the mock account would get the noURI error.
Made select and unselect tasks extend the AbstractBookTask class, so
that the 401 error can be caught and handled in the controller.
If select or unselect trigger token refresh and it's successful, the
view is reloaded, but the popup does still show, though it is delayed
due to the login behaviour.

SyncBook behaviour was also adjusted, as it was triggering the refresh
twice in a row, causing the first one cto succeed and the next one fail,
causing the user to be logged out unnecessarily.
@natlibfi-burger natlibfi-burger marked this pull request as ready for review February 7, 2025 14:32
Moved the tabs around so that loans and reservation are under one tab,
and that selected books are on another, TO this second tab, new tabs can
be added for other lists.

Revoking a book worked incorrectly when a book was selected, causing an
old state of the book to be stored in registry. Fixed this.

There also was a stalement issue with the views on logging out and back
in. Fixed this by popping the backstack of the botton fragments, so that
they are recreated the next time the views are looked at.

Hid the collection buttons for all languages.
Added a check for select funtion, that shows the login page when trying
to choose a book while logged out.

Fized a string.
Added the same account check to unselect, though ending up in that case
shouldn't ever happen.
Added a translatable title to selected, and added back an old title, in
case it needs usage later, and to not delete the translation.
Copy link

@NatLibFi-JoonaKupe NatLibFi-JoonaKupe left a comment

Choose a reason for hiding this comment

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

The use of sets and maps in booksynctasks for tracking IDs and entries should provide efficient lookups and insertions, which is important when handling potentially large numbers of books and feed entries.
Some of the database and feed processing could also be refactored into separate methods or classes to improve readability and maintainability is just a little something to think about for future use, but as it already is somewhat spread around, we'll just need to keep this in mind.

The code covers set of functionalities required to handle multiple book feeds in a way I kinda like here. Good practices in error handling and logging.

Did not try to run the code. Only code review done. Should this be presented to our feedback group for comments before merging?

@natlibfi-ptalosel
Copy link
Contributor

natlibfi-ptalosel commented Mar 5, 2025

First notes from testing with device.

  • Both loans list and holds list show the same text if lists are empty. Text is either "Jos sinulla on varauksia" or the "Lainauksesi näytetään täällä". Holds list should show the holds text and vice versa.
  • It is hard to tap the Select button in any book view, as the button is quite small (or the button has small target area)
  • In Favorites list the button with Finnish text "Poista" can be misleading. User might think the button removes the book from the Favorites list, but actually it is the button that returns the book. Maybe Palauta should be used instead of Poista, and also the Swedish and English translations should probably check. The button text was not previously a problem, as we had only loans and holds book lists.
  • If user tries to reserve a book and there is an error (for example user has already the maximum number of books reserved), the book is displayed in loans list and not in holds list
  • If the favorite list is ordered by Author, the Authors firstname is used for sorting instead of surname

Otherwise favorite functionality works as expected when testing with device. Toast notifications when adding or removing a book from favorites were nice! 👍

Increased the touch area of the favorite button on all views.

Added a UI refresh to showing empty feed, so the shown text is correct
in each view.
Refresh the views after a book's status is reset to either holdable or
loanable. This fixes unloaned books showing up in loans when a loan
fails and the error is dismissed.
Copy link
Contributor

@natlibfi-ptalosel natlibfi-ptalosel left a comment

Choose a reason for hiding this comment

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

Tested with device after updates. OK!

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.

3 participants