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.
What's the status with this bug?
(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..
Still valid?
(In reply to Marc Véron from comment #3) > Still valid? It probably is. Unless you can prove that it is no longer?
(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
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..
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>
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>
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>
Moving 20687 to see also instead of deps. They can be applied independent of each other.
Applying: Bug 11943: Prevent double accepts in Koha::Virtualshelfshare fatal: sha1 information is lacking or useless (Koha/Virtualshelfshare.pm).
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>
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>
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>
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>
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>
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>
Status change confirmed with Kyle on IRC
The user click on a link and... nothing happens? Should not we display a message to explain that the list is already shared?
(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.
(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.
(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.
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>
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>
Pushed to master for 18.05, thanks to everybody involved!
Pushed to stable for 17.11.6
Pushed to 17.05.x for v17.05.12