Bug 26567 - Allow to limit subscription search to subscriptions with routing lists
Summary: Allow to limit subscription search to subscriptions with routing lists
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Danyon Sewell
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-09-29 13:44 UTC by Katrin Fischer
Modified: 2024-05-12 08:37 UTC (History)
6 users (show)

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


Attachments
Bug 26567 WIP (3.26 KB, patch)
2023-12-20 04:21 UTC, Danyon Sewell
Details | Diff | Splinter Review
Bug 26567: Allow to limit subscription search to subscriptions with routing lists (4.03 KB, patch)
2023-12-20 04:45 UTC, Danyon Sewell
Details | Diff | Splinter Review
Bug 26567: Allow to limit subscription search to subscriptions with routing lists (4.03 KB, patch)
2024-01-29 23:12 UTC, Danyon Sewell
Details | Diff | Splinter Review
Bug 26567: Allow to limit subscription search to subscriptions with routing lists (4.12 KB, patch)
2024-01-29 23:15 UTC, Danyon Sewell
Details | Diff | Splinter Review
Bug 26567: Allow to limit subscription search to subscriptions with routing lists (6.58 KB, patch)
2024-02-01 21:37 UTC, Danyon Sewell
Details | Diff | Splinter Review
Bug 26567: Allow to limit subscription search to subscriptions with routing lists (8.03 KB, patch)
2024-02-05 10:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26567: (follow-up) Update label and add check for RoutingSerials (2.04 KB, patch)
2024-02-05 10:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26567: Allow to limit subscription search to subscriptions with routing lists (8.04 KB, patch)
2024-05-10 06:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26567: (follow-up) Update label and add check for RoutingSerials (2.08 KB, patch)
2024-05-10 06:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26567: Allow to limit subscription search to subscriptions with routing lists (8.10 KB, patch)
2024-05-10 06:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26567: (follow-up) Update label and add check for RoutingSerials (2.14 KB, patch)
2024-05-10 06:22 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 Katrin Fischer 2020-09-29 13:44:48 UTC
At the moment there is no way for the library to have a look at the list of serials that have routing lists attach. A search option or checkbox for this would be a nice addition.
Comment 1 Katrin Fischer 2023-10-18 08:14:16 UTC
This is for the advanced search form in the serials module:
https://staff.next.bsz-bw.de/cgi-bin/koha/serials/serials-home.pl

We'd like to have an option there to filter on subscriptions that are linked to a routing list. This could be realized as a checkbox or similar.
Comment 2 Danyon Sewell 2023-12-20 04:21:26 UTC
Created attachment 160094 [details] [review]
Bug 26567 WIP
Comment 3 Danyon Sewell 2023-12-20 04:45:09 UTC
Created attachment 160095 [details] [review]
Bug 26567: Allow to limit subscription search to subscriptions with routing lists

Test plan:
1. Apply this patch
2. Create two subscriptions, one with a routing list and one without
3. Navigate to Serials home and tick the checkbox labeled "Search routing lists only:"
4. Confirm that the only search result to appear is the subscription you added the routing list to

Sponsored by: Bibliotheksservice-Zentrum Baden-Wuerttemberg
Comment 4 Esther Melander 2023-12-22 18:51:52 UTC
Tested this and the search does return results. I added 7 patrons to a routing list for one serial. When I search the serials, I didn't fill out any fields but checked the routing list box. The search results are unexpected. My serial was listed 7 times. I would have expected only one search result because only one serial had a routing list

Is this search supposed to return serial titles with a routing list? If this is the case, there should have only been one search result.

Or is the search intended to list patrons on a routing list and the associated title? If this is the case, the patron name should have be included in the table of search results.
Comment 5 Katrin Fischer 2023-12-23 11:36:34 UTC
Hi Esther, thanks for testing! 

The subscription should only appear once in the list as we only want to check for "has a routing list".

We'll also be needing unit tests for the change in SearchSubscriptions.

I am also pondering if we should rename the search option... Maybe: "Has routing list"? This could make it clearer that we are not looking for routing lists, but for subscriptions still.
Comment 6 Danyon Sewell 2024-01-29 23:12:49 UTC
Created attachment 161617 [details] [review]
Bug 26567: Allow to limit subscription search to subscriptions with routing lists

Test plan:
1. Apply this patch
2. Create two subscriptions, one with a routing list and one without
3. Navigate to Serials home and tick the checkbox labeled "Search routing lists only:"
4. Confirm that the only search result to appear is the subscription you added the routing list to

Sponsored by: Bibliotheksservice-Zentrum Baden-Wuerttemberg
Comment 7 Danyon Sewell 2024-01-29 23:15:33 UTC
Created attachment 161618 [details] [review]
Bug 26567: Allow to limit subscription search to subscriptions with routing lists

Have fixed the bug where multiple results were showing for one subscription, have done testing with subscriptions that have differing numbers of patrons added to the routing list.

Test plan:
1. Apply this patch
2. Create two subscriptions, one with a routing list and one without
3. Navigate to Serials home and tick the checkbox labeled "Search routing lists only:"
4. Confirm that the only search result to appear is the subscription you added the routing list to

Sponsored by: Bibliotheksservice-Zentrum Baden-Wuerttemberg
Comment 8 Christian Stelzenmüller 2024-02-01 09:18:47 UTC
An error occured when trying to add two or more patrons to the routing list.

Error trace
C4::Serials::addroutingmember(): DBI Exception: DBD::mysql::st execute failed: Duplicate entry '2-19' for key 'subscriptionid' at /kohadevbox/koha/serials/routing.pl line 69
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77
Comment 9 Christian Stelzenmüller 2024-02-01 09:33:30 UTC
I'll take it back. 
I'll verified it without the patch, this error has nothing to do with the patch.
Comment 10 Danyon Sewell 2024-02-01 21:37:02 UTC
Created attachment 161718 [details] [review]
Bug 26567: Allow to limit subscription search to subscriptions with routing lists

unit tests have now been written for the SearchSubscription subroutine.

Test plan:
1. Apply this patch
2. Create two subscriptions, one with a routing list and one without
3. Navigate to Serials home and tick the checkbox labeled "Search routing lists only:"
4. Confirm that the only search result to appear is the subscription you added the routing list to

Sponsored by: Bibliotheksservice-Zentrum Baden-Wuerttemberg
Comment 11 Katrin Fischer 2024-02-05 10:22:45 UTC
Created attachment 161750 [details] [review]
Bug 26567: Allow to limit subscription search to subscriptions with routing lists

Adds a search option to the advanced search in the
serials module that allows to limit search on subscriptions
with routing lists.

Test plan:
1. Apply this patch
2. Create two subscriptions, one with a routing list and one without
3. Navigate to Serials home and tick the checkbox labeled "Search routing lists only:"
4. Confirm that the only search result to appear is the subscription you added the routing list to
5. Run unit tests: prove t/db_dependent/Serials.t

Sponsored by: Bibliotheksservice-Zentrum Baden-Wuerttemberg

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Perltidied changes to make QA test tools pass.
Comment 12 Katrin Fischer 2024-02-05 10:22:48 UTC
Created attachment 161751 [details] [review]
Bug 26567: (follow-up) Update label and add check for RoutingSerials

The system preference RoutingSerials controls if the routing list
feature is available in the staff interface or not. If routing lists
are deactivated, the search option should not show.

Also updated the label to read: "Has routing list"

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 13 Marcel de Rooy 2024-04-25 14:27:58 UTC
            q| INNER JOIN (SELECT DISTINCT subscriptionid FROM subscriptionroutinglist) srl ON srl.subscriptionid = subscription.subscriptionid|;
This looks a bit odd?

Couldnt you LEFT JOIN subscriptionroutinglist AND GROUP BY subscriptionid and WHERE srl.subscriptionid IS NOT NULL ?

Or without a GROUP BY something like:
LEFT JOIN subscriptionroutinglist srl ON srl.routingid = ( SELECT routingid FROM subscriptionroutinglist WHERE srl.subscriptionid=subscription.subscriptionid LIMIT 1 )
Comment 14 Katrin Fischer 2024-05-02 07:11:08 UTC
Isn't in Discussion a bit harsh for a question? ;)

Can you explain why the group or limit 1 is better than using a distinct?
Comment 15 Marcel de Rooy 2024-05-02 07:16:47 UTC
(In reply to Katrin Fischer from comment #14)
> Isn't in Discussion a bit harsh for a question? ;)
> 
> Can you explain why the group or limit 1 is better than using a distinct?

Isnt Failed QA a bit hard too? lol. Had no reason to fail here.

Counter question:
q| INNER JOIN (SELECT DISTINCT subscriptionid FROM subscriptionroutinglist) srl ON srl.subscriptionid = subscription.subscriptionid|;

Doesnt this looks a bit odd? Joining a subselect. Probably we c/should compare performance on the various constructs. But I gladly get a response from the author of doing that all by myself ;)
Comment 16 Katrin Fischer 2024-05-02 07:26:15 UTC
(In reply to Marcel de Rooy from comment #15)
> (In reply to Katrin Fischer from comment #14)
> > Isn't in Discussion a bit harsh for a question? ;)
> > 
> > Can you explain why the group or limit 1 is better than using a distinct?
> 
> Isnt Failed QA a bit hard too? lol. Had no reason to fail here.
> 
> Counter question:
> q| INNER JOIN (SELECT DISTINCT subscriptionid FROM subscriptionroutinglist)
> srl ON srl.subscriptionid = subscription.subscriptionid|;
> 
> Doesnt this looks a bit odd? Joining a subselect. Probably we c/should
> compare performance on the various constructs. But I gladly get a response
> from the author of doing that all by myself ;)

You will have to deal with me I am afraid. Our sponsorship for this has run out.
Comment 17 Katrin Fischer 2024-05-02 07:30:38 UTC
What construct would you prefer? I can try and provide a follow-up.
Comment 18 Katrin Fischer 2024-05-07 11:33:01 UTC
(In reply to Katrin Fischer from comment #17)
> What construct would you prefer? I can try and provide a follow-up.

Hi Marcel, did you have another chance to take a look yet?
Comment 19 Marcel de Rooy 2024-05-07 11:50:32 UTC
(In reply to Katrin Fischer from comment #18)
> (In reply to Katrin Fischer from comment #17)
> > What construct would you prefer? I can try and provide a follow-up.
> 
> Hi Marcel, did you have another chance to take a look yet?

On my list still, thx for your attention :)
Comment 20 Marcel de Rooy 2024-05-10 06:21:52 UTC
Created attachment 166489 [details] [review]
Bug 26567: Allow to limit subscription search to subscriptions with routing lists

Adds a search option to the advanced search in the
serials module that allows to limit search on subscriptions
with routing lists.

Test plan:
1. Apply this patch
2. Create two subscriptions, one with a routing list and one without
3. Navigate to Serials home and tick the checkbox labeled "Search routing lists only:"
4. Confirm that the only search result to appear is the subscription you added the routing list to
5. Run unit tests: prove t/db_dependent/Serials.t

Sponsored by: Bibliotheksservice-Zentrum Baden-Wuerttemberg

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Perltidied changes to make QA test tools pass.
Comment 21 Marcel de Rooy 2024-05-10 06:21:55 UTC
Created attachment 166490 [details] [review]
Bug 26567: (follow-up) Update label and add check for RoutingSerials

The system preference RoutingSerials controls if the routing list
feature is available in the staff interface or not. If routing lists
are deactivated, the search option should not show.

Also updated the label to read: "Has routing list"

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 22 Marcel de Rooy 2024-05-10 06:22:17 UTC
Created attachment 166491 [details] [review]
Bug 26567: Allow to limit subscription search to subscriptions with routing lists

Adds a search option to the advanced search in the
serials module that allows to limit search on subscriptions
with routing lists.

Test plan:
1. Apply this patch
2. Create two subscriptions, one with a routing list and one without
3. Navigate to Serials home and tick the checkbox labeled "Search routing lists only:"
4. Confirm that the only search result to appear is the subscription you added the routing list to
5. Run unit tests: prove t/db_dependent/Serials.t

Sponsored by: Bibliotheksservice-Zentrum Baden-Wuerttemberg

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Perltidied changes to make QA test tools pass.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2024-05-10 06:22:20 UTC
Created attachment 166492 [details] [review]
Bug 26567: (follow-up) Update label and add check for RoutingSerials

The system preference RoutingSerials controls if the routing list
feature is available in the staff interface or not. If routing lists
are deactivated, the search option should not show.

Also updated the label to read: "Has routing list"

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2024-05-10 06:24:20 UTC
(In reply to Katrin Fischer from comment #16)
> (In reply to Marcel de Rooy from comment #15)
> > (In reply to Katrin Fischer from comment #14)
> > > Isn't in Discussion a bit harsh for a question? ;)
> > > 
> > > Can you explain why the group or limit 1 is better than using a distinct?
> > 
> > Isnt Failed QA a bit hard too? lol. Had no reason to fail here.
> > 
> > Counter question:
> > q| INNER JOIN (SELECT DISTINCT subscriptionid FROM subscriptionroutinglist)
> > srl ON srl.subscriptionid = subscription.subscriptionid|;
> > 
> > Doesnt this looks a bit odd? Joining a subselect. Probably we c/should
> > compare performance on the various constructs. But I gladly get a response
> > from the author of doing that all by myself ;)
> 
> You will have to deal with me I am afraid. Our sponsorship for this has run
> out.

Still do not really like the construct, but have no formal argument.
Running out of sponsorship as a reason for not replying does not sound good to me btw. The author has a moral obligation too. Welcome to open source.
Comment 25 Katrin Fischer 2024-05-10 14:46:45 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 26 Chris Cormack 2024-05-12 08:37:19 UTC
(In reply to Marcel de Rooy from comment #24)
> (In reply to Katrin Fischer from comment #16)
> > (In reply to Marcel de Rooy from comment #15)
> > > (In reply to Katrin Fischer from comment #14)
> > > > Isn't in Discussion a bit harsh for a question? ;)
> > > > 
> > > > Can you explain why the group or limit 1 is better than using a distinct?
> > > 
> > > Isnt Failed QA a bit hard too? lol. Had no reason to fail here.
> > > 
> > > Counter question:
> > > q| INNER JOIN (SELECT DISTINCT subscriptionid FROM subscriptionroutinglist)
> > > srl ON srl.subscriptionid = subscription.subscriptionid|;
> > > 
> > > Doesnt this looks a bit odd? Joining a subselect. Probably we c/should
> > > compare performance on the various constructs. But I gladly get a response
> > > from the author of doing that all by myself ;)
> > 
> > You will have to deal with me I am afraid. Our sponsorship for this has run
> > out.
> 
> Still do not really like the construct, but have no formal argument.
> Running out of sponsorship as a reason for not replying does not sound good
> to me btw. The author has a moral obligation too. Welcome to open source.

This comment was unnecessary.