Description
David Cook
2012-03-28 04:29:53 UTC
Created attachment 8855 [details] [review] This patch adds a tab to the patron record that displays all the subscription routing lists to which the patron is linked. Minor changes were made to Serials.pm and circ-menu.inc. I also created routing-lists.pl and routing-lists.tt based using other pages in the patron record as models. David, this could go to Needs Signoff I think? Or do you think it needs discussion? (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. 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 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 (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 Created attachment 9550 [details] [review] Updated patch (w/ proper file permissions) 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 Created attachment 9559 [details] [review] [SIGNED-OFF] Bug 7839 Enhancement: Show routing lists attached to patron in Patron record. Fixed permissions for routing-lists.pl. Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com> Tested by viewing patrons who were and weren't on routing lists everyone looks a-ok. 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 ;-) ) (I forgot to add that it's an interesting feature !) (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 ;). Created attachment 10553 [details] [review] Updated function name and SQL I'm still getting accustomed to using the git, but hopefully this will do the trick. Created attachment 10554 [details] [review] Bug 7839 ENH: Show subscription routing lists attached to borrower in Patron record. Updated function name and SQL. Created attachment 10558 [details] [review] Bug 7839 : Follow up, fixing copyright Created attachment 10593 [details] [review] Bug 7839 : Follow up, fixing company copyright Created attachment 10594 [details] [review] Bug 7839 ENH: Add tab in patron record to show patron's routing lists Fixed the copyright and an accidental double quote bug in the template. Created attachment 10736 [details] [review] Bug 7839 ENH: Add tab in patron record to show patron's routing lists Fix a typo in SQL statement 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. 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. > 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.
Created attachment 10761 [details] [review] Bug 7839 ENH: Add tab in patron record to show patron's routing lists Fixed aforementioned SQL error and removed blank target references. 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. 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... |