Bug 11943 - Koha::Virtualshelfshare duplicates rows for the same list
Summary: Koha::Virtualshelfshare duplicates rows for the same list
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Lists (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 9032
Blocks: 20753 20754
  Show dependency treegraph
 
Reported: 2014-03-14 15:01 UTC by Marcel de Rooy
Modified: 2019-10-14 19:56 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 11943: Prevent double accepts in Koha::Virtualshelfshare (2.07 KB, patch)
2018-05-04 08:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11943: Add unit test for double accepts (1.90 KB, patch)
2018-05-04 08:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11943: Db revision to remove double accepted shares (1.75 KB, patch)
2018-05-04 08:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11943: Prevent double accepts in Koha::Virtualshelfshare (2.09 KB, patch)
2018-05-07 11:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 11943: Add unit test for double accepts (1.92 KB, patch)
2018-05-07 11:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 11943: Db revision to remove double accepted shares (1.77 KB, patch)
2018-05-07 11:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 11943: Prevent double accepts in Koha::Virtualshelfshare (2.16 KB, patch)
2018-05-07 13:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11943: Add unit test for double accepts (1.99 KB, patch)
2018-05-07 13:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11943: Db revision to remove double accepted shares (1.84 KB, patch)
2018-05-07 13:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11943: Prevent duplicating rows in Koha::Virtualshelfshare (2.25 KB, patch)
2018-05-11 05:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11943: Add unit test for double accepts (2.01 KB, patch)
2018-05-11 05:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2014-03-14 15:01:39 UTC
As a follow-up on bug 9032, noting a small problem here:
If you share the same list twice and one patron accepts both invites, a second share record exists for the same combination of patron and list. Apart from that, there is no problem when viewing the lists or removing the share (both records are removed). When the user accepts the second time, he will be redirected to the list just as the first time.

But for better housekeeping, I will still submit a patch here that deals with this situation. Imho this should not block pushing the 9032 patches.
Comment 1 Joonas Kylmälä 2015-08-25 15:21:02 UTC
What's the status with this bug?
Comment 2 Marcel de Rooy 2015-08-26 11:11:54 UTC
(In reply to Joonas Kylmälä from comment #1)
> What's the status with this bug?

There is a report now that rewrites a lot of lists code. After that we can verify if this problem is still possible. (Probably..)

See bug 14544..
Comment 3 Marc Véron 2016-09-29 15:23:57 UTC
Still valid?
Comment 4 Marcel de Rooy 2016-10-10 06:50:32 UTC
(In reply to Marc Véron from comment #3)
> Still valid?
It probably is. Unless you can prove that it is no longer?
Comment 5 Marc Véron 2016-10-10 16:20:00 UTC
(In reply to Marcel de Rooy from comment #4)
> (In reply to Marc Véron from comment #3)
> > Still valid?
> It probably is. Unless you can prove that it is no longer?

Still valid.

At the moment you send an inivtation, an entry is created in table virtualshelfshares.
The table does not know, where the invitation went to (borrowernumber is NULL), mail address of invitee is not captured).

After confirmation (link contained in mail), the borrowernumber is filled in with the inviteed's number, and the invitee is redirected to the list as appropriate.

If the list owner created two invitations, we have two entries in virtualshelfshares. No chance to find out that the invitee already has been invited.

With 2 invitations, invitee gets 2 mails (as expected from findings above and as described in initial comment).
Clicking on the link in the first mail results in following message:
"Sorry, but we could not accept this key. The invitation may have expired. Contact the patron who sent you the invitation."

Clicking on the link in the 2nd mail works and shows the list.

If I delete the list, both entries are removed from virtualshelfshares.

-----------

Hmm, what happens if I have 2 patrons with the same email address?

Sending an invitation with this email address creates 1 entry in virtualshelfshares and 1 invitation mail. 

Clicking on the link takes me to the OPAC login screen. The first of the 2 patrons who logs in wins the race.

We have something similar with password resetting requests, see
Bug 16711 - OPAC Password recovery: Handling if multiple accounts have the same mail address
and it would be a problem with:
Bug 2237 - Use primary email address as Koha login account
Comment 6 Marcel de Rooy 2018-05-04 07:00:46 UTC
While testing 20687, I discovered that this one is still valid :)

Still possible to accept two shares on the same list.
+----+-------------+----------------+-----------+---------------------+
| id | shelfnumber | borrowernumber | invitekey | sharedate           |
+----+-------------+----------------+-----------+---------------------+
|  4 |          16 |             37 | NULL      | 2018-05-04 08:17:08 |
|  5 |          16 |             37 | NULL      | 2018-05-04 08:44:04 |
+----+-------------+----------------+-----------+---------------------+

Will give it a try now..
Comment 7 Marcel de Rooy 2018-05-04 08:57:24 UTC
Created attachment 75049 [details] [review]
Bug 11943: Prevent double accepts in Koha::Virtualshelfshare

This 'bug' existed long enough now to finally remove it ;)

We do so by deleting the invitation if the borrower already has a share
on this list. Actually not that hard.

We still need: a unit test and a db revision.

Test plan:
[1] Share a list. Let user B accept.
[2] Without this patch: Share again and let B accept again.
[3] Verify that you have two shares for this list in virtualshelfshares.
[4] With this patch: Share another list, let B accept.
[5] Share this other list again, let B accept again.
[6] Verify that virtualshelfshares does not contain double entries now.
    (Note: This pertains to the second list only.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2018-05-04 08:57:28 UTC
Created attachment 75050 [details] [review]
Bug 11943: Add unit test for double accepts

Test plan:
Run t/db_dependent/Virtualshelves.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2018-05-04 08:57:33 UTC
Created attachment 75051 [details] [review]
Bug 11943: Db revision to remove double accepted shares

Although it is no problem to have them, we could do a cleanup.
This patch just removes duplicate rows from the table.

Note: I considered adding a unique index like:
    ALTER TABLE virtualshelfshares ADD UNIQUE INDEX (shelfnumber, borrowernumber, invitekey);
But the possible NULL values in borrowernumber and/or invitekey require
additional code changes. So I left it alone.

Test plan:
[1] Create two records with same borrowernumber and shelfnumber in the shares
    table, if not present already.
[2] Run updatedatabase.pl

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2018-05-04 09:14:29 UTC
Moving 20687 to see also instead of deps.
They can be applied independent of each other.
Comment 11 Nick Clemens 2018-05-07 11:24:54 UTC
Applying: Bug 11943: Prevent double accepts in Koha::Virtualshelfshare
fatal: sha1 information is lacking or useless (Koha/Virtualshelfshare.pm).
Comment 12 Nick Clemens 2018-05-07 11:29:40 UTC
Created attachment 75104 [details] [review]
Bug 11943: Prevent double accepts in Koha::Virtualshelfshare

This 'bug' existed long enough now to finally remove it ;)

We do so by deleting the invitation if the borrower already has a share
on this list. Actually not that hard.

We still need: a unit test and a db revision.

Test plan:
[1] Share a list. Let user B accept.
[2] Without this patch: Share again and let B accept again.
[3] Verify that you have two shares for this list in virtualshelfshares.
[4] With this patch: Share another list, let B accept.
[5] Share this other list again, let B accept again.
[6] Verify that virtualshelfshares does not contain double entries now.
    (Note: This pertains to the second list only.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Nick Clemens 2018-05-07 11:29:43 UTC
Created attachment 75105 [details] [review]
Bug 11943: Add unit test for double accepts

Test plan:
Run t/db_dependent/Virtualshelves.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Nick Clemens 2018-05-07 11:29:47 UTC
Created attachment 75106 [details] [review]
Bug 11943: Db revision to remove double accepted shares

Although it is no problem to have them, we could do a cleanup.
This patch just removes duplicate rows from the table.

Note: I considered adding a unique index like:
    ALTER TABLE virtualshelfshares ADD UNIQUE INDEX (shelfnumber, borrowernumber, invitekey);
But the possible NULL values in borrowernumber and/or invitekey require
additional code changes. So I left it alone.

Test plan:
[1] Create two records with same borrowernumber and shelfnumber in the shares
    table, if not present already.
[2] Run updatedatabase.pl

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Kyle M Hall 2018-05-07 13:05:49 UTC
Created attachment 75120 [details] [review]
Bug 11943: Prevent double accepts in Koha::Virtualshelfshare

This 'bug' existed long enough now to finally remove it ;)

We do so by deleting the invitation if the borrower already has a share
on this list. Actually not that hard.

We still need: a unit test and a db revision.

Test plan:
[1] Share a list. Let user B accept.
[2] Without this patch: Share again and let B accept again.
[3] Verify that you have two shares for this list in virtualshelfshares.
[4] With this patch: Share another list, let B accept.
[5] Share this other list again, let B accept again.
[6] Verify that virtualshelfshares does not contain double entries now.
    (Note: This pertains to the second list only.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall 2018-05-07 13:05:57 UTC
Created attachment 75121 [details] [review]
Bug 11943: Add unit test for double accepts

Test plan:
Run t/db_dependent/Virtualshelves.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2018-05-07 13:06:00 UTC
Created attachment 75122 [details] [review]
Bug 11943: Db revision to remove double accepted shares

Although it is no problem to have them, we could do a cleanup.
This patch just removes duplicate rows from the table.

Note: I considered adding a unique index like:
    ALTER TABLE virtualshelfshares ADD UNIQUE INDEX (shelfnumber, borrowernumber, invitekey);
But the possible NULL values in borrowernumber and/or invitekey require
additional code changes. So I left it alone.

Test plan:
[1] Create two records with same borrowernumber and shelfnumber in the shares
    table, if not present already.
[2] Run updatedatabase.pl

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Marcel de Rooy 2018-05-09 11:13:09 UTC
Status change confirmed with Kyle on IRC
Comment 19 Jonathan Druart 2018-05-10 15:33:32 UTC
The user click on a link and... nothing happens?

Should not we display a message to explain that the list is already shared?
Comment 20 Jonathan Druart 2018-05-10 16:12:46 UTC
(In reply to Jonathan Druart from comment #19)
> The user click on a link and... nothing happens?
> 
> Should not we display a message to explain that the list is already shared?

This should be done on a separate bug report, to not block the inclusion into 18.05 (string changes).

However, should not we put the update DB entry on a separate bug report and deal with the DB structure later? We will have to recheck the duplicate on create the unique constraint anyway.
Comment 21 Marcel de Rooy 2018-05-11 05:38:23 UTC
(In reply to Jonathan Druart from comment #19)

> The user click on a link and... nothing happens?

No, that is a misunderstanding. He clicks on the link and he is redirected to view this shared list. The behavior interface-wise is exactly the same with and without this patch. The only difference this bugfix makes is that the shared is not duplicated.

> Should not we display a message to explain that the list is already shared?

I thought about it too. Strictly speaking, this bugfix can still be pushed without string changes. And adding a message could be considered as an enhancement for a new report. Opened bug 20753 for it.

In view of the above I am renaming/narrowing down the bug title. I will also look at the commit message in that sense.
Comment 22 Marcel de Rooy 2018-05-11 05:45:47 UTC
(In reply to Jonathan Druart from comment #20)
> However, should not we put the update DB entry on a separate bug report and
> deal with the DB structure later? We will have to recheck the duplicate on
> create the unique constraint anyway.

I am not sure; see commit message. If we add a unique constraint, we need some more code changes (replacing NULL by empty string and that kind).
We could move it to its own report and discuss further ;)
If we do not really care, the dbrev is sufficient with the changed code. I opened report 20754 for it, but would certainly not mind pushing the third patch.
Comment 23 Marcel de Rooy 2018-05-11 05:51:38 UTC
Created attachment 75246 [details] [review]
Bug 11943: Prevent duplicating rows in Koha::Virtualshelfshare

This 'bug' existed long enough now to finally remove it ;)

We do so by deleting the invitation if the borrower already has a share
on this list. The user does not notice that, since he is redirected to
the list view just as before.

We still need: a unit test and a db revision.

Test plan:
[1] Share a list. Let user B accept.
[2] Without this patch: Share again and let B accept again.
[3] Verify that you have two shares for this list in virtualshelfshares.
[4] With this patch: Share another list, let B accept.
[5] Share this other list again, let B accept again.
[6] Verify that virtualshelfshares does not contain double entries now.
    (Note: This pertains to the second list only.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Marcel de Rooy 2018-05-11 05:51:43 UTC
Created attachment 75247 [details] [review]
Bug 11943: Add unit test for double accepts

Test plan:
Run t/db_dependent/Virtualshelves.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Jonathan Druart 2018-05-11 14:12:39 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 26 Nick Clemens 2018-05-22 11:10:29 UTC
Pushed to stable for 17.11.6
Comment 27 Fridolin Somers 2018-05-23 05:31:19 UTC
Pushed to 17.05.x for v17.05.12