The module Koha::Borrower::Modifications depends on module C4::SQLHelper. However, the module SQLHelper is obsolete, so the calls for InsertInTable and UpdateInTable have to be replaced by the use of the module DBIx::Class.
Created attachment 29951 [details] [review] Bug 12623: SQLHelper replacement - Borrower::Modifications With this patch, the subroutines AddModification and ApproveModifications uses DBIx::Class instead of C4::SQLHelper. Test plan: 1) Apply the patch 2) Execute the unit tests by launching : prove t/db_dependent/Koha_borrower_modifications.t 3) The result has to be a success without error or warning : t/db_dependent/Koha_borrower_modifications.t .. ok All tests successful. Files=1, Tests=14, 2 wallclock secs ( 0.03 usr 0.01 sys + 1.60 cusr 0.08 csys = 1.72 CPU) Result: PASS
Created attachment 29980 [details] [review] Bug 12623: SQLHelper replacement - Borrower::Modifications With this patch, the subroutines AddModification and ApproveModifications uses DBIx::Class instead of C4::SQLHelper. Moreover, the tests has been wrapped in a transaction. Test plan: 1) Apply the patch 2) Execute the unit tests by launching : prove t/db_dependent/Koha_borrower_modifications.t 3) The result has to be a success without error or warning : t/db_dependent/Koha_borrower_modifications.t .. ok All tests successful. Files=1, Tests=14, 2 wallclock secs ( 0.03 usr 0.01 sys + 1.60 cusr 0.08 csys = 1.72 CPU) Result: PASS
Created attachment 30092 [details] [review] [SIGNED-OFF] Bug 12623: SQLHelper replacement - Borrower::Modifications With this patch, the subroutines AddModification and ApproveModifications uses DBIx::Class instead of C4::SQLHelper. Moreover, the tests has been wrapped in a transaction. Test plan: 1) Apply the patch 2) Execute the unit tests by launching : prove t/db_dependent/Koha_borrower_modifications.t 3) The result has to be a success without error or warning : t/db_dependent/Koha_borrower_modifications.t .. ok All tests successful. Files=1, Tests=14, 2 wallclock secs ( 0.03 usr 0.01 sys + 1.60 cusr 0.08 csys = 1.72 CPU) Result: PASS Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
1..14 ok 1 - Test AddModifications() and GetModifications() ok 2 - Test that Verify() succeeds with a valid token ok 3 - Test DelModifications() ok 4 - Test that Verify() method fails for a bad token ok 5 - Test GetPendingModificationsCount() ok 6 - Add a new pending modification and test GetPendingModificationsCount() again not ok 7 - Test GetPendingModifications() again # Failed test 'Test GetPendingModifications() again' # at t/db_dependent/Koha_borrower_modifications.t line 70. not ok 8 - Test GetPendingModifications() # Failed test 'Test GetPendingModifications()' # at t/db_dependent/Koha_borrower_modifications.t line 74. ok 9 - Test DenyModifications() ok 10 - Test ApproveModifications() removes pending modification from db ok 11 - Test ApproveModifications() applys modification to borrower ok 12 - Test GetPendingModificationsCount() ok 13 - Test ApproveModifications() removes pending modification from db, again ok 14 - Test ApproveModifications() applys modification to borrower, again # Looks like you failed 2 tests of 14.
This part depends on the order of surnames in the borrowers table: ## Check GetPendingModifications my $pending = Koha::Borrower::Modifications->GetPendingModifications(); print "ZZZ".$pending->[0]->{firstname}."\n"; ok( $pending->[0]->{'firstname'} eq 'Sandy', 'Test GetPendingModifications() again' ); ok( $pending->[1]->{'firstname'} eq 'Kyle', 'Test GetPendingModifications()' ); $pending is sorted by surname, firstname My borrower 2 and 3 apparently had the order of surnames just in reverse as you had them while testing. Please make your test independent of this circumstance..
Did the tests pass before applying the patch ? Because my patch doesn't change the previous unit tests, so if the tests failed before applying the patch, I don't kow what I could do to solve it.
(In reply to Yohann Dufour from comment #6) > Did the tests pass before applying the patch ? > Because my patch doesn't change the previous unit tests, so if the tests > failed before applying the patch, I don't kow what I could do to solve it. In some cases it will pass, in other cases it won't. Depends on the surnames of your borrowers. Now that we stumble over it, I suggest you fix it. I am confident that you can do it in a few lines..
Created attachment 30628 [details] [review] Bug 12623: SQLHelper replacement - Borrower::Modifications With this patch, the subroutines AddModification and ApproveModifications uses DBIx::Class instead of C4::SQLHelper. Moreover, the tests has been wrapped in a transaction. Test plan: 1) Apply the patch 2) Execute the unit tests by launching : prove t/db_dependent/Koha_borrower_modifications.t 3) The result has to be a success without error or warning : t/db_dependent/Koha_borrower_modifications.t .. ok All tests successful. Files=1, Tests=14, 2 wallclock secs ( 0.03 usr 0.01 sys + 1.60 cusr 0.08 csys = 1.72 CPU) Result: PASS Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 30629 [details] [review] Bug 12623: SQLHelper replacement - Borrower::Modifications With this patch, the subroutines AddModification and ApproveModifications uses DBIx::Class instead of C4::SQLHelper. Moreover, the tests has been wrapped in a transaction. Test plan: 1) Apply the patch 2) Execute the unit tests by launching : prove t/db_dependent/Koha_borrower_modifications.t 3) The result has to be a success without error or warning : t/db_dependent/Koha_borrower_modifications.t .. ok All tests successful. Files=1, Tests=14, 2 wallclock secs ( 0.03 usr 0.01 sys + 1.60 cusr 0.08 csys = 1.72 CPU) Result: PASS Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
QA Comment: The signoff from Kyle is still valid for me. Test passes now (for the surnames). Code looks good to me. No qa tools complaints. Passed QA
Patch pushed to master. Thanks Yohann!
This broke the patron details update request from the OPAC: DBIx::Class::Storage::DBI::_dbh_execute(): Duplicate entry '1' for key 'PRIMARY' at /home/katrin/kohaclone/Koha/Borrower/Modifications.pm line 63
In another test I got: DBIx::Class::Row::new(): No such column 'B_email' on Koha::Schema::Result::BorrowerModification at /home/katrin/kohaclone/Koha/Borrower/Modifications.pm line 63
(In reply to Katrin Fischer from comment #13) > In another test I got: > DBIx::Class::Row::new(): No such column 'B_email' on > Koha::Schema::Result::BorrowerModification at > /home/katrin/kohaclone/Koha/Borrower/Modifications.pm line 63 DBIx schema not up to date?
(In reply to M. de Rooy from comment #14) > (In reply to Katrin Fischer from comment #13) > > In another test I got: > > DBIx::Class::Row::new(): No such column 'B_email' on > > Koha::Schema::Result::BorrowerModification at > > /home/katrin/kohaclone/Koha/Borrower/Modifications.pm line 63 > > DBIx schema not up to date? This is broader than i first thought. Moved to its own bug 12781.
I've created bug 12784 for the bug about patron details update request from the OPAC.