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

SIMPLY-3356 NYU Book status wording #1275

Merged
merged 2 commits into from
Dec 22, 2020
Merged

Conversation

vladimirfedorov
Copy link
Contributor

@vladimirfedorov vladimirfedorov commented Dec 18, 2020

What's this do?

  • change book details "This open-access book is available to keep" to "This book is available to borrow." for cases when the book is available and has no specified number of total or available to borrow items.
  • removes NYPLBookButtonsStateCanKeep case from NYPLBookButtonsState

Why are we doing this? (w/ JIRA link if applicable)
https://jira.nypl.org/browse/SIMPLY-3356
Currently NYU book details display the misleading status "This open-access book is available to keep". This status appears when OPDS entries for these books don't have any specified amount available to borrow and isn't connected with "open-access" link relation.

How should this be tested? / Do these changes have associated tests?
In "New York University Libraries" tap any book, e.g., "A Doll's House". Book status should read "This book is available to borrow.".

Dependencies for merging? Releasing to production?
No

Has the application documentation been updated for these changes?
N/A

Did someone actually run this code to verify it works?
@vladimirfedorov

@vladimirfedorov vladimirfedorov marked this pull request as ready for review December 18, 2020 20:18
@vladimirfedorov vladimirfedorov changed the title Feature/simply 3356 SIMPLY-3356 NYU Book status wording Dec 18, 2020
Copy link
Contributor

@leonardr leonardr left a comment

Choose a reason for hiding this comment

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

This branch removes the "yours to keep" language altogether, rather than fixing it to only apply to books that are actually open-access. This is my preferred solution but I don't want to merge this code until we've actually agreed to go in that direction.

We also need to investigate whether this issue applies on Android, and make the messaging consistent across platforms. Removing the language here means also removing it from Android, even though I'm guessing Android displays the message only for actual open-access books.

@vbessonov
Copy link

@vladimirfedorov, could you keep This open-access book is available to keep for open-access acquisition links to not change the current behaviour?

Copy link
Collaborator

@ettore ettore left a comment

Choose a reason for hiding this comment

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

after reading the ticket, these changes make sense to me code-wise, but my approval is conditional to @leonardr's for timing about when we should merge this.

@vladimirfedorov
Copy link
Contributor Author

@vladimirfedorov, could you keep This open-access book is available to keep for open-access acquisition links to not change the current behaviour?

Currently the app doesn't have that feature, we can't keep this line for open-access acquisitions but instead we can add this feature the way it is done on Android. If Android displays this text correctly, we can add the same logics to iOS version of the app.

Copy link
Contributor

@leonardr leonardr left a comment

Choose a reason for hiding this comment

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

Risa has approved removing this message altogether until we come up with a better solution.

@vladimirfedorov vladimirfedorov merged commit 4de9b6d into develop Dec 22, 2020
@vladimirfedorov vladimirfedorov deleted the feature/SIMPLY-3356 branch December 22, 2020 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants