work doesn't get selected when "Want to Read" button is pressed

[Billa05]

Closes #6881

It fixes the bug that was causing to select the work when pressed on the Want to Read button.

also, I have added a test case for toggelSelected method

Testing

1. setup locally and login as admin
2. go to search result and press want to read button

Screenshot

example.mp4

Stakeholders

@scottbarnes

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (308a35d) 16.51% compared to head (023667a) 17.61%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8529      +/-   ##
==========================================
+ Coverage   16.51%   17.61%   +1.09%     
==========================================
  Files          85       85              
  Lines        4456     4456              
  Branches      782      782              
==========================================
+ Hits          736      785      +49     
+ Misses       3227     3187      -40     
+ Partials      493      484       -9     

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

[scottbarnes]

Thanks for this, @Billa05! The feature works like it should, which is definitely a win. :)

The set up for the tests in this case turned out to be more involved than I thought, and you were 90% of the way there.

One thing to remember with tests is you want to make sure that the test doesn't always pass. In this case, even without the change to toggleSelected(), the test still passed, so I tweaked it and pushed the changes to this branch, with one test that simulates clicking on the dropper / 'want to read', and another that simulates clicking elsewhere.

I'm still not 100% sold on the test set up myself, so if you have any suggestions, please let me know. Also, if you want to make further changes, please pull from your remote branch first on your Open Library fork, so you can integrate these changes into your own local branch.

Once you're happy with everything, I will go ahead and merge this.

[mheiman]

@scottbarnes — I have a PR in review with Drini that refactors this file in some significant ways (e.g. toggleSelected is called something else and has new parameters). If you could hold off on merging this for a few days until that one lands, it would avoid some confusion.

[scottbarnes]

@scottbarnes — I have a PR in review with Drini that refactors this file in some significant ways (e.g. toggleSelected is called something else and has new parameters). If you could hold off on merging this for a few days until that one lands, it would avoid some confusion.

Will do! Thanks for the heads up.

[Billa05]

Thanks for this, @Billa05! The feature works like it should, which is definitely a win. :)

The set up for the tests in this case turned out to be more involved than I thought, and you were 90% of the way there.

One thing to remember with tests is you want to make sure that the test doesn't always pass. In this case, even without the change to toggleSelected(), the test still passed, so I tweaked it and pushed the changes to this branch, with one test that simulates clicking on the dropper / 'want to read', and another that simulates clicking elsewhere.

I'm still not 100% sold on the test set up myself, so if you have any suggestions, please let me know. Also, if you want to make further changes, please pull from your remote branch first on your Open Library fork, so you can integrate these changes into your own local branch.

Once you're happy with everything, I will go ahead and merge this.

Sorry for the delayed response I was travelling. @scottbarnes thank you for the feedback this was my first time writing a test case in js. I appreciate your insights on the test setup, and I understand the importance of ensuring test reliability. Your adjustments make sense, as it checks individually for clicking on ctaDiv and listItem elements. will make the needful changes once @mheiman pr gets reviewed.

[jimchamp]

Merging is blocked until #8441 is merged.

[cdrini]

We can make this a little tighter :) This will also handle any other <button> elements.

Suggested change

	$(clickEvent.target).closest('.searchResultItemCTA').length > 0 ||
	($(clickEvent.target).closest('a').is('a') &&
	($(clickEvent.target).closest('a, button').length > 0 &&

[Billa05]

looks good to me. tested and it is working properly

[Billa05]

the read and want to read buttons are not button elements. but still, this solves the issue. @scottbarnes could you look into this?

[cdrini]

No longer blocked! This will need a rebase though

[Billa05]

@scottbarnes is there any need to change the test case? I think the test case for ctaDiv has to be removed. if required I can try to add a test case for the button, if it makes sense.

[Billa05]

I have modified the testcase file. @scottbarnes could you please review it.

[scottbarnes]

I have modified the testcase file. @scottbarnes could you please review it.

Hi, @Billa05. Thanks for updating this. Can you rebase it against the latest master branch and push the updated commits to this PR?

Please see Making Updates to Your Pull Request if you run into issues rebasing.

[Billa05]

I have modified the testcase file. @scottbarnes could you please review it.

Hi, @Billa05. Thanks for updating this. Can you rebase it against the latest master branch and push the updated commits to this PR?

Please see Making Updates to Your Pull Request if you run into issues rebasing.

this is the first time I have done rebasing. is this right? and should I remove all the commented code from the test case?

[scottbarnes]

Alas, @Billa05, the rebasing has gone slightly awry. You'll want to remove (drop) the commits that aren't related to your PR: https://github.com/internetarchive/openlibrary/pull/8529/commits.

Using the following should get you started:

git fetch upstream master
git rebase -i upstream/master

I have done this exact same thing during some of my first rebases. :) If you get stuck let me know and I can give further direction, though it won't be until tomorrow evening Pacific time at the earliest, most likely.

I happened to have all your commits, at least of perhaps 12 hours or so ago, handy, so this may help:

pick 6d05cdd3e resolve the bug and added a test case
pick 1c09cff50 [pre-commit.ci] auto fixes from pre-commit.com hooks
pick 9ed1e8cba SelectionManager: toggleSelected shouldn't activate with dropper clicks
pick 9d3fd49a6 also handle any other <button> elements
pick 4372bf2d9 renaming
pick aa6c8de2c changing funtion name in testcase
pick 7ce03e9f1 modified testcases

[Billa05]

Alas, @Billa05, the rebasing has gone slightly awry. You'll want to remove (drop) the commits that aren't related to your PR: https://github.com/internetarchive/openlibrary/pull/8529/commits.

Using the following should get you started:

git fetch upstream master
git rebase -i upstream/master

I have done this exact same thing during some of my first rebases. :) If you get stuck let me know and I can give further direction, though it won't be until tomorrow evening Pacific time at the earliest, most likely.

I happened to have all your commits, at least of perhaps 12 hours or so ago, handy, so this may help:

pick 6d05cdd3e resolve the bug and added a test case
pick 1c09cff50 [pre-commit.ci] auto fixes from pre-commit.com hooks
pick 9ed1e8cba SelectionManager: toggleSelected shouldn't activate with dropper clicks
pick 9d3fd49a6 also handle any other <button> elements
pick 4372bf2d9 renaming
pick aa6c8de2c changing funtion name in testcase
pick 7ce03e9f1 modified testcases

firstly this shows up:

then if i ctrl+z it shows this:

now if I am dropping all the others the issue is this ( pick 6d05cdd resolve the bug and added a test case) is getting drop with
(pick 2b93e92 modified testcases)

I tired to undo the rebasing but got nothing other than frustration 😩

according to the commit messages I was resolving the conflicts, I accepted the changes from the respective commits instead of the current code. it that the right way? at last i was left with this:

should I push this?
I was thinking couldn't I have just merged the master branch to the feature branch to update it, is it necessary to have the commits in line?

[Billa05]

hey, @scottbarnes could you please look into this?

[scottbarnes]

@Billa05, I think that the last screenshot, with the following, looks correct:

pick 9d3fd49a6 also handle any other <button> elements
pick 4372bf2d9 renaming
pick aa6c8de2c changing funtion name in testcase
pick 7ce03e9f1 modified testcases

Assuming you resolved all the conflicts, the fix works, and the tests work, then yeah, push it and I can take a look.

In answer to your question about the commented out code, yes, please go ahead and remove it, and as for whether the tests need to change, I can look more closely at that once the rebase is done and the code pushed here.

With respect to why not merge master into your feature branch, it does fundamentally do the same thing in terms of resolving conflicts, but in doing so it adds other code, unrelated to your branch, into your branch. That's fine so long as none of that code later changes, however, it often does change, and it makes it harder to review.

[Billa05]

@scottbarnes thank you for the explanation. I have pushed the commits but it doesn't seem to fix it.
the previous rebase caused this issue, right? So cant I undo it in some way

[scottbarnes]

@Billa05, I was able to rebase the branch with this latest code. I can simply push it here, but if you'd like we could also do a screenshare for a few minutes and I can show you what I've done.

Let me know if you'd like to do a screenshare, or if I should just push the rebased branch. :)

[Billa05]

@Billa05, I was able to rebase the branch with this latest code. I can simply push it here, but if you'd like we could also do a screenshare for a few minutes and I can show you what I've done.

Let me know if you'd like to do a screenshare, or if I should just push the rebased branch. :)

@scottbarnes, I would love to do that. I want to clear the mess I have created 😅

[scottbarnes]

@Billa05, I'll send you a message on Slack to coordinate.

[Billa05]

@Billa05, I'll send you a message on Slack to coordinate.

sure, I am waiting

[Billa05]

@scottbarnes could you please review the test case

[scottbarnes]

Looks good to me. Thanks, @Billa05!

[Billa05]

Looks good to me. Thanks, @Billa05!

Thank you, @scottbarnes, for all your help throughout the issue.