Bug 21479 - Removing from cart removes 2 items
Summary: Removing from cart removes 2 items
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Owen Leonard
QA Contact:
URL:
Keywords:
Depends on: 19802
Blocks: 21174 22881
  Show dependency treegraph
 
Reported: 2018-10-02 20:23 UTC by Jonathan Druart
Modified: 2020-01-06 20:17 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21479: Fix remove from cart - OPAC (1.77 KB, patch)
2018-10-02 20:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21479: Fix remove from cart - OPAC (1.97 KB, patch)
2018-10-03 02:19 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21479: (alternate) Removing from cart removes 2 items (7.65 KB, patch)
2018-10-03 14:28 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21479: (alternate) Removing from cart removes 2 items (7.66 KB, patch)
2018-10-03 15:55 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21479: (alternate) Removing from cart removes 2 items (7.73 KB, patch)
2018-10-04 16:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 21479: (alternate) Removing from cart removes 2 items (7.76 KB, patch)
2018-10-08 13:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21479: Fix remove from cart links at the OPAC (should remove only 1) (7.88 KB, patch)
2018-10-09 16:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21479: Add regression tests (2.61 KB, patch)
2018-10-09 16:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21479: Fix remove from cart links at the OPAC (should remove only 1) (7.85 KB, patch)
2018-10-09 18:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21479: Add regression tests (2.67 KB, patch)
2018-10-09 18:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21479: Remove the trace if the node does not exist (1.82 KB, patch)
2018-10-12 18:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21479: Zebra index can return different result (2.52 KB, patch)
2018-10-15 14:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21479: Mock SearchEngine to use Zebra (976 bytes, patch)
2018-10-15 17:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21479: "Mock" SearchEngine to use Zebra (1.35 KB, patch)
2018-10-15 20:43 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2018-10-02 20:23:23 UTC
This bug affects all stable releases and seems to have been there for a long time.

To recreate:
- Launch a search at the OPAC
- Add 2 elements to the cart
- Remove one
=> Cart is empty

Note that it does not exist on the staff interface
Comment 1 Jonathan Druart 2018-10-02 20:36:31 UTC
Created attachment 79852 [details] [review]
Bug 21479: Fix remove from cart - OPAC

This bug affects all stable releases and seems to have been there for a
long time.

To recreate:
- Launch a search at the OPAC
- Add 2 elements to the cart
- Remove one
=> Cart is empty

Note that it does not exist on the staff interface

The code in opac-bottom is only for the detail page, but the evenement
binding was done on the result search page as they have the same selector
(.cartRemove)
Using a more specific selector (already used for "add to shelf") fixes the problem.

Test plan:
Add and remove elements to/from your cart
On both search and detail view
Comment 2 Mark Tompsett 2018-10-03 02:19:26 UTC
Created attachment 79857 [details] [review]
Bug 21479: Fix remove from cart - OPAC

This bug affects all stable releases and seems to have been there for a
long time.

To recreate:
- Launch a search at the OPAC
- Add 2 elements to the cart
- Remove one
=> Cart is empty

Note that it does not exist on the staff interface

The code in opac-bottom is only for the detail page, where
biblionumber is defined, but the event binding was done
on the result search page as they have the same selector
(.cartRemove) On the results page, the biblionumber is not
defined and triggers the deletion of all the items as a result.
Using a more specific selector (this solution is already used
for "add to shelf") fixes the problem

Test plan:
Add and remove elements to/from your cart
On both search and detail view

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 3 Jonathan Druart 2018-10-03 10:29:59 UTC
David reported on IRC he did not recreate the issue, if the problem does not exist for everybody we will need to understand why.

Please do not push without knowing all the details.
Comment 4 Owen Leonard 2018-10-03 14:28:35 UTC
Created attachment 79893 [details] [review]
Bug 21479: (alternate) Removing from cart removes 2 items

This patch changes the way biblionumbers are passed to the cart script
for adding and removing single items. The title's biblionumber is now
stored in a data-attribute on the links for adding and removing. This
should be a more robust, unified way to handle these single-item
operations.

To test, apply the patch and enable the opacbookbag system preference.

Test adding and removing items from the cart from various pages using
various methods:

 - By clicking an "Add to your cart" / "Remove" link in search results
 - By clicking an "Add to your cart" / "Remove" link on a detail page
 - By checking boxes in search results and adding via the dropdown

Each operation should work correctly.
Comment 5 Jonathan Druart 2018-10-03 15:10:53 UTC
Owen, with your patch:
From the result page: Add
Go to the detail page, "In your cart (remove)"
The "In your cart" link does nothing => KO
Comment 6 Owen Leonard 2018-10-03 15:55:52 UTC
Created attachment 79903 [details] [review]
Bug 21479: (alternate) Removing from cart removes 2 items

This patch changes the way biblionumbers are passed to the cart script
for adding and removing single items. The title's biblionumber is now
stored in a data-attribute on the links for adding and removing. This
should be a more robust, unified way to handle these single-item
operations.

To test, apply the patch and enable the opacbookbag system preference.

Test adding and removing items from the cart from various pages using
various methods:

 - By clicking an "Add to your cart" / "Remove" link in search results
 - By clicking an "Add to your cart" / "Remove" link on a detail page
 - By checking boxes in search results and adding via the dropdown

Each operation should work correctly.
Comment 7 David Nind 2018-10-04 14:52:41 UTC
Able to see the problem now - was using cart itself to remove items, rather than adding and removing from the search result list.

Getting this message when trying to apply patch (git bz apply 21479):

Applying: Bug 21479: Fix remove from cart - OPAC
Applying: Bug 21479: (alternate) Removing from cart removes 2 items
Using index info to reconstruct a base tree...
M	koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-bottom.inc
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-bottom.inc
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-bottom.inc
error: Failed to merge in the changes.
Patch failed at 0001 Bug 21479: (alternate) Removing from cart removes 2 items
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-21479-alternate-Removing-from-cart-removes-2-i-Fk_Tok.patch
Comment 8 David Nind 2018-10-04 16:26:29 UTC
Created attachment 79950 [details] [review]
Bug 21479: (alternate) Removing from cart removes 2 items

This patch changes the way biblionumbers are passed to the cart script
for adding and removing single items. The title's biblionumber is now
stored in a data-attribute on the links for adding and removing. This
should be a more robust, unified way to handle these single-item
operations.

To test, apply the patch and enable the opacbookbag system preference.

Test adding and removing items from the cart from various pages using
various methods:

 - By clicking an "Add to your cart" / "Remove" link in search results
 - By clicking an "Add to your cart" / "Remove" link on a detail page
 - By checking boxes in search results and adding via the dropdown

Each operation should work correctly.

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 Martin Renvoize 2018-10-08 13:37:54 UTC
Created attachment 80219 [details] [review]
Bug 21479: (alternate) Removing from cart removes 2 items

This patch changes the way biblionumbers are passed to the cart script
for adding and removing single items. The title's biblionumber is now
stored in a data-attribute on the links for adding and removing. This
should be a more robust, unified way to handle these single-item
operations.

To test, apply the patch and enable the opacbookbag system preference.

Test adding and removing items from the cart from various pages using
various methods:

 - By clicking an "Add to your cart" / "Remove" link in search results
 - By clicking an "Add to your cart" / "Remove" link on a detail page
 - By checking boxes in search results and adding via the dropdown

Each operation should work correctly.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2018-10-08 13:40:04 UTC
I like Owens alternative.. as he says, it just feels more robust.

QA Comment though.. could we possibly add a selenium test for this perhaps?
Comment 11 Jonathan Druart 2018-10-09 16:34:41 UTC
Created attachment 80291 [details] [review]
Bug 21479: Fix remove from cart links at the OPAC (should remove only 1)

This patch changes the way biblionumbers are passed to the cart script
for adding and removing single items. The title's biblionumber is now
stored in a data-attribute on the links for adding and removing. This
should be a more robust, unified way to handle these single-item
operations.

To test, apply the patch and enable the opacbookbag system preference.

Test adding and removing items from the cart from various pages using
various methods:

 - By clicking an "Add to your cart" / "Remove" link in search results
 - By clicking an "Add to your cart" / "Remove" link on a detail page
 - By checking boxes in search results and adding via the dropdown

Each operation should work correctly.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Jonathan Druart 2018-10-09 16:34:45 UTC
Created attachment 80292 [details] [review]
Bug 21479: Add regression tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Jonathan Druart 2018-10-09 16:35:47 UTC
(In reply to Martin Renvoize from comment #10)
> I like Owens alternative.. as he says, it just feels more robust.

Agreed.

> QA Comment though.. could we possibly add a selenium test for this perhaps?

Done, the patch is waiting for a/your QA stamp.
Comment 14 Martin Renvoize 2018-10-09 18:20:40 UTC
Created attachment 80304 [details] [review]
Bug 21479: Fix remove from cart links at the OPAC (should remove only 1)

This patch changes the way biblionumbers are passed to the cart script
for adding and removing single items. The title's biblionumber is now
stored in a data-attribute on the links for adding and removing. This
should be a more robust, unified way to handle these single-item
operations.

To test, apply the patch and enable the opacbookbag system preference.

Test adding and removing items from the cart from various pages using
various methods:

 - By clicking an "Add to your cart" / "Remove" link in search results
 - By clicking an "Add to your cart" / "Remove" link on a detail page
 - By checking boxes in search results and adding via the dropdown

Each operation should work correctly.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Martin Renvoize 2018-10-09 18:20:45 UTC
Created attachment 80305 [details] [review]
Bug 21479: Add regression tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2018-10-09 18:22:30 UTC
Works well, no regressions found.  Thankyou for the regression test Jonathan :).

Passing QA
Comment 17 Jonathan Druart 2018-10-12 18:38:58 UTC
Created attachment 80557 [details] [review]
Bug 21479: Remove the trace if the node does not exist

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Tomás Cohen Arazi 2018-10-12 18:46:03 UTC
Pushed for 18.11. Thanks all!
Comment 19 Jonathan Druart 2018-10-15 14:44:01 UTC
Created attachment 80605 [details] [review]
Bug 21479: Zebra index can return different result

If the zebra index has been rebuilt by previous tests, the order/results
will be different, and we will not have biblionumber 1, 3, 5

Let make this more flexible

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Nick Clemens 2018-10-15 17:05:47 UTC
(In reply to Jonathan Druart from comment #19)
> Created attachment 80605 [details] [review] [review]
> Bug 21479: Zebra index can return different result
> 
> If the zebra index has been rebuilt by previous tests, the order/results
> will be different, and we will not have biblionumber 1, 3, 5
> 
> Let make this more flexible
> 
> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Follow up pushed to  master for 18.11
Comment 21 Jonathan Druart 2018-10-15 17:53:59 UTC
Created attachment 80625 [details] [review]
Bug 21479: Mock SearchEngine to use Zebra

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 22 Jonathan Druart 2018-10-15 20:43:54 UTC
Created attachment 80643 [details] [review]
Bug 21479: "Mock" SearchEngine to use Zebra

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 23 Martin Renvoize 2018-10-16 13:55:03 UTC
Pushed to 18.05.x for 18.05.05
Comment 24 Fridolin Somers 2018-11-09 07:56:47 UTC
Pushed to 17.11.x for 17.11.12
Comment 25 Fridolin Somers 2018-11-28 15:26:28 UTC
Regression tests removed from 17.11.x, test suite not able to run selenium for this version.