Description
Chris Cormack
2012-10-14 22:15:41 UTC
Development sponsored by Albany Senior High School Created attachment 12831 [details] [review] Bug 8917 : Creating tables and system pref, no functional changes at all Created attachment 12832 [details] [review] Bug 8917 : Adding link tracking to the opac detail page, without xslt Created attachment 12833 [details] [review] Bug 8917 : Script to log link clicks Created attachment 12834 [details] [review] Bug 8917 : Link tracking, changes to XSLT To test Apply patches, ... nothing should change. Switch the TrackClicks syspref to anonymous. View a result with a link, the link should be changed, clicking on it should take you to the real link, but a row should be inserted into the linktracker table. The borrowernumber should be 0 Switch the syspref to Track Try the link again, if you are not logged in borrower will still be 0 If you are logged in, it should have your borrowernumber logged. When TrackClicks is set to track, clicking a link gives this error: Can't use string ("") as a HASH ref while "strict refs" in use at /home/koha/kohaclone/C4/Auth.pm line 1624. It does, however, work fine when set to track anonymously. Created attachment 12998 [details] [review] Bug 8917 : Creating tables and system pref, no functional changes at all Created attachment 12999 [details] [review] Bug 8917 : Adding link tracking to the opac detail page, without xslt Created attachment 13000 [details] [review] Bug 8917 : Script to log link clicks Created attachment 13001 [details] [review] Bug 8917 : Link tracking, changes to XSLT (In reply to comment #7) > When TrackClicks is set to track, clicking a link gives this error: > > Can't use string ("") as a HASH ref while "strict refs" in use at > /home/koha/kohaclone/C4/Auth.pm line 1624. > > It does, however, work fine when set to track anonymously. How exactly did you get this error? I can't recreate it, were you logged in at the time? Or not logged in? I have rebased all the patches against the current HEAD of master, so if you can let me know how you got the error ill try to recreate and fix it. > How exactly did you get this error? I can't recreate it, were you logged in
> at the time? Or not logged in?
>
> I have rebased all the patches against the current HEAD of master, so if you
> can let me know how you got the error ill try to recreate and fix it.
I tried it both logged in and logged out iirc. I'll have to retest and see if I still get those errors.
Tested, I get the error when I am logged in. I tried it as two separate users and got the same error each time. Can't use string ("") as a HASH ref while "strict refs" in use at /home/koha/kohaclone/C4/Auth.pm line 1625. If I'm not logged in, it works fine. The feature works for me. I can't reproduce the error describes by Kyle. Given that Kyle get an error, I don't sign-off this patch. Could someone else please test, Kyle is getting an error that currently neither Jonathan or I can reproduce. If someone else could test as well that would be great. I cannot apply the last patch. I'll attempt to retest the rest of them though. I think I'm giving up on testing this one. I just did a full reinstall from master, with only the default data. I changed nothing except to enable TrackClicks, and catalog an item with a URL, and I *still* get the error: Can't use string ("") as a HASH ref while "strict refs" in use at /home/koha/kohaclone/C4/Auth.pm line 1625. Created attachment 14058 [details] [review] Bug 8917 : Creating tables and system pref, no functional changes at all Created attachment 14059 [details] [review] Bug 8917 : Adding link tracking to the opac detail page, without xslt Created attachment 14060 [details] [review] Bug 8917 : Script to log link clicks Created attachment 14061 [details] [review] Bug 8917 : Link tracking, changes to XSLT Does not apply. Created attachment 15669 [details] [review] Bug 8917 : Creating tables and system pref, no functional changes at all Created attachment 15670 [details] [review] Bug 8917 : Adding link tracking to the opac detail page, without xslt Created attachment 15671 [details] [review] Bug 8917 : Script to log link clicks Created attachment 15672 [details] [review] Bug 8917 : Link tracking, changes to XSLT Well, I found Kyle's problem with logged in user. Software error: Can't use string ("") as a HASH ref while "strict refs" in use at /home/bgkriegel/kohaclone/C4/Auth.pm line 1682 Digging a little, I found: 1) in tracklinks.pl you call "checkauth( $cgi, 1, '', 'opac' );" 2) in C4/Auth.pm ^ THIS sub checkauth { my $query = shift; $debug and warn "Checking Auth"; # $authnotrequired will be set for scripts which will run without authentication my $authnotrequired = shift; my $flagsrequired = shift; <----------- HERE my $type = shift; my $persona = shift; $type = 'opac' unless $type; 3) some lines later, $flagsrequired is used in $flags = haspermission($userid, $flagsrequired); 4) more lines later, in haspermission() (1682) foreach my $module ( keys %$flagsrequired ) { And that's the error's line. 5) $flagsrequired need to be a hashref, you send '' 6) If I change 1) to checkauth( $cgi, 1, {}, 'opac' ); now Koha is happy and logs the correct user in linktracker table. (Perl (v5.14.2) built for x86_64-linux-gnu-thread-multi) Perhaps declaring args and then using them in checkout() would be cleaner and more understandable. 7) Not related, koha-qa complains of tabulations in updatedatabase.pl, I could fix that. (In reply to comment #28) > Perhaps declaring args and then using them in checkout() Sorry, I mean checkauth() Created attachment 15711 [details] [review] Bug 8917 : Fixing a bug bgkriegel spotted and removing tabs Created attachment 15712 [details] [review] [SIGNED-OFF] Bug 8917 : Creating tables and system pref, no functional changes at all Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 15713 [details] [review] [SIGNED-OFF] Bug 8917 : Adding link tracking to the opac detail page, without xslt Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 15714 [details] [review] [SIGNED_OFF] Bug 8917 : Script to log link clicks Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 15715 [details] [review] [SIGNED_OFF] Bug 8917 : Link tracking, changes to XSLT Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 15716 [details] [review] [SIGNED-OFF] Bug 8917 : Fixing a bug bgkriegel spotted and removing tabs Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off. Work as described. No errors. Tests 1) Syspref disabled, no tracking 2) Syspref enabled to anonymous, tracking with borrowernumber 0, anonymous search or logged in. 3) Syspref enabled to track, tracking, borrowernumber depends, 0 for anonymous search, borrowernumber for logged in user. Comment on attachment 15713 [details] [review] [SIGNED-OFF] Bug 8917 : Adding link tracking to the opac detail page, without xslt Review of attachment 15713 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt @@ +716,4 @@ > <br />[% END %] > <!-- here you might do a tmpl_if name="toc" and use greybox or equivalent for table of contents --> > > + [% IF ( MARCurl.OPACurlOpenInNewWindow ) %] This is always false. The variable to test is OPACurlOpenInNewWindow. Comment on attachment 15714 [details] [review] [SIGNED_OFF] Bug 8917 : Script to log link clicks Review of attachment 15714 [details] [review]: ----------------------------------------------------------------- ::: opac/tracklinks.pl @@ +65,5 @@ > + print $cgi->redirect("/cgi-bin/koha/errors/404.pl"); # escape early > + exit; > +} > + > +sub trackclick { Don't we want to have this routine into a new module (in order to prevent sql code into pl file)? Marked as Failed QA for the test on the wrong variable. Ahh someone must have fixed that since the patch was first made http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=12832&action=diff I'll change the patch and resubmit Created attachment 15783 [details] [review] Bug 8917 : Creating tables and system pref, no functional changes at all Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 15784 [details] [review] Bug 8917 : Adding link tracking to the opac detail page, without xslt Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 15785 [details] [review] Bug 8917 : Script to log link clicks Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 15786 [details] [review] Bug 8917 : Link tracking, changes to XSLT Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 15787 [details] [review] Bug 8917 : Fixing a bug bgkriegel spotted and removing tabs Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> (In reply to comment #38) > Comment on attachment 15714 [details] [review] [review] > [SIGNED_OFF] Bug 8917 : Script to log link clicks > > Review of attachment 15714 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: opac/tracklinks.pl > @@ +65,5 @@ > > + print $cgi->redirect("/cgi-bin/koha/errors/404.pl"); # escape early > > + exit; > > +} > > + > > +sub trackclick { > > Don't we want to have this routine into a new module (in order to prevent > sql code into pl file)? Hi Chris, You didn't address this remark, isn't it relevant? (In reply to comment #46) > (In reply to comment #38) > > Comment on attachment 15714 [details] [review] [review] [review] > > [SIGNED_OFF] Bug 8917 : Script to log link clicks > > > > Review of attachment 15714 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > ::: opac/tracklinks.pl > > @@ +65,5 @@ > > > + print $cgi->redirect("/cgi-bin/koha/errors/404.pl"); # escape early > > > + exit; > > > +} > > > + > > > +sub trackclick { > > > > Don't we want to have this routine into a new module (in order to prevent > > sql code into pl file)? > > Hi Chris, > You didn't address this remark, isn't it relevant? I didn't think it was, no. (In reply to comment #47) > (In reply to comment #46) > > (In reply to comment #38) > > > Comment on attachment 15714 [details] [review] [review] [review] [review] > > > [SIGNED_OFF] Bug 8917 : Script to log link clicks > > > > > > Review of attachment 15714 [details] [review] [review] [review] [review]: > > > ----------------------------------------------------------------- > > > > > > ::: opac/tracklinks.pl > > > @@ +65,5 @@ > > > > + print $cgi->redirect("/cgi-bin/koha/errors/404.pl"); # escape early > > > > + exit; > > > > +} > > > > + > > > > +sub trackclick { > > > > > > Don't we want to have this routine into a new module (in order to prevent > > > sql code into pl file)? > > > > Hi Chris, > > You didn't address this remark, isn't it relevant? > > I didn't think it was, no. So please set out your argument, mine is the coding guidelines : http://wiki.koha-community.org/wiki/Coding_Guidelines#SQL8:_SQL_code_in_.pl_scripts (In reply to comment #48) > (In reply to comment #47) > > (In reply to comment #46) > > > (In reply to comment #38) > > > > Comment on attachment 15714 [details] [review] [review] [review] [review] [review] > > > > [SIGNED_OFF] Bug 8917 : Script to log link clicks > > > > > > > > Review of attachment 15714 [details] [review] [review] [review] [review] [review]: > > > > ----------------------------------------------------------------- > > > > > > > > ::: opac/tracklinks.pl > > > > @@ +65,5 @@ > > > > > + print $cgi->redirect("/cgi-bin/koha/errors/404.pl"); # escape early > > > > > + exit; > > > > > +} > > > > > + > > > > > +sub trackclick { > > > > > > > > Don't we want to have this routine into a new module (in order to prevent > > > > sql code into pl file)? > > > > > > Hi Chris, > > > You didn't address this remark, isn't it relevant? > > > > I didn't think it was, no. > > So please set out your argument, mine is the coding guidelines : > http://wiki.koha-community.org/wiki/Coding_Guidelines#SQL8:_SQL_code_in_. > pl_scripts I no longer care, someone else can work on this Chris, I feel like you are upset. Did I offense you? If it is the case, I am sorry, I didn't mean it. I am just trying to do my QA job, and this job consists in following the coding guidelines. I just would like to understand why do you think this remark is no relevant, because I though i was. Created attachment 15812 [details] [review] Bug 8917 : Shifting db dependent code to a module Shifted the db insert to a new module. Spotted a bug with the behaviour of the links that existed before this new feature, even if we dont have a uri, we are putting an empty href in. Will do a follow up to fix this bug. Created attachment 15815 [details] [review] Bug 8917 : Follow up fixing bad behaviour that existed prior to this feature But the bug only becomes visible with link tracking on Prior to this patch it would put an empty link in the html Created attachment 15818 [details] [review] [SIGNED-OFF] Bug 8917 : Creating tables and system pref, no functional changes at all Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 15819 [details] [review] [SIGNED-OFF] Bug 8917 : Adding link tracking to the opac detail page, without xslt Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 15820 [details] [review] [SIGNED-OFF] Bug 8917 : Script to log link clicks Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 15821 [details] [review] [SIGNED-OFF] Bug 8917 : Link tracking, changes to XSLT Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 15822 [details] [review] [SIGNED-OFF] Bug 8917 : Fixing a bug bgkriegel spotted and removing tabs Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 15823 [details] [review] [SIGNED-OFF] Bug 8917 : Shifting db dependent code to a module Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 15824 [details] [review] [SIGNED-OFF] Bug 8917 : Follow up fixing bad behaviour that existed prior to this feature But the bug only becomes visible with link tracking on Prior to this patch it would put an empty link in the html Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> General comment: Work as described. No errors. Tested all preference values with and without logged in user. Created attachment 15825 [details] [review] Bug 8917 : [ Sign followup ] Track clicks to links external to Koha Remove empty pod section in Koha/Linktracker.pm to make koha-qa happy. Add missing "\n" in installer/data/mysql/updatedatabase.pl QA comment: This patch add a link tracker at the OPAC for the result and detail pages. The feature works as expected without XSLT (3 options tested). I didn't test the pages with XSLT, I am a unimarc user and this patch does not modify the unimarc XSLTs. Marked as Passed QA. Note: the update database message is not correct ("Adds feature Bug 8917, the ability to track links clicked"). Created attachment 15850 [details] [review] Bug 8917 : Creating tables and system pref, no functional changes at all Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 15851 [details] [review] Bug 8917 : Adding link tracking to the opac detail page, without xslt Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 15852 [details] [review] Bug 8917 : Script to log link clicks Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 15853 [details] [review] Bug 8917 : Link tracking, changes to XSLT Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 15854 [details] [review] Bug 8917 : Fixing a bug bgkriegel spotted and removing tabs Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 15855 [details] [review] Bug 8917 : Shifting db dependent code to a module Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 15856 [details] [review] Bug 8917 : Follow up fixing bad behaviour that existed prior to this feature But the bug only becomes visible with link tracking on Prior to this patch it would put an empty link in the html Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> General comment: Work as described. No errors. Tested all preference values with and without logged in user. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 15857 [details] [review] Bug 8917 : [ Sign followup ] Track clicks to links external to Koha Remove empty pod section in Koha/Linktracker.pm to make koha-qa happy. Add missing "\n" in installer/data/mysql/updatedatabase.pl Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 15858 [details] [review] Bug 8917: replaces tab with spaces Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> This patch has been pushed to master. Is somebody planning to write a followup bug for UNIMARC XSLT? M. Saby |