Bug 7839

Summary: Enhancement: Show routing lists attached to patron in Patron record
Product: Koha Reporter: David Cook <dcook>
Component: PatronsAssignee: David Cook <dcook>
Status: CLOSED FIXED QA Contact: Paul Poulain <paul.poulain>
Severity: enhancement    
Priority: P5 - low CC: chris, f.demians, gmcharlt, nengard, paul.poulain
Version: 3.10   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: This patch adds a tab to the patron record that displays all the subscription routing lists to which the patron is linked.
Updated patch (w/ proper file permissions)
[SIGNED-OFF] Bug 7839 Enhancement: Show routing lists attached to patron in Patron record. Fixed permissions for routing-lists.pl.
Updated function name and SQL
Bug 7839 ENH: Show subscription routing lists attached to borrower in Patron record.
Bug 7839 : Follow up, fixing copyright
Bug 7839 : Follow up, fixing company copyright
Bug 7839 ENH: Add tab in patron record to show patron's routing lists
Bug 7839 ENH: Add tab in patron record to show patron's routing lists
Bug 7839 ENH: Add tab in patron record to show patron's routing lists
Signed-off patch

Description David Cook 2012-03-28 04:29:53 UTC
Adds a tab called "Routing Lists" into the Patron record between "Fines" and "Circulation History".

It lists all the subscription titles that the patron has routed to them. Each title has a link to the subscription record and a link to the edit window of the routing list.
Comment 1 David Cook 2012-04-04 01:27:56 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2012-04-04 02:52:57 UTC
David, this could go to Needs Signoff I think? Or do you think it needs discussion?
Comment 3 David Cook 2012-04-04 03:23:38 UTC
(In reply to comment #2)
> David, this could go to Needs Signoff I think? Or do you think it needs
> discussion?

I think it could go to Needs Signoff as well. I just wasn't sure about the process with something a bit more complex than my last patch.
Comment 4 Jared Camins-Esakov 2012-05-12 14:40:13 UTC
When I apply this patch and try to view the new page, I get a 500 error, and the following message in the logs:

[Sat May 12 10:23:57 2012] [error] [client 192.168.56.1] (13)Permission denied: exec of '/home/jcamins/kohaclone/members/routing-lists.pl' failed, referer: http://192.168.56.57:8080/cgi-bin/koha/members/moremember.pl?borrowernumber=217
[Sat May 12 10:23:57 2012] [error] [client 192.168.56.1] Premature end of script headers: routing-lists.pl, referer: http://192.168.56.57:8080/cgi-bin/koha/members/moremember.pl?borrowernumber=217
Comment 5 David Cook 2012-05-13 23:13:25 UTC
Hi Jared:

Check the security settings for routing-lists.pl on your server. You should have it set so that your public/everyone users can Read & Execute this file. I can assure you that you're getting the "(13)Permission denied: exec of..." error, because your users can only Read the file. 

(In reply to comment #4)
> When I apply this patch and try to view the new page, I get a 500 error, and
> the following message in the logs:
> 
> [Sat May 12 10:23:57 2012] [error] [client 192.168.56.1] (13)Permission
> denied: exec of '/home/jcamins/kohaclone/members/routing-lists.pl' failed,
> referer:
> http://192.168.56.57:8080/cgi-bin/koha/members/moremember.
> pl?borrowernumber=217
> [Sat May 12 10:23:57 2012] [error] [client 192.168.56.1] Premature end of
> script headers: routing-lists.pl, referer:
> http://192.168.56.57:8080/cgi-bin/koha/members/moremember.
> pl?borrowernumber=217
Comment 6 Chris Cormack 2012-05-13 23:15:56 UTC
(In reply to comment #5)
> Hi Jared:
> 
> Check the security settings for routing-lists.pl on your server. You should
> have it set so that your public/everyone users can Read & Execute this file.
> I can assure you that you're getting the "(13)Permission denied: exec of..."
> error, because your users can only Read the file. 

You are right :)

But it is your patch creating the file with 644

create mode 100644 members/routing-lists.pl

So you might want to chmod +x it in your repo, git add, git commit --amend and send a new patch that sets the right permissions. Or everyone will face this problem on install.

Chris
Comment 7 David Cook 2012-05-14 05:55:10 UTC Comment hidden (obsolete)
Comment 8 David Cook 2012-05-14 05:56:54 UTC
Thanks for pointing that out, Chris! Much appreciated.

(In reply to comment #6)
> 
> You are right :)
> 
> But it is your patch creating the file with 644
> 
> create mode 100644 members/routing-lists.pl
> 
> So you might want to chmod +x it in your repo, git add, git commit --amend
> and send a new patch that sets the right permissions. Or everyone will face
> this problem on install.
> 
> Chris
Comment 9 Nicole C. Engard 2012-05-14 12:25:43 UTC Comment hidden (obsolete)
Comment 10 Paul Poulain 2012-05-28 12:10:56 UTC
QA comments:
 1 the sub "get" in "getAttachedSubscriptions" is improperly spelled. It should be GetAttachedSubscriptions 
 2 the sub name is wrong I think:
    - Get...Subscriptions let think it returns a list of subscriptions, that's the case, that's OK
    - the "Attached" keyword is related to nothing. And there is no reference to the borrowernumber or routinglist thing.
   This sub should have another name. Something like GetSubscriptionsFromRoutinglistSubscriber or GetSubscriptionsFromBorrower
 3 the
+ JOIN subscriptionroutinglist ON subscriptionroutinglist.subscriptionid = subscription.subscriptionid
could be written
+ JOIN subscriptionroutinglist ON USING(subscriptionid)
(not requested for passed QA status, but shorter and should be used as much as possible in the code)
 4 the new script and C4/Serials.pm passes perlcritic
 5 prove xt/author/translatable-templates.t xt/author/valid-templates.t  is OK as well

Please provide a follow-up or an updated fix for point 1, 2 and optionnally 3 to earn passed QA status (and be the 183th committer to join the list of Koha committers ;-) )
Comment 11 Paul Poulain 2012-05-28 12:11:22 UTC
(I forgot to add that it's an interesting feature !)
Comment 12 David Cook 2012-05-31 00:19:40 UTC
(In reply to comment #10)
> QA comments:
>  1 the sub "get" in "getAttachedSubscriptions" is improperly spelled. It
> should be GetAttachedSubscriptions 
>  2 the sub name is wrong I think:
>     - Get...Subscriptions let think it returns a list of subscriptions,
> that's the case, that's OK
>     - the "Attached" keyword is related to nothing. And there is no
> reference to the borrowernumber or routinglist thing.
>    This sub should have another name. Something like
> GetSubscriptionsFromRoutinglistSubscriber or GetSubscriptionsFromBorrower
>  3 the
> + JOIN subscriptionroutinglist ON subscriptionroutinglist.subscriptionid =
> subscription.subscriptionid
> could be written
> + JOIN subscriptionroutinglist ON USING(subscriptionid)
> (not requested for passed QA status, but shorter and should be used as much
> as possible in the code)
>  4 the new script and C4/Serials.pm passes perlcritic
>  5 prove xt/author/translatable-templates.t xt/author/valid-templates.t  is
> OK as well
> 
> Please provide a follow-up or an updated fix for point 1, 2 and optionnally
> 3 to earn passed QA status (and be the 183th committer to join the list of
> Koha committers ;-) )

Sounds good, Paul. It's been a busy few days, but I'll be sure to get on that when I find the time!

As for the list of Koha committers, I think that you already put me on there as the 178th ;).
Comment 13 David Cook 2012-06-29 04:17:05 UTC Comment hidden (obsolete)
Comment 14 David Cook 2012-06-29 04:19:49 UTC Comment hidden (obsolete)
Comment 15 Chris Cormack 2012-06-29 08:02:37 UTC Comment hidden (obsolete)
Comment 16 David Cook 2012-07-01 23:22:47 UTC Comment hidden (obsolete)
Comment 17 David Cook 2012-07-02 00:21:55 UTC Comment hidden (obsolete)
Comment 18 Frédéric Demians 2012-07-10 06:37:13 UTC Comment hidden (obsolete)
Comment 19 Frédéric Demians 2012-07-10 06:38:15 UTC
It doesn't work. You SQL statement has a typo. You have:

  JOIN subscriptionroutinglist ON USING(subscriptionid)

You need:

  JOIN subscriptionroutinglist USING(subscriptionid)

After this fix, the new Routing Lists tab displays properly patron
routing lists. It's great.

When Routing Lists tab is selected, under the Edit-Change password
buttons bar, there is a search form.  When running such a search on
subcription, the search result is displayed in a new browser tab. The
search form has a target="_blank" attribute. This is not something which
is done elsewhere in Koha, and it's generally considered has a 'bad
practice'. It means that you have to close the new browser tab in order
to come back to the patron page, and then refresh by yourself the page,
if you want to see what you've done in serial module. Same issue with
the Edit Routing List link which open a new browser tab.

For me, this point is a matter of discussion.
Comment 20 David Cook 2012-07-10 06:53:35 UTC
Thanks for the typo fix! 

I see what you're saying about the "_blank" attribute. As I look over other parts of Koha, I see that everything stays in the same browser tab. For the sake of consistency, it's probably best to keep the same behaviour here as well.

Plus, as you say, it's bad practice in general, especially as there is no warning that it opens a new tab. I'd be willing to nix the "_blank" targets from the search form and the links.



(In reply to comment #19)
> It doesn't work. You SQL statement has a typo. You have:
> 
>   JOIN subscriptionroutinglist ON USING(subscriptionid)
> 
> You need:
> 
>   JOIN subscriptionroutinglist USING(subscriptionid)
> 
> After this fix, the new Routing Lists tab displays properly patron
> routing lists. It's great.
> 
> When Routing Lists tab is selected, under the Edit-Change password
> buttons bar, there is a search form.  When running such a search on
> subcription, the search result is displayed in a new browser tab. The
> search form has a target="_blank" attribute. This is not something which
> is done elsewhere in Koha, and it's generally considered has a 'bad
> practice'. It means that you have to close the new browser tab in order
> to come back to the patron page, and then refresh by yourself the page,
> if you want to see what you've done in serial module. Same issue with
> the Edit Routing List link which open a new browser tab.
> 
> For me, this point is a matter of discussion.
Comment 21 Frédéric Demians 2012-07-10 07:06:03 UTC
> Plus, as you say, it's bad practice in general, especially as there is
> no warning that it opens a new tab. I'd be willing to nix the "_blank"
> targets from the search form and the links.

I'd be pleased to sign-off, after others, such a fixed patch, since
otherwise everything seems ok.
Comment 22 David Cook 2012-07-10 23:38:00 UTC Comment hidden (obsolete)
Comment 23 Frédéric Demians 2012-07-11 04:51:44 UTC
Created attachment 10763 [details] [review]
Signed-off patch

Thanks. Works as described. Valuable addition on patron information
page.

Ideas of enhancement:

- For some library, and some patron, the routing lists table could be
  very long, and a paged table would be a plus. 
- In the table, a new Vendor column.
- The possibility to sort the table entries by Title and Vendor column.
Comment 24 Paul Poulain 2012-08-02 10:05:17 UTC
QA comment:
 * passes perlcritic, tt_valid, valid_templates
 * does not respect "(HTML4) We have decided to use upper cases only for the first letter of a given sentence/label/tab description/page title/..." Sometimes I put "failed QA", sometimes I fix myself. As it's a simple template, i've fixed

passed QA

The number of tabs attached to a given patron is quite large (Check out, Details, Fines, Routing lists, Circulation history, Modification log, Notices, Statistics, Files)
We will have to find a solution for a more useable display...