Bug 8260 - Deleting patrons leaves holes in routing list ranking
Summary: Deleting patrons leaves holes in routing list ranking
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-18 02:25 UTC by David Cook
Modified: 2019-10-17 13:54 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00


Attachments
Force re-order of routing list rankings on page load (2.26 KB, patch)
2012-06-18 03:13 UTC, David Cook
Details | Diff | Splinter Review
Bug 8260: user rank order instead of absolute rank to set dropdowns on routing.pl (1.89 KB, patch)
2019-06-06 19:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 8260: user rank order instead of absolute rank to set dropdowns on routing.pl (1.94 KB, patch)
2019-10-15 14:12 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 8260: user rank order instead of absolute rank to set dropdowns on routing.pl (2.05 KB, patch)
2019-10-16 06:20 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2012-06-18 02:25:45 UTC
Currently, deleting a patron will create a missing value in the ranking of members in a routing list. For instance, the ranking will go 1, 2, 3, 5 in the database when the patron ranked at #4 is deleted. 

This causes the intranet to display these rankings as 1, 2, 3, 1, since 5 is not a valid option due to there only being 4 entries in the database. 

I've written up a little re-order function based on the reorder_members function, which just re-orders all the members for that routing list every time routing.pl or routing-preview.pl are called. 

It might be more ideal to only run it if errors are detected, but that error detection would require more code that might just add more bulk.
Comment 1 David Cook 2012-06-18 02:28:13 UTC
Actually, I'm not currently running it for routing-preview.pl, since there are no numbers displayed. 

(In reply to comment #0)
> Currently, deleting a patron will create a missing value in the ranking of
> members in a routing list. For instance, the ranking will go 1, 2, 3, 5 in
> the database when the patron ranked at #4 is deleted. 
> 
> This causes the intranet to display these rankings as 1, 2, 3, 1, since 5 is
> not a valid option due to there only being 4 entries in the database. 
> 
> I've written up a little re-order function based on the reorder_members
> function, which just re-orders all the members for that routing list every
> time routing.pl or routing-preview.pl are called. 
> 
> It might be more ideal to only run it if errors are detected, but that error
> detection would require more code that might just add more bulk.
Comment 2 David Cook 2012-06-18 03:13:54 UTC
Created attachment 10342 [details] [review]
Force re-order of routing list rankings on page load
Comment 3 Chris Cormack 2012-06-24 07:40:03 UTC
This works well, and mimics the behaviour that happens with the reserves that are set if you have system preference set to create reserves for routing lists.
Comment 4 Chris Cormack 2012-06-24 07:45:43 UTC
The only problem is that it is called unconditionally on every load of the page, even if we just reranked the list by deleting a member, or changing the rank. 

For that reason i'm hesitant to sign off, does anyone else have a better idea on how to solve this? I'm thinking fixing the borrower delete to not delete borrowers who are in a routing list might be better.
Comment 5 David Cook 2012-06-25 02:04:52 UTC
(In reply to comment #4)
> The only problem is that it is called unconditionally on every load of the
> page, even if we just reranked the list by deleting a member, or changing
> the rank. 
> 
> For that reason i'm hesitant to sign off, does anyone else have a better
> idea on how to solve this? I'm thinking fixing the borrower delete to not
> delete borrowers who are in a routing list might be better.

I agree with the problem, but I think it's still a good idea to be able to delete borrowers who are in routing lists. I think it would be a bit too finicky to have to go into each routing list and remove them before doing a complete delete.

Why don't we call the new reorder function on patron delete? I see that there is already a function for handling patron deletes in regards to Lists/VirtualShelves.  That probably makes the most sense.
Comment 6 Galen Charlton 2013-08-12 16:07:00 UTC
(In reply to David Cook from comment #5)
> Why don't we call the new reorder function on patron delete? I see that
> there is already a function for handling patron deletes in regards to
> Lists/VirtualShelves.  That probably makes the most sense.

I prefer this option.
Comment 7 Nick Clemens 2019-06-06 19:22:09 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2019-07-13 08:02:46 UTC
I think I like the idea of renumbering in the database a bit better, as it will also fix things for any reports and similar. And it always seems a little odd to display something different than what to is stored in the db. That said, I guess David's patch would have to be reworked for the Koha namespace?
Comment 9 Katrin Fischer 2019-10-09 09:47:04 UTC
(In reply to Katrin Fischer from comment #8)
> I think I like the idea of renumbering in the database a bit better, as it
> will also fix things for any reports and similar. And it always seems a
> little odd to display something different than what to is stored in the db.
> That said, I guess David's patch would have to be reworked for the Koha
> namespace?

Another issue: Printing routing lists. I think Nick's patch only fixes the view, not the print.
Comment 10 David Cook 2019-10-09 23:15:03 UTC
(In reply to Katrin Fischer from comment #9)
> (In reply to Katrin Fischer from comment #8)
> > That said, I guess David's patch would have to be reworked for the Koha
> > namespace?
> 
> Another issue: Printing routing lists. I think Nick's patch only fixes the
> view, not the print.

Looks like I wrote my patch back in 2012 and I think I had it applied locally in 3.8, but didn't bother to apply it locally after that. Seems that I haven't had any complaints about it since 2012 either, so not really a priority for me at the moment.
Comment 11 Nick Clemens 2019-10-11 14:27:37 UTC
(In reply to Katrin Fischer from comment #9)
> (In reply to Katrin Fischer from comment #8)
> > I think I like the idea of renumbering in the database a bit better, as it
> > will also fix things for any reports and similar. And it always seems a
> > little odd to display something different than what to is stored in the db.
> > That said, I guess David's patch would have to be reworked for the Koha
> > namespace?
> 
> Another issue: Printing routing lists. I think Nick's patch only fixes the
> view, not the print.

The print doesn't include the numbers, and the ordering will still be correct

In reports too, the numbering will still be ordered correctly, it just may have a gap.

happy to have someone supply an alternate
Comment 12 Owen Leonard 2019-10-15 14:12:02 UTC
Created attachment 94215 [details] [review]
Bug 8260: user rank order instead of absolute rank to set dropdowns on routing.pl

When deleting a borrower on a routing list we leave a gap in the numbering. It appears to only be a problem when viewing
or editing the list in the staff client.

Rather than use the rank in the DB to set the values, we use the implicit order

To test:
 1 - Create a subscription (or find one)
 2 - Add a routing list
 3 - Add five patrons
 4 - Delete the patron who is third in the list
 5 - Edit the routing list
 6 - Note the priority for the last patron is 1
 7 - Apply patch
 8 - Reload the page
 9 - Values look correct
10 - Confirm reordering works as expected

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 13 Katrin Fischer 2019-10-15 19:36:29 UTC
(In reply to Nick Clemens from comment #11)
> (In reply to Katrin Fischer from comment #9)
> > (In reply to Katrin Fischer from comment #8)
> > > I think I like the idea of renumbering in the database a bit better, as it
> > > will also fix things for any reports and similar. And it always seems a
> > > little odd to display something different than what to is stored in the db.
> > > That said, I guess David's patch would have to be reworked for the Koha
> > > namespace?
> > 
> > Another issue: Printing routing lists. I think Nick's patch only fixes the
> > view, not the print.
> 
> The print doesn't include the numbers, and the ordering will still be correct
> 
> In reports too, the numbering will still be ordered correctly, it just may
> have a gap.
> 
> happy to have someone supply an alternate

You got a point there. If we are sure it doesn't cause issues with reordering or similar, I am ok.
Comment 14 Katrin Fischer 2019-10-16 06:20:56 UTC
Created attachment 94226 [details] [review]
Bug 8260: user rank order instead of absolute rank to set dropdowns on routing.pl

When deleting a borrower on a routing list we leave a gap in the numbering. It appears to only be a problem when viewing
or editing the list in the staff client.

Rather than use the rank in the DB to set the values, we use the implicit order

To test:
 1 - Create a subscription (or find one)
 2 - Add a routing list
 3 - Add five patrons
 4 - Delete the patron who is third in the list
 5 - Edit the routing list
 6 - Note the priority for the last patron is 1
 7 - Apply patch
 8 - Reload the page
 9 - Values look correct
10 - Confirm reordering works as expected

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Martin Renvoize 2019-10-17 13:54:31 UTC
Nice work!

Pushed to master for 19.11.00