Bug 20456 - Remove the C4::Serials::GetSubscriptionsFromBorrower
Summary: Remove the C4::Serials::GetSubscriptionsFromBorrower
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Katrin Fischer
QA Contact: Alex Arnaud
URL:
Keywords:
Depends on: 20400
Blocks:
  Show dependency treegraph
 
Reported: 2018-03-20 23:35 UTC by Katrin Fischer
Modified: 2019-10-14 19:56 UTC (History)
6 users (show)

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


Attachments
Bug 20456: Switch routling list tab in staff to use Koha::Object (6.83 KB, patch)
2018-03-20 23:37 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20456: Remove the C4::Serials::GetSubscriptionsFromBorrower (2.23 KB, patch)
2018-03-20 23:37 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20456: Switch routling list tab in staff to use Koha::Object (6.88 KB, patch)
2018-04-20 21:46 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20456: Remove the C4::Serials::GetSubscriptionsFromBorrower (2.23 KB, patch)
2018-04-20 21:46 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20456: (follow-up) Fix number of subscriptions and position display (1.62 KB, patch)
2018-04-21 09:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20456: Switch routling list tab in staff to use Koha::Object (7.03 KB, patch)
2018-04-21 10:37 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 20456: Remove the C4::Serials::GetSubscriptionsFromBorrower (2.38 KB, patch)
2018-04-21 10:37 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 20456: (follow-up) Fix number of subscriptions and position display (1.77 KB, patch)
2018-04-21 10:38 UTC, Séverine Queune
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2018-03-20 23:35:51 UTC
C4::Serials::GetSubscriptionsFromBorrower is only used for the display of the routing list tab in staff. Bug 20400 implements this for the OPAC using Koha::Object. After switching staff the routine can be safely removed.
Comment 1 Katrin Fischer 2018-03-20 23:37:07 UTC
Created attachment 73119 [details] [review]
Bug 20456: Switch routling list tab in staff to use Koha::Object

Bug 20400 added a routing list tab to the patron account in the
OPAC using Koha::Object.

This patch switches the routing list tab in the patron account
in intranet over to the new code. It also adds an additional
column showing the position of the patron on the routing list
and fixes the search.

To test:
- Create some subscriptions with routing lists
- Take a look at the patron accounts of several patrons having
  - no entries on routing lists
  - 1 entry on a routing list
  - entries on several routing lists
- Make sure the display works correctly.
- Search for a subscription and make sure search works.
Comment 2 Katrin Fischer 2018-03-20 23:37:11 UTC
Created attachment 73120 [details] [review]
Bug 20456: Remove the C4::Serials::GetSubscriptionsFromBorrower

Bug 20400 added a routing list tab to the patron account in the
OPAC using Koha::Object. After changing the staff view too,
we can now safely remove GetSubscriptionsFromBorrower as it
is no longer used.

To test:
- Make sure that GetSubscriptionsFromBorrower is no longer used.
Comment 3 Katrin Fischer 2018-03-20 23:49:38 UTC
I assume this is a false positive as removing a method should reduce POD :)

 FAIL	C4/Serials.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   FAIL	  pod coverage
		POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666
   OK	  spelling
   OK	  valid
Comment 4 Katrin Fischer 2018-04-20 21:46:40 UTC
Created attachment 74667 [details] [review]
Bug 20456: Switch routling list tab in staff to use Koha::Object

Bug 20400 added a routing list tab to the patron account in the
OPAC using Koha::Object.

This patch switches the routing list tab in the patron account
in intranet over to the new code. It also adds an additional
column showing the position of the patron on the routing list
and fixes the search.

To test:
- Create some subscriptions with routing lists
- Take a look at the patron accounts of several patrons having
  - no entries on routing lists
  - 1 entry on a routing list
  - entries on several routing lists
- Make sure the display works correctly.
- Search for a subscription and make sure search works.
Comment 5 Katrin Fischer 2018-04-20 21:46:43 UTC
Created attachment 74668 [details] [review]
Bug 20456: Remove the C4::Serials::GetSubscriptionsFromBorrower

Bug 20400 added a routing list tab to the patron account in the
OPAC using Koha::Object. After changing the staff view too,
we can now safely remove GetSubscriptionsFromBorrower as it
is no longer used.

To test:
- Make sure that GetSubscriptionsFromBorrower is no longer used.
Comment 6 Séverine Queune 2018-04-21 08:43:28 UTC
No problem to use routing lists, but several display issues an patron's account :
  - the number of subscritpion lists always display "0", whatever the number of lists visible is
  - the position priority is always 1, even if the patron is not the first on list
  - routing lists are displayed even on patron's account that are not part of them

Probably should not write that point here, but "Your routing list" tab in the OPAC always leads to an "Internal server error" (bug 20400).
Comment 7 Katrin Fischer 2018-04-21 09:38:18 UTC
Created attachment 74675 [details] [review]
Bug 20456: (follow-up) Fix number of subscriptions and position display

TT variables needed to be adjusted because of changes from bug 20400.
Also: while we want logged_in_user in OPAC, we don't want that in staff.

1) Check the right number of subscriptions is displayed on top of the table
2) check the right position number is display for the routing list
Comment 8 Katrin Fischer 2018-04-21 09:41:38 UTC
(In reply to Séverine Queune from comment #6)

>   - the number of subscription lists always display "0", whatever the number
> of lists visible is

Thx for catching that - fixed!

>   - the position priority is always 1, even if the patron is not the first
> on list

Fixed!

>   - routing lists are displayed even on patron's account that are not part
> of them

The tab in staff is only dependent on the pref, not on the existence. I didn't touch that logic, could be a separate bug? But as we have the search option in staff, maybe we want to keep it?

> Probably should not write that point here, but "Your routing list" tab in
> the OPAC always leads to an "Internal server error" (bug 20400).

This is a hard one. I can see it in the sandbox, but I can't replicate it on my installation with the same conditions (patron with one routing list, being first position). Better to move that to bug 20400.
Comment 9 Séverine Queune 2018-04-21 10:37:55 UTC
Created attachment 74683 [details] [review]
Bug 20456: Switch routling list tab in staff to use Koha::Object

Bug 20400 added a routing list tab to the patron account in the
OPAC using Koha::Object.

This patch switches the routing list tab in the patron account
in intranet over to the new code. It also adds an additional
column showing the position of the patron on the routing list
and fixes the search.

To test:
- Create some subscriptions with routing lists
- Take a look at the patron accounts of several patrons having
  - no entries on routing lists
  - 1 entry on a routing list
  - entries on several routing lists
- Make sure the display works correctly.
- Search for a subscription and make sure search works.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 10 Séverine Queune 2018-04-21 10:37:58 UTC
Created attachment 74684 [details] [review]
Bug 20456: Remove the C4::Serials::GetSubscriptionsFromBorrower

Bug 20400 added a routing list tab to the patron account in the
OPAC using Koha::Object. After changing the staff view too,
we can now safely remove GetSubscriptionsFromBorrower as it
is no longer used.

To test:
- Make sure that GetSubscriptionsFromBorrower is no longer used.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 11 Séverine Queune 2018-04-21 10:38:01 UTC
Created attachment 74685 [details] [review]
Bug 20456: (follow-up) Fix number of subscriptions and position display

TT variables needed to be adjusted because of changes from bug 20400.
Also: while we want logged_in_user in OPAC, we don't want that in staff.

1) Check the right number of subscriptions is displayed on top of the table
2) check the right position number is display for the routing list

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 12 Séverine Queune 2018-04-21 10:47:25 UTC
No more count problems !

(In reply to Katrin Fischer from comment #8)
> (In reply to Séverine Queune from comment #6)
> 
> >   - routing lists are displayed even on patron's account that are not part
> > of them
> 
> The tab in staff is only dependent on the pref, not on the existence. I
> didn't touch that logic, could be a separate bug? But as we have the search
> option in staff, maybe we want to keep it?
> 
I didn't notice that before, as I only use routing list for tests and not in production.
Behaviour is now different, only the routing lists patron is on are now displayed (empty list for patron that have no routing list).
If that changes the original behaviour, maybe I shouldn't have signed it...
Comment 13 Alex Arnaud 2018-05-07 13:45:38 UTC
All is ok for me. Passed QA
Comment 14 Jonathan Druart 2018-05-07 15:18:23 UTC
String changes, skipping for 18.05
Comment 15 Nick Clemens 2018-06-22 18:10:29 UTC
Awesome work all!

Pushed to master for 18.11.x
Comment 16 Martin Renvoize 2018-06-27 09:31:18 UTC
Pushed to 18.05.x for 18.05.02

Thanks guys, I've made the decision to port this one back as it only just missed the 18.05.00 release due to string changes and doesn't fundamentally change functionality but rather tidies up code.
Comment 17 Fridolin Somers 2018-06-28 08:37:00 UTC
Depends on Bug 20400 not in 17.11.x