Bug 10020

Summary: Remove old code related to 'ethnicity'
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact:
Severity: enhancement    
Priority: P5 - low CC: chris, jonathan.druart, nengard, nick, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 14981    
Attachments: Bug 10020: Remove code related to ethnicity
Bug 10020: Remove DB table and fields related to ethnicity
Bug 10020: Remove code related to ethnicity
[SIGNED OFF] Bug 10020: Remove DB table and fields related to ethnicity
[SIGNED OFF] Bug 10020: Remove code related to ethnicity
[PASSED QA] Bug 10020: Remove DB table and fields related to ethnicity
[PASSED QA] Bug 10020: Remove code related to ethnicity
Bug 10020: Remove DB table and fields related to ethnicity
Bug 10020: Remove code related to ethnicity
Bug 10020: (RM followup) DBIx update
Bug 10020: (RM followup) DBIx update / delete leftover file

Description Katrin Fischer 2013-04-10 08:34:09 UTC
While updating our database documentation I found ethnicity and I suspect, that this is no longer used in current versions of Koha.

Related code I found so far:

- database table ethnicity
- database columns ethnicity and ethnotes in borrowers and deletedborrowers
- ethnicitycategories in C4/Members.pm
- some code in memberentry.pl

I can see no way in current versions for a library to enter data in the related fields and tables. 

Also we have a very flexible feature for patron attributes now, that I think, should make it possible to replicate this feature, if a library wanted to use it.
Comment 1 Katrin Fischer 2013-04-10 10:05:41 UTC
Note: We need to check the table and columns are empty before dropping them. If possible migrate data to patron attributes automatically.
Comment 2 Chris Cormack 2013-04-10 10:07:38 UTC
(In reply to comment #1)
> Note: We need to check the table and columns are empty before dropping them.
> If possible migrate data to patron attributes automatically.

If non empty we must migrate them, we can't throw away users data.
Comment 3 Galen Charlton 2014-02-21 16:19:09 UTC
(In reply to Chris Cormack from comment #2)
> (In reply to comment #1)
> > Note: We need to check the table and columns are empty before dropping them.
> > If possible migrate data to patron attributes automatically.
> 
> If non empty we must migrate them, we can't throw away users data.

My inclination is to treat this like bug 7372: i.e., create an ETHNICITY authval, migrate the ethnicity table to that, then drop it.

Moving the ethnicity column to a patron attribute can come later, if it all.
Comment 4 Katrin Fischer 2014-02-21 16:22:39 UTC
+1
Comment 5 Nicole C. Engard 2015-03-05 13:33:25 UTC
+1
Comment 6 Jonathan Druart 2015-04-10 11:26:56 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2015-04-10 11:27:03 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2015-04-10 11:30:01 UTC Comment hidden (obsolete)
Comment 9 Nick Clemens 2015-04-15 22:35:47 UTC
Test plan successful for first patch, when checking for regression I cannot delete a patron

Software error:

DBIx::Class::Storage::DBI::_dbh_execute(): Unknown column 'me.ethnicity' in 'field list' at /home/kidclamp/kohaclone/C4/Members.pm line 1864
For help, please send mail to the webmaster ([no address given]), giving this error message and the time and date of the error.
Comment 10 Jonathan Druart 2015-04-16 08:02:26 UTC
(In reply to Nick Clemens from comment #9)
> Test plan successful for first patch, when checking for regression I cannot
> delete a patron
> 
> Software error:
> 
> DBIx::Class::Storage::DBI::_dbh_execute(): Unknown column 'me.ethnicity' in
> 'field list' at /home/kidclamp/kohaclone/C4/Members.pm line 1864
> For help, please send mail to the webmaster ([no address given]), giving
> this error message and the time and date of the error.

I have retested and it works.
Did you update the schema (step 0 of first patch)?
You need to edit the file and set $db_name, $db_user and $db_password
Comment 11 Nick Clemens 2015-04-16 18:30:00 UTC Comment hidden (obsolete)
Comment 12 Nick Clemens 2015-04-16 18:30:10 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2015-04-23 11:22:41 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2015-04-23 11:22:49 UTC Comment hidden (obsolete)
Comment 15 Tomás Cohen Arazi 2015-06-04 17:22:08 UTC
I agree with Galen here: I think we need to create extended patron attributes for storing this. It can be linked to an authorized value category or not, but I don't think we should leave cruft on the database (even with a warning message: the users will read it, but won't be capable of doing anything).
Comment 16 Jonathan Druart 2015-06-16 10:24:38 UTC
(In reply to Tomás Cohen Arazi from comment #15)
> I agree with Galen here: I think we need to create extended patron
> attributes for storing this. It can be linked to an authorized value
> category or not, but I don't think we should leave cruft on the database
> (even with a warning message: the users will read it, but won't be capable
> of doing anything).

IMO it does not worth spending time on this.
The patch removes the tables/fields if data exist (which is certainly not the case), or leave them.

How do you see the data migration? Just create 2 attributes "ethnicity" and "ethnotes"? I don't understand why we should need an authorized value here.
Comment 17 Katrin Fischer 2015-07-09 00:52:03 UTC
I agree with Jonathan, I think creating authorized values is not necessary. If we are going with extended patron attributes, then I'd just copy the values.
Comment 18 Jonathan Druart 2015-07-09 07:31:49 UTC
(In reply to Jonathan Druart from comment #16)
> The patch removes the tables/fields if data exist (which is certainly not
> the case), or leave them.

Hum, trying again...
The patch removes the tables/fields when they don't contain any data (which is certainly the case) or keep them if they are not empty.
Comment 19 Jonathan Druart 2015-09-22 11:01:05 UTC
(In reply to Jonathan Druart from comment #18)
> (In reply to Jonathan Druart from comment #16)
> > The patch removes the tables/fields if data exist (which is certainly not
> > the case), or leave them.
> 
> Hum, trying again...
> The patch removes the tables/fields when they don't contain any data (which
> is certainly the case) or keep them if they are not empty.

And I think it's enough, don't you think?
Back to the RM queue for reconsidering it.
Comment 20 Tomás Cohen Arazi 2015-10-02 18:05:48 UTC
Please, small rebase needed. Thanks!
Comment 21 Jonathan Druart 2015-10-05 07:39:52 UTC
Created attachment 43103 [details] [review]
Bug 10020: Remove DB table and fields related to ethnicity

This patch remove the ethnicity table and the ethnicity and ethnotes
fields from the 3 tables: borrowers, deletedborrowers and
borrower_modifications IF no data exist.

Test plan:
0/ Apply this patch and update the Koha::Schema
(misc/devel/update_dbix_class_files.pl).
1/ Make sure the ethnicity has at least 1 entry
2/ Launch the updatedb entry and confirm you get a warning message and
the DB structure is not updated.
3/ truncate the ethnicity table and fill ethnicity (or ethnotes) for at least 1
row of the borrowers table.
4/ Launch the updatedb entry and confirm you get a warning message and
the DB structure is not updated.
5/ Make sure the ethnicity table is empty as well as the ethniciy and
the ethnotes fields of borrowers, deletedborrowers and
borrower_modifications
6/ Launch the updatedb entry and confirm you don't get the warning
message and the DB structure is updated (ie. table and fields dropped).

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Jonathan Druart 2015-10-05 07:39:57 UTC
Created attachment 43104 [details] [review]
Bug 10020: Remove code related to ethnicity

This is a(nother) vestige of Koha (2.2?).
This patch removes unused code related to the 'ethnicity'.

In detail:
There is no way to fill the ethnicity table.
There is no way to fill the borrowers.ethnicity and borrowers.ethnotes.

BUT if borrowers.ethnicity exists, the value is displayed on
members/moremember.pl (and only here).

Test plan:
Apply this patch and confirm there is no regression on
adding/updating/deleting patrons.
Note that you don't see the ethnicity value on the moremember.pl page even if a patron has it.

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Tomás Cohen Arazi 2015-10-06 13:56:34 UTC
Created attachment 43174 [details] [review]
Bug 10020: (RM followup) DBIx update

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi 2015-10-06 14:08:20 UTC
Patch pushed to master.

Thanks Jonathan!
Comment 25 Tomás Cohen Arazi 2015-10-13 15:14:46 UTC
Created attachment 43384 [details] [review]
Bug 10020: (RM followup) DBIx update / delete leftover file

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>