While trying to write tests (bug 10895), I was frustrated for an excessively long time while trying to copy t/db_dependent/Members.t as a template. The problem became apparent when I actually stop to note the difference between a :: over a -> in the calling convention. C4::Context->method passes the object as the first parameter. C4::Context::function calls the routine in a purely functional way. C4::Context methods should be called in an object oriented manner, particularly things like C4::Context::set_userenv. Set/Get are very much OO things! As pieces get fixed, the patches will drop here.
Created attachment 21152 [details] [review] Bug 10900 - Incorrect calling conventions accessing C4::Context There were multiple calling conventions for C4::Context's set_userenv routine. So the following commands were used to find discrepancies: grep "C4::Context::set_userenv" `find .` grep "C4::Context->set_userenv" `find .` It was recommended that it should be ->, and so a shift was added to the routine to properly ignore the object that is passed.
The validity of the change was checked with: $ prove -v t/db_dependent/Circulation.t If it works for that case, it should work for all cases. I was able to log in, and seeing as that set_userenv is in Auth, I figured it got triggered there too. I had a DB that was two versions behind, and I triggered an upgrade on the staff client successfully too.
How to trigger all the set_userenv's in C4/Auth.pm: log in using userid (triggers 1, 2, 6) log in using cardnumber (triggers 7 and others) library-intra.mydnsname.org/cgi-bin/koha/svc/bib/{bibnumber}?userid={userid}&password={password} (first time triggers 4) library-intra.mydnsname.org/cgi-bin/koha/svc/bib/{bibnumber} (once logged in triggering 4, this triggers 3) tools -> tags -> type something in the "Check lists" text box and click 'test'. (triggers 5) To trigger the set_userenv's in installer/InstallAuth.pm, just run an upgrade of a database from a previous version to the current master, and redirect to the log in screen. And having figured all this out... It worked.
Created attachment 21263 [details] [review] Bug 10900 - Incorrect calling conventions accessing C4::Context As the results of trying to roll a distribution, t/Circulation_bardcodedecode.t was discovered to be faulty. The cause being incorrect parameters! This was hidden when there was no shift in the set_userenv routine. However, with its correction, the test broke. This led me to read the POD documentation for the function set_userenv in C4::Context and realize that was outdated as well. It has been revised to match the current version of the function.
A little more to come... Another set of side related problems. *sigh*
Created attachment 21290 [details] [review] Correct calling convention for set_userenv
Created attachment 21291 [details] [review] Fix t/Circulation_barcodedecode.t set_userenv calls
Created attachment 21292 [details] [review] Fix other set_userenv calls with bad parameters
How to trigger all the set_userenv's in C4/Auth.pm: log in using userid (triggers 1, 2, 6) log in using cardnumber (triggers 7 and others) library-intra.mydnsname.org/cgi-bin/koha/svc/bib/{bibnumber}?userid={userid}&password={password} (first time triggers 4) library-intra.mydnsname.org/cgi-bin/koha/svc/bib/{bibnumber} (once logged in triggering 4, this triggers 3) tools -> tags -> type something in the "Check lists" text box and click 'test'. (triggers 5) To trigger the set_userenv's in installer/InstallAuth.pm, just run an upgrade of a database from a previous version to the current master, and redirect to the log in screen. prove -v t/Circulation_barcodedecode.t I have yet to determine how to trigger the patched areas in the third patch.
Okay, I believe there are 19 change points in these three patches. To trigger these change points (ie. Here is a test plan): koha staff client - login userid - 2,6,7,8,14 koha staff client - logout - 1 koha staff client - login cardnumber - 9,10,11 prove -v t/Circulation_barcodedecode.t - 18 prove -v t/db_dependent/Circulation.t - 19 http://{staff client url}/cgi-bin/koha/svc/bib/{biblionumber}?userid={userid}&password={password} - 4 koha staff client - login userid AND then http://{staff client url}/cgi-bin/koha/svc/bib/{biblionumber} - 3 tools -> tags -> type something in the "Check lists" text box and click 'test'. - 5 koha staff client - login system user - 12 perldoc C4::Context /set_userenv - 13 koha staff client - upgrade database - 16,17,15
Patch applied cleanly, go forth and signoff
I'm not sure this is correct. C4::Context is not an object, hence has no methods. I believe :: is more appropriate under the circumstances. Although we may agree to call C4::Context subs using ->, but that would be just a waste of the first argument. Context *should* be an object, I agree, but unfortunately it is not. Is this being addressed in Koha:: namespace?
I don't know if it is being addressed in the Koha:: namespace. However, if someone is working on splitting C4::Context, this means that one piece of the split up is closer to being the object it should be. See bug 7176.
Ok, then I'll rephrase: If Context is moving to Koha, then this is not necessary. If it is not, well then I can see the point.
Created attachment 22037 [details] [review] Bug 10900 - Incorrect calling conventions accessing C4::Context There were multiple calling conventions for C4::Context's set_userenv routine. So the following commands were used to find discrepancies: grep "::set_userenv" `find .` grep "\->set_userenv" `find .` The first grep demonstrated that the smaller change is from :: to -> as only C4/Auth.pm, installer/InstallAuth.pm, and t/db_dependent/Circulation.t would need to be modified. This patch corrects C4::Context's set_userenv routine to be object call based (use ->) by using a shift to ignore the first parameter, and modify the three files found with :: calls. As the result of trying to roll a distribution, t/Circulation_barcodedecode.t was discovered to be faulty. The cause being incorrect parameters! This was hidden when there was no shift in the set_userenv routine. However, with its correction, the test broke. This led me to read the POD documentation for the function set_userenv in C4::Context and realize it was outdated as well. It has been revised to match the current version of the function. Then intentionally bad parameters passed to the sent_userenv routine in C4::Context were hunted down. The biggest problems were missing surnames or branch names.
Had to rebase due to the changed hashing algorithm of the passwords patch which has made it into master.
Created attachment 22038 [details] [review] Bug 10900 - Incorrect calling conventions accessing C4::Context There were multiple calling conventions for C4::Context's set_userenv routine. So the following commands were used to find discrepancies: grep "::set_userenv" `find .` grep "\->set_userenv" `find .` The first grep demonstrated that the smaller change is from :: to -> as only C4/Auth.pm, installer/InstallAuth.pm, and t/db_dependent/Circulation.t would need to be modified. This patch corrects C4::Context's set_userenv routine to be object call based (use ->) by using a shift to ignore the first parameter, and modify the three files found with :: calls. As the result of trying to roll a distribution, t/Circulation_barcodedecode.t was discovered to be faulty. The cause being incorrect parameters! This was hidden when there was no shift in the set_userenv routine. However, with its correction, the test broke. This led me to read the POD documentation for the function set_userenv in C4::Context and realize it was outdated as well. It has been revised to match the current version of the function. Then intentionally bad parameters passed to the set_userenv routine in C4::Context were hunted down. The biggest problems were missing surnames or branch names.
The test plan is in comment #10.
Looks like this patch is causing Circulation_barcodedecode.t to fails some tests. All the unit tests pass on master. perl t/Circulation_barcodedecode.t 1..26 ok 1 - use C4::Circulation; ok 2 - EAN13: '892685001928' => '0892685001928' ok 3 - EAN13: '695152' => '0000000695152' ok 4 - T-prefix: 'T0031472' => 'T0031472' ok 5 - T-prefix: 'T32' => 'T0000002' ok 6 - cuecat: '26002315' => '26002315' ok 7 - cuecat: '.C3nZC3nZC3nYD3b6ENnZCNnY.fHmc.C3D1Dxr2C3nZE3n7.' => '046675000808' ok 8 - cuecat: '.C3nZC3nZC3nYD3b6ENnZCNnY.fHmc.C3D1Dxr2C3nZE3n7. # ' => '046675000808' ok 9 - cuecat: 'q.C3nZC3nZC3nWDNzYDxf2CNnY.fHmc.C3DWC3nZCNjXD3nW.' => '043000112403' ok 10 - cuecat: '.C3nZC3nZC3nWCxjWE3D1C3nX.cGf2.ENr7C3v7D3T3ENj3C3zYDNnZ.' => '978068484914051500' not ok 11 - libsuite8: 'b000126' => 'IMS-b-126' # Failed test ' libsuite8: 'b000126' => 'IMS-b-126'' # at t/Circulation_barcodedecode.t line 38. # Bad output: 'kale-b-126' not ok 12 - libsuite8: 'b12' => 'IMS-b-12' # Failed test ' libsuite8: 'b12' => 'IMS-b-12'' # at t/Circulation_barcodedecode.t line 38. # Bad output: 'kale-b-12' not ok 13 - libsuite8: 'B0126' => 'IMS-B-126' # Failed test ' libsuite8: 'B0126' => 'IMS-B-126'' # at t/Circulation_barcodedecode.t line 38. # Bad output: 'kale-B-126' not ok 14 - libsuite8: 'IMS-B-126' => 'IMS-B-126' # Failed test ' libsuite8: 'IMS-B-126' => 'IMS-B-126'' # at t/Circulation_barcodedecode.t line 38. # Bad output: 'kale-IMS-B--126' not ok 15 - libsuite8: 'ims-b-126' => 'ims-b-126' # Failed test ' libsuite8: 'ims-b-126' => 'ims-b-126'' # at t/Circulation_barcodedecode.t line 38. # Bad output: 'kale-ims-b--126' not ok 16 - libsuite8: 'CD0000024' => 'IMS-CD-24' # Failed test ' libsuite8: 'CD0000024' => 'IMS-CD-24'' # at t/Circulation_barcodedecode.t line 38. # Bad output: 'kale-CD-24' not ok 17 - libsuite8: '00123' => 'IMS-b-123' # Failed test ' libsuite8: '00123' => 'IMS-b-123'' # at t/Circulation_barcodedecode.t line 38. # Bad output: 'kale-b-123' not ok 18 - libsuite8: '11998' => 'IMS-b-11998' # Failed test ' libsuite8: '11998' => 'IMS-b-11998'' # at t/Circulation_barcodedecode.t line 38. # Bad output: 'kale-b-11998' ok 19 - other: '26002315' => '26002315' ok 20 - other: 'T0031472' => 'T0031472' ok 21 - other: 'T32' => 'T32' ok 22 - other: 'Alphanum123' => 'Alphanum123' ok 23 - other: 'Alpha Num 345' => 'Alpha Num 345' ok 24 - whitespace: ' 26002315' => '26002315' ok 25 - whitespace: '26002315 ' => '26002315' ok 26 - whitespace: ' # 26002315 # ' => '26002315' # Looks like you failed 8 tests of 26.
After reading your post, Kyle, I did a fresh git install on a fresh OS clone. I could not replicate your failure. mtompset@ubuntu:~/kohaclone$ git checkout -b bug_10900 origin/master Branch bug_10900 set up to track remote branch master from origin. Switched to a new branch 'bug_10900' mtompset@ubuntu:~/kohaclone$ git diff origin/master mtompset@ubuntu:~/kohaclone$ git bz apply 10900 Bug 10900 - Incorrect calling conventions accessing C4::Context Bug 10900 - Incorrect calling conventions accessing C4::Context Apply? [yn] y Applying: Bug 10900 - Incorrect calling conventions accessing C4::Context mtompset@ubuntu:~/kohaclone$ prove -v t/Circulation_barcodedecode.t t/Circulation_barcodedecode.t .. 1..26 ok 1 - use C4::Circulation; ok 2 - EAN13: '892685001928' => '0892685001928' ok 3 - EAN13: '695152' => '0000000695152' ok 4 - T-prefix: 'T0031472' => 'T0031472' ok 5 - T-prefix: 'T32' => 'T0000002' ok 6 - cuecat: '26002315' => '26002315' ok 7 - cuecat: '.C3nZC3nZC3nYD3b6ENnZCNnY.fHmc.C3D1Dxr2C3nZE3n7.' => '046675000808' ok 8 - cuecat: '.C3nZC3nZC3nYD3b6ENnZCNnY.fHmc.C3D1Dxr2C3nZE3n7. # ' => '046675000808' ok 9 - cuecat: 'q.C3nZC3nZC3nWDNzYDxf2CNnY.fHmc.C3DWC3nZCNjXD3nW.' => '043000112403' ok 10 - cuecat: '.C3nZC3nZC3nWCxjWE3D1C3nX.cGf2.ENr7C3v7D3T3ENj3C3zYDNnZ.' => '978068484914051500' ok 11 - libsuite8: 'b000126' => 'IMS-b-126' ok 12 - libsuite8: 'b12' => 'IMS-b-12' ok 13 - libsuite8: 'B0126' => 'IMS-B-126' ok 14 - libsuite8: 'IMS-B-126' => 'IMS-B-126' ok 15 - libsuite8: 'ims-b-126' => 'ims-b-126' ok 16 - libsuite8: 'CD0000024' => 'IMS-CD-24' ok 17 - libsuite8: '00123' => 'IMS-b-123' ok 18 - libsuite8: '11998' => 'IMS-b-11998' ok 19 - other: '26002315' => '26002315' ok 20 - other: 'T0031472' => 'T0031472' ok 21 - other: 'T32' => 'T32' ok 22 - other: 'Alphanum123' => 'Alphanum123' ok 23 - other: 'Alpha Num 345' => 'Alpha Num 345' ok 24 - whitespace: ' 26002315' => '26002315' ok 25 - whitespace: '26002315 ' => '26002315' ok 26 - whitespace: ' # 26002315 # ' => '26002315' ok All tests successful. Files=1, Tests=26, 0 wallclock secs ( 0.01 usr 0.01 sys + 0.30 cusr 0.02 csys = 0.34 CPU) Result: PASS So, I looked at your bad output line: # Bad output: 'kale-b-126' 'kale' is the last name in the set_userenv call. The line that changed in the test file is: C4::Context->set_userenv(1,'kmkale' , 1, 'km', 'kale' , 'IMS', 'IMS Branch DEscription', 0, 'kmkale@anantcorp.com'); This would be affected if the set_userenv function in C4::Context is missing shift @_; or the call is :: in the test file. From C4::Context: shift @_; my ($usernum, $userid, $usercnum, $userfirstname, $usersurname, $userbranch, $branchname, $userflags, $emailaddress, $branchprinter, $persona)= @_; The -> passes an object, which is shifted off, and $usernum=1,$userid='kmkale',$usercnum=1,$userfirstname='km',$usersurname='kale',$userbranch='IMS' The barcodedecode function uses the userbranch. So, if there is a missing shift @_ and the Circulationbarcode.t file has ->set_userenv(...) as above, then that $userbranch would be 'kale', which matches your failed output. The 'kale-' portion is based on the branchcode. From C4/Circulation.pm sub barcodedecode { my ($barcode, $filter) = @_; my $branch = C4::Branch::mybranch(); ... } elsif ($filter eq 'libsuite8') { unless($barcode =~ m/^($branch)-/i){ #if barcode starts with branch code its in Koha style. Skip it. if($barcode =~ m/^(\d)/i){ #Some barcodes even start with 0's & numbers and are assumed to have b as the item type in the libsuite8 software $barcode =~ s/^[0]*(\d+)$/$branch-b-$1/i; }else{ $barcode =~ s/^(\D+)[0]*(\d+)$/$branch-$1-$2/i; } } From C4/Branch.pm # always returns a string for OK comparison via "eq" or "ne" sub mybranch { C4::Context->userenv or return ''; return C4::Context->userenv->{branch} || ''; }
Created attachment 22585 [details] [review] Bug 10900 - Incorrect calling conventions accessing C4::Context There were multiple calling conventions for C4::Context's set_userenv routine. So the following commands were used to find discrepancies: grep "::set_userenv" `find .` grep "\->set_userenv" `find .` The first grep demonstrated that the smaller change is from :: to -> as only C4/Auth.pm, installer/InstallAuth.pm, and t/db_dependent/Circulation.t would need to be modified. This patch corrects C4::Context's set_userenv routine to be object call based (use ->) by using a shift to ignore the first parameter, and modify the three files found with :: calls. As the result of trying to roll a distribution, t/Circulation_barcodedecode.t was discovered to be faulty. The cause being incorrect parameters! This was hidden when there was no shift in the set_userenv routine. However, with its correction, the test broke. This led me to read the POD documentation for the function set_userenv in C4::Context and realize it was outdated as well. It has been revised to match the current version of the function. Then intentionally bad parameters passed to the set_userenv routine in C4::Context were hunted down. The biggest problems were missing surnames or branch names. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
QA comment: All looks good, no regression found (I did not launch all UT). The persona login works. As Srdjan already said, I am not sure the set_userenv routine should be used as a method. But no importance when looking at the C4::Context module... I would have been nice to make the most of the opportunity to replace the parameter list with a hashref. Marked as Passed QA.
Created attachment 22937 [details] [review] Bug 10900 - Incorrect calling conventions accessing C4::Context There were multiple calling conventions for C4::Context's set_userenv routine. So the following commands were used to find discrepancies: grep "::set_userenv" `find .` grep "\->set_userenv" `find .` The first grep demonstrated that the smaller change is from :: to -> as only C4/Auth.pm, installer/InstallAuth.pm, and t/db_dependent/Circulation.t would need to be modified. This patch corrects C4::Context's set_userenv routine to be object call based (use ->) by using a shift to ignore the first parameter, and modify the three files found with :: calls. As the result of trying to roll a distribution, t/Circulation_barcodedecode.t was discovered to be faulty. The cause being incorrect parameters! This was hidden when there was no shift in the set_userenv routine. However, with its correction, the test broke. This led me to read the POD documentation for the function set_userenv in C4::Context and realize it was outdated as well. It has been revised to match the current version of the function. Then intentionally bad parameters passed to the set_userenv routine in C4::Context were hunted down. The biggest problems were missing surnames or branch names. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
(In reply to Jonathan Druart from comment #22) > I would have been nice to make the most of the opportunity to replace the > parameter list with a hashref. I think the fixing the parameters to hashrefs would be better done in bug 7176.
Created attachment 27729 [details] [review] Bug 10900 - Incorrect calling conventions accessing C4::Context There were multiple calling conventions for C4::Context's set_userenv routine. So the following commands were used to find discrepancies: grep "::set_userenv" `find .` grep "\->set_userenv" `find .` The first grep demonstrated that the smaller change is from :: to -> as only C4/Auth.pm, installer/InstallAuth.pm, and t/db_dependent/Circulation.t would need to be modified. This patch corrects C4::Context's set_userenv routine to be object call based (use ->) by using a shift to ignore the first parameter, and modify the three files found with :: calls. As the result of trying to roll a distribution, t/Circulation_barcodedecode.t was discovered to be faulty. The cause being incorrect parameters! This was hidden when there was no shift in the set_userenv routine. However, with its correction, the test broke. This led me to read the POD documentation for the function set_userenv in C4::Context and realize it was outdated as well. It has been revised to match the current version of the function. Then intentionally bad parameters passed to the set_userenv routine in C4::Context were hunted down. The biggest problems were missing surnames or branch names. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> NOTE: Rebased because of Bug 11077.
Punting to next release as it's tied to bug 10589.
Actually, it is tied to bug 10589 only in that it is required for it to work. This actually changes no functionality whatsoever. It tidies calls to functions in C4::Context, making them called the same way. That is, use -> instead of :: in C4::Context function calls, particularly this piece which corrects set_userenv. It also corrects a few bugs which are working by accident, because the parameter list is wrong.
Created attachment 28183 [details] [review] Bug 10900 - Follow up, since more has been added to master Booksellers.t and Koha_template_plugin_Branches.t both had function calls to ::set_userenv added to them. This patch handles those additions. TEST PLAN --------- 1) Branch a new git branch 2) prove -v t/db_dependent/Bookseller.t -- It should work. 3) prove -v t/db_dependent/Koha_template_plugin_Branches.t -- It should work. 4) Apply only the first patch. 5) Repeat steps 2 and 3. -- They should both FAIL! 6) Apply the second patch as well. 7) Repeat steps 2 and 3. -- The should both work.
Even if 10589 and 10895 don't make it into Koha, I think this refactor/clean up is still useful to do. There are some "works by mistake" changes in here.
Created attachment 30243 [details] [review] Bug 10900 - Incorrect calling conventions accessing C4::Context There were multiple calling conventions for C4::Context's set_userenv routine. So the following commands were used to find discrepancies: grep "::set_userenv" `find .` grep "\->set_userenv" `find .` The first grep demonstrated that the smaller change is from :: to -> as only C4/Auth.pm, installer/InstallAuth.pm, and t/db_dependent/Circulation.t would need to be modified. This patch corrects C4::Context's set_userenv routine to be object call based (use ->) by using a shift to ignore the first parameter, and modify the three files found with :: calls. As the result of trying to roll a distribution, t/Circulation_barcodedecode.t was discovered to be faulty. The cause being incorrect parameters! This was hidden when there was no shift in the set_userenv routine. However, with its correction, the test broke. This led me to read the POD documentation for the function set_userenv in C4::Context and realize it was outdated as well. It has been revised to match the current version of the function. Then intentionally bad parameters passed to the set_userenv routine in C4::Context were hunted down. The biggest problems were missing surnames or branch names. REBASED Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 30244 [details] [review] Bug 10900 - Follow up, since more has been added to master Booksellers.t and Koha_template_plugin_Branches.t both had function calls to ::set_userenv added to them. This patch handles those additions. TEST PLAN --------- 1) Branch a new git branch 2) prove -v t/db_dependent/Bookseller.t -- It should work. 3) prove -v t/db_dependent/Koha_template_plugin_Branches.t -- It should work. 4) Apply only the first patch. 5) Repeat steps 2 and 3. -- They should both FAIL! 6) Apply the second patch as well. 7) Repeat steps 2 and 3. -- The should both work. REBASED
Created attachment 31297 [details] [review] Bug 10900 - Follow up, since more has been added to master Booksellers.t and Koha_template_plugin_Branches.t both had function calls to ::set_userenv added to them. This patch handles those additions. TEST PLAN --------- 1) Branch a new git branch 2) prove -v t/db_dependent/Bookseller.t -- It should work. 3) prove -v t/db_dependent/Koha_template_plugin_Branches.t -- It should work. 4) Apply only the first patch. 5) Repeat steps 2 and 3. -- They should both FAIL! 6) Apply the second patch as well. 7) Repeat steps 2 and 3. -- The should both work. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 31298 [details] [review] Bug 10900: 2 occurrences more Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Sorry, I forgot this one. Need QA!
See http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10589#c34 for how this all relates piecewise to the other bugs.
On QAing the 3 bug reports: The Circulation UT file is broken: prove t/db_dependent/Circulation.t t/db_dependent/Circulation.t .. 2/51 Undefined subroutine &C4::Circulation::GetItem called at C4/Circulation.pm line 1204.
(In reply to Jonathan Druart from comment #36) > On QAing the 3 bug reports: > > The Circulation UT file is broken: > prove t/db_dependent/Circulation.t > t/db_dependent/Circulation.t .. 2/51 Undefined subroutine > &C4::Circulation::GetItem called at C4/Circulation.pm line 1204. Hmmm... this is not because of this bug report, which can be QA'd by itself. comment #10 comment #32 prove -v t/db_dependent/Members.t I will try to figure out the mess on the other two bugs. Setting this to Signed Off, since this bug can be QA'd seperately.
Created attachment 31719 [details] [review] Bug 10900 - Incorrect calling conventions accessing C4::Context There were multiple calling conventions for C4::Context's set_userenv routine. So the following commands were used to find discrepancies: grep "::set_userenv" `find .` grep "\->set_userenv" `find .` The first grep demonstrated that the smaller change is from :: to -> as only C4/Auth.pm, installer/InstallAuth.pm, and t/db_dependent/Circulation.t would need to be modified. This patch corrects C4::Context's set_userenv routine to be object call based (use ->) by using a shift to ignore the first parameter, and modify the three files found with :: calls. As the result of trying to roll a distribution, t/Circulation_barcodedecode.t was discovered to be faulty. The cause being incorrect parameters! This was hidden when there was no shift in the set_userenv routine. However, with its correction, the test broke. This led me to read the POD documentation for the function set_userenv in C4::Context and realize it was outdated as well. It has been revised to match the current version of the function. Then intentionally bad parameters passed to the set_userenv routine in C4::Context were hunted down. The biggest problems were missing surnames or branch names. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 31720 [details] [review] Bug 10900 - Follow up, since more has been added to master Booksellers.t and Koha_template_plugin_Branches.t both had function calls to ::set_userenv added to them. This patch handles those additions. TEST PLAN --------- 1) Branch a new git branch 2) prove -v t/db_dependent/Bookseller.t -- It should work. 3) prove -v t/db_dependent/Koha_template_plugin_Branches.t -- It should work. 4) Apply only the first patch. 5) Repeat steps 2 and 3. -- They should both FAIL! 6) Apply the second patch as well. 7) Repeat steps 2 and 3. -- The should both work. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 31721 [details] [review] Bug 10900: 2 occurrences more Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
I will try to add a rule in the qa tools when pushed.
Created attachment 34355 [details] [review] Bug 10900 - Incorrect calling conventions accessing C4::Context There were multiple calling conventions for C4::Context's set_userenv routine. So the following commands were used to find discrepancies: grep "::set_userenv" `find .` grep "\->set_userenv" `find .` The first grep demonstrated that the smaller change is from :: to -> as only C4/Auth.pm, installer/InstallAuth.pm, and t/db_dependent/Circulation.t would need to be modified. This patch corrects C4::Context's set_userenv routine to be object call based (use ->) by using a shift to ignore the first parameter, and modify the three files found with :: calls. As the result of trying to roll a distribution, t/Circulation_barcodedecode.t was discovered to be faulty. The cause being incorrect parameters! This was hidden when there was no shift in the set_userenv routine. However, with its correction, the test broke. This led me to read the POD documentation for the function set_userenv in C4::Context and realize it was outdated as well. It has been revised to match the current version of the function. Then intentionally bad parameters passed to the set_userenv routine in C4::Context were hunted down. The biggest problems were missing surnames or branch names. Rebase required because of shibboleth change in C4/Context.pm Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 34356 [details] [review] Bug 10900 - Follow up, since more has been added to master Booksellers.t and Koha_template_plugin_Branches.t both had function calls to ::set_userenv added to them. This patch handles those additions. TEST PLAN --------- 1) Branch a new git branch 2) prove -v t/db_dependent/Bookseller.t -- It should work. 3) prove -v t/db_dependent/Koha_template_plugin_Branches.t -- It should work. 4) Apply only the first patch. 5) Repeat steps 2 and 3. -- They should both FAIL! 6) Apply the second patch as well. 7) Repeat steps 2 and 3. -- The should both work. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 34357 [details] [review] Bug 10900: 2 occurrences more Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Because of the new shibboleth parameter to set_userenv in C4::Context, a rebase was required. There should be no other visible changes between the last two versions of " Bug 10900 - Incorrect calling conventions accessing C4::Context". Leaving as Passed QA.
Created attachment 35542 [details] [review] Bug 10900 - Incorrect calling conventions accessing C4::Context There were multiple calling conventions for C4::Context's set_userenv routine. So the following commands were used to find discrepancies: grep "::set_userenv" `find .` grep "\->set_userenv" `find .` The first grep demonstrated that the smaller change is from :: to -> as only C4/Auth.pm, installer/InstallAuth.pm, and t/db_dependent/Circulation.t would need to be modified. This patch corrects C4::Context's set_userenv routine to be object call based (use ->) by using a shift to ignore the first parameter, and modify the three files found with :: calls. As the result of trying to roll a distribution, t/Circulation_barcodedecode.t was discovered to be faulty. The cause being incorrect parameters! This was hidden when there was no shift in the set_userenv routine. However, with its correction, the test broke. This led me to read the POD documentation for the function set_userenv in C4::Context and realize it was outdated as well. It has been revised to match the current version of the function. Then intentionally bad parameters passed to the set_userenv routine in C4::Context were hunted down. The biggest problems were missing surnames or branch names. Rebase required because of shibboleth change in C4/Context.pm Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 35543 [details] [review] Bug 10900 - Follow up, since more has been added to master Booksellers.t and Koha_template_plugin_Branches.t both had function calls to ::set_userenv added to them. This patch handles those additions. TEST PLAN --------- 1) Branch a new git branch 2) prove -v t/db_dependent/Bookseller.t -- It should work. 3) prove -v t/db_dependent/Koha_template_plugin_Branches.t -- It should work. 4) Apply only the first patch. 5) Repeat steps 2 and 3. -- They should both FAIL! 6) Apply the second patch as well. 7) Repeat steps 2 and 3. -- The should both work. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 35544 [details] [review] Bug 10900: 2 occurrences more Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Rebased after auth.pm tidy in bug 13499
Pushed to Module Maintainers Branch - https://github.com/mrenvoize/Koha/tree/authentication
Patches pushed to master. Thanks Mark!