Bug 12623 - SQLHelper replacement - Koha::Borrower::Modifications
Summary: SQLHelper replacement - Koha::Borrower::Modifications
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Yohann Dufour
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 11385 12222
  Show dependency treegraph
 
Reported: 2014-07-22 08:41 UTC by Yohann Dufour
Modified: 2015-06-04 23:33 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 12623: SQLHelper replacement - Borrower::Modifications (3.99 KB, patch)
2014-07-22 10:09 UTC, Yohann Dufour
Details | Diff | Splinter Review
Bug 12623: SQLHelper replacement - Borrower::Modifications (4.89 KB, patch)
2014-07-23 12:26 UTC, Yohann Dufour
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12623: SQLHelper replacement - Borrower::Modifications (4.96 KB, patch)
2014-07-25 13:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12623: SQLHelper replacement - Borrower::Modifications (5.66 KB, patch)
2014-08-08 14:26 UTC, Yohann Dufour
Details | Diff | Splinter Review
Bug 12623: SQLHelper replacement - Borrower::Modifications (5.76 KB, patch)
2014-08-08 14:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Yohann Dufour 2014-07-22 08:41:41 UTC
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.
Comment 1 Yohann Dufour 2014-07-22 10:09:59 UTC Comment hidden (obsolete)
Comment 2 Yohann Dufour 2014-07-23 12:26:12 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2014-07-25 13:02:17 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2014-08-07 12:33:26 UTC
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.
Comment 5 Marcel de Rooy 2014-08-07 12:35:18 UTC
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..
Comment 6 Yohann Dufour 2014-08-07 12:40:00 UTC
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.
Comment 7 Marcel de Rooy 2014-08-07 12:43:37 UTC
(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..
Comment 8 Yohann Dufour 2014-08-08 14:26:43 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2014-08-08 14:38:24 UTC
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>
Comment 10 Marcel de Rooy 2014-08-08 14:40:07 UTC
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
Comment 11 Tomás Cohen Arazi 2014-08-11 14:52:38 UTC
Patch pushed to master.

Thanks Yohann!
Comment 12 Katrin Fischer 2014-08-16 21:03:05 UTC
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
Comment 13 Katrin Fischer 2014-08-16 21:05:28 UTC
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
Comment 14 Marcel de Rooy 2014-08-18 07:24:44 UTC
(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?
Comment 15 Tomás Cohen Arazi 2014-08-18 16:14:22 UTC
(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.
Comment 16 Yohann Dufour 2014-08-19 08:54:34 UTC
I've created bug 12784 for the bug about patron details update request from the OPAC.