Bug 12633 - SQLHelper replacement - C4::Members
Summary: SQLHelper replacement - C4::Members
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 12457 12648 13021 13891 13892 13894 13926 14038
Blocks: 8753 11385 15332
  Show dependency treegraph
 
Reported: 2014-07-23 13:19 UTC by Yohann Dufour
Modified: 2017-06-14 22:11 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 12633: SQLHelper replacement - C4::Members (11.01 KB, patch)
2014-07-25 14:22 UTC, Yohann Dufour
Details | Diff | Splinter Review
Bug 12633: SQLHelper replacement - C4::Members (10.86 KB, patch)
2014-07-25 14:26 UTC, Yohann Dufour
Details | Diff | Splinter Review
[Signed-off] Bug 12633: SQLHelper replacement - C4::Members (11.06 KB, patch)
2014-08-05 19:44 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12633: SQLHelper replacement - C4::Members (11.58 KB, patch)
2014-08-18 14:00 UTC, Yohann Dufour
Details | Diff | Splinter Review
Bug 12633: Remove SQLHelper in C4::Members (13.86 KB, patch)
2015-04-13 14:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12633: Fix specific default values (1.06 KB, patch)
2015-04-21 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12633: Remove SQLHelper in C4::Members (13.92 KB, patch)
2015-04-21 15:35 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 12633: Fix specific default values (1.13 KB, patch)
2015-04-21 15:35 UTC, Mirko Tietgen
Details | Diff | Splinter Review
[PASSED QA] Bug 12633: Remove SQLHelper in C4::Members (13.98 KB, patch)
2015-04-29 21:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12633: Fix specific default values (1.19 KB, patch)
2015-04-29 21:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12633: Remove export line (555 bytes, patch)
2016-11-07 15:17 UTC, Jonathan Druart
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-23 13:19:52 UTC
The module C4::Members depends on module C4::SQLHelper. However, the module SQLHelper is obsolete, so the calls for InsertInTable, UpdateInTable and SearchInTable have to be replaced by the use of the module DBIx::Class.
Comment 1 Yohann Dufour 2014-07-25 14:22:00 UTC Comment hidden (obsolete)
Comment 2 Yohann Dufour 2014-07-25 14:26:49 UTC Comment hidden (obsolete)
Comment 3 Marc Véron 2014-08-05 19:44:08 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2014-08-17 09:51:46 UTC
When adding patrons to a routing list, this explodes:
DBIx::Class::Storage::DBI::_dbh_execute(): Unknown column 'subscriptionid' in 'where clause' at /home/katrin/kohaclone/C4/Members.pm line 358

To test:
- Go to serials
- Create a subscription
- Go to 'serial collection'
- Create a routing list
- Add recipients to routing list
- Window for patron search opens and shows error from above
Comment 5 Yohann Dufour 2014-08-18 14:00:53 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2014-08-27 14:35:44 UTC
Enabling ExtendedPatronAttributes completely breaks patron searching with this patch.
Comment 7 Kyle M Hall 2014-08-27 14:40:18 UTC
Actually, it only breaks it for patroncards/members-search.pl
Comment 8 Kyle M Hall 2014-08-27 14:51:18 UTC
This ties in a bit with bug 12833. This bug is why this patch isn't breaking the main patron search. They aren't directly related though.
Comment 9 Jonathan Druart 2014-10-01 14:51:51 UTC
If I am not wrong, there are 3 occurrences of C4::Members::Search:
1/ acqui/aqbasketuser_search.pl
2/ members/guarantor_search.pl
3/ patroncards/members-search.pl

1 is removed by 12648, I have a patch for 2.
3 is todo.

When done, I will completely remove the C4::Members::Search subroutine.
Some code should be move from C4::Utils::DT::Members to C4::Members (or Koha::Members).
Comment 10 Jonathan Druart 2014-10-02 08:28:34 UTC
(In reply to Jonathan Druart from comment #9)
> 1 is removed by 12648, I have a patch for 2.

The call in guarantor_search is removed by 13021.
Comment 11 Katrin Fischer 2014-10-22 05:34:21 UTC
Do we need a new patch here? It looks like this is the last missing piece to get rid of SQLHelper.
Comment 12 Jonathan Druart 2014-11-03 11:04:45 UTC
(In reply to Katrin Fischer from comment #11)
> Do we need a new patch here? It looks like this is the last missing piece to
> get rid of SQLHelper.

see comment 9.
Comment 13 Jonathan Druart 2015-03-20 10:31:12 UTC
No track of SQLHelper in C4::Members anymore :)
Comment 14 Katrin Fischer 2015-03-20 10:41:23 UTC
Woohoo!
Comment 15 Jonathan Druart 2015-03-20 10:44:49 UTC
Sorry, but there is one left in C4::Members::Search :-/
Comment 16 Jonathan Druart 2015-03-20 17:03:08 UTC
Ok so...
The good news is that only 1 occurrence still exists in C4::Members::Search
The bad one is that it is called from 3 different places  (but I may have missed some others...):
  admin/aqbudget_user_search.pl
  serials/member-search.pl
  patroncards/members-search.pl

I have a patch and will be able to submitted it on monday.
Comment 17 Jonathan Druart 2015-03-23 11:09:47 UTC
(In reply to Jonathan Druart from comment #16)
> Ok so...
> The good news is that only 1 occurrence still exists in C4::Members::Search
> The bad one is that it is called from 3 different places  (but I may have
> missed some others...):
>   admin/aqbudget_user_search.pl
>   serials/member-search.pl
>   patroncards/members-search.pl
> 
> I have a patch and will be able to submitted it on monday.

Done on bug 13891.
Comment 18 Jonathan Druart 2015-03-23 12:17:21 UTC
circ/ysearch.pl should not use C4::Members::Search, done on bug 13892.
Comment 19 Jonathan Druart 2015-04-13 14:26:35 UTC Comment hidden (obsolete)
Comment 20 Josef Moravec 2015-04-15 11:11:04 UTC
I wanted to test it, but there is merge conflict. Could you please rebase?
Comment 21 Jonathan Druart 2015-04-15 11:55:12 UTC
(In reply to Josef Moravec from comment #20)
> I wanted to test it, but there is merge conflict. Could you please rebase?

A patch has been pushed to master and I think it is a mistake. I pingued Tomas on bug 13189.

You can test it reverting a7822b0a32faa5ffe6e5a3b43c1cd2c58a8a802a before.
Comment 22 Jonathan Druart 2015-04-17 13:33:22 UTC
The patch has been reverted from master.
Comment 23 Mirko Tietgen 2015-04-21 14:00:33 UTC
With this patch applied, leaving the »Date of birth« field empty results in a default value of 02/00/0, which, on a subsequent change of another field, leads to the error message »The following fields are wrong. Please fix them. Date of birth is invalid.«

If Date of birth is empty, there should not be anything added on saving the record.
Comment 24 Jonathan Druart 2015-04-21 14:35:17 UTC
I don't get it,
I created a new patron, didn't fill the date of birth, save
I got null in DB and nothing in the input on editing.
Comment 25 Jonathan Druart 2015-04-21 14:35:49 UTC
Ok sorry, too quick, I get it when modifying a patron
Comment 26 Jonathan Druart 2015-04-21 14:51:24 UTC Comment hidden (obsolete)
Comment 27 Mirko Tietgen 2015-04-21 15:35:31 UTC Comment hidden (obsolete)
Comment 28 Mirko Tietgen 2015-04-21 15:35:37 UTC Comment hidden (obsolete)
Comment 29 Katrin Fischer 2015-04-21 21:00:38 UTC
Hi Jonathan,

I think I located another use of the Search routine, can you please check?

members/nl-search.pl:        my $local_results = Search( $identifier );
Comment 30 Marc Véron 2015-04-22 06:20:00 UTC
git grep ' Search('

result:
members/nl-search.pl:        my $local_results = Search( $identifier );
Comment 31 Jonathan Druart 2015-04-22 08:19:18 UTC
(In reply to Katrin Fischer from comment #29)
> Hi Jonathan,
> 
> I think I located another use of the Search routine, can you please check?
> 
> members/nl-search.pl:        my $local_results = Search( $identifier );

Good catch, bug 14038 opened.
Comment 32 Jonathan Druart 2015-04-29 13:01:01 UTC
Bug 14038 is signed off.
Comment 33 Katrin Fischer 2015-04-29 21:01:51 UTC
Created attachment 38697 [details] [review]
[PASSED QA] Bug 12633: Remove SQLHelper in C4::Members

This is the only places where SQLHelper is still called.
The C4::Members::Search is not used anymore, but ModMember and
AddMember.

This patch replaced the calls to SQLHelper to use DBIX::Class.
TODO: Move them to Koha::Borrower.

Test plan:
1/ Make sure the patron search still works (no changes expected since the
code was not in used).
2/ Add a patron with all fields filled
3/ Add another patron with some fields filled
4/ Update them with other values
5/ Delete them

You should not get any errors.

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 34 Katrin Fischer 2015-04-29 21:02:06 UTC
Created attachment 38698 [details] [review]
[PASSED QA] Bug 12633: Fix specific default values

The dates should be set to undef if defined but empty, otherwise with an
empty string, '0000-00-00' will be inserted into the DB.

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 35 Katrin Fischer 2015-04-29 21:13:09 UTC
Habits.. this was only a sign off, fixing.
Comment 36 Katrin Fischer 2015-04-29 21:14:36 UTC
It's getting too late... this is PASSED QA.
Comment 37 Tomás Cohen Arazi 2015-05-15 18:44:40 UTC
Patches pushed to master.

Thanks Jonathan, almost there!
Comment 38 Jonathan Druart 2016-11-07 15:17:21 UTC
Created attachment 57270 [details] [review]
Bug 12633: Remove export line