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

remove inlinejs for waitlist #9038

Merged
merged 8 commits into from
Apr 15, 2024

Conversation

RayBB
Copy link
Collaborator

@RayBB RayBB commented Apr 5, 2024

Closes #8378

Technical

Testing

I don't know locate a book that is waitlistable to test better. However, I have a book on my waitlist from long ago. The testing plan is:

  1. Demonstrate the functionality of the "Leave Waitlist" button on the Open Library testing platform, accessible at https://testing.openlibrary.org/account/loans.
  2. Deploy a version of the application to the testing environment that temporarily removes this functionality. This step is crucial for observing the impact of the removal on the user experience.
  3. Deploy a refactored version of the application to the testing environment. This deployment aims to reintroduce the "Leave Waitlist" functionality, ensuring that it operates as expected and maintains consistency with the previous version.

This plan will be shown via the screen recording

Screenshot

waitlist720_2.mp4

Stakeholders

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 15.89%. Comparing base (53fb146) to head (9247654).

Files Patch % Lines
openlibrary/plugins/openlibrary/js/waitlist.js 0.00% 7 Missing and 1 partial ⚠️
openlibrary/plugins/openlibrary/js/index.js 0.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9038      +/-   ##
==========================================
- Coverage   15.93%   15.89%   -0.05%     
==========================================
  Files          89       90       +1     
  Lines        4720     4732      +12     
  Branches      823      825       +2     
==========================================
  Hits          752      752              
- Misses       3457     3467      +10     
- Partials      511      513       +2     

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

@RayBB RayBB force-pushed the refactor/move-waitlist-code branch from 1b416c8 to 9247654 Compare April 9, 2024 09:28
@RayBB RayBB added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Apr 9, 2024
@RayBB RayBB marked this pull request as ready for review April 9, 2024 09:54
@RayBB RayBB requested a review from jimchamp April 9, 2024 09:55
@jimchamp jimchamp self-assigned this Apr 9, 2024
Copy link
Collaborator

@jimchamp jimchamp left a comment

Choose a reason for hiding this comment

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

Thanks @RayBB!

While testing with production data, the dialog would open when I clicked a a.leave link. However, confirming would not remove me from the waitlist.

I mocked a waiting list on my local environment, and discovered that the context of this has changed from a.leave to the initLeaveWaitlist function. I've provided some code that should fix that.

I also noticed that the "Leave waitlist" dialog has an existing bug. There should be a "Are you sure you want to leave the waiting list of {title}?" CTA displayed in the dialog, but it is hidden by the hidden class. I think that removing hidden from the #leave-waitinglist-dialog element on a.leave clicks would fix this, but I didn't test it out.

openlibrary/plugins/openlibrary/js/index.js Outdated Show resolved Hide resolved
openlibrary/plugins/openlibrary/js/waitlist.js Outdated Show resolved Hide resolved
RayBB and others added 3 commits April 15, 2024 11:23
Co-authored-by: jimchamp <jameschamp@acm.org>
Co-authored-by: jimchamp <jameschamp@acm.org>
@RayBB RayBB requested a review from jimchamp April 15, 2024 09:51
@RayBB
Copy link
Collaborator Author

RayBB commented Apr 15, 2024

@jimchamp I addressed your comments. Good catch on the context changing. I've updated the code as you mentioned and put it on testing and it seems to be working right (easier to tell now that the title isn't hidden in the dialog).

Did you figure out how to add books to your waitlist in prod?
Or can you share how you were mocking the responses? Is there an easy way to do that?

In any case, I'm hoping this PR is done now! 👍

@RayBB RayBB added the Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] label Apr 15, 2024
Copy link
Collaborator

@jimchamp jimchamp left a comment

Choose a reason for hiding this comment

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

Lgtm! Tested removing some myself from some books' wait lists on testing.openlibrary.org.

Did you figure out how to add books to your waitlist in prod?

I had to join wait lists on archive.org details pages, from the borrow button's drop down options:
Screenshot from 2024-04-15 15-49-37

Or can you share how you were mocking the responses? Is there an easy way to do that?

I hacked together this code to test your changes locally. I mocked the response based on how the wait list is represented in the store, as per this docstring, and mocked the methods that are called in /account/loans.html (the commented-out attributes weren't needed for that page).

This was definitely a quick and dirty way to mock this. It would be more broadly useful if User.get_waitinglist() returned some configurable waiting list in dev enviroments, and that the mocked methods returned expected values (without the need for additional parameters, like the get_book call in the linked branch). I'm not sure if this is a worthwhile pursuit, though, as we rarely need a working waitlist in dev...

@jimchamp jimchamp merged commit e8ea2ba into internetarchive:master Apr 15, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove inline js: account/loans.html
3 participants