Bug 7154 - Group borrower attributes and link between patron category and attribute
Summary: Group borrower attributes and link between patron category and attribute
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: 3.8
Hardware: All All
: P3 enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Ian Walls
URL:
Keywords:
Depends on:
Blocks: 7836 21080
  Show dependency treegraph
 
Reported: 2011-11-04 13:33 UTC by Jonathan Druart
Modified: 2018-07-16 22:45 UTC (History)
7 users (show)

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


Attachments
Proposed patch (22.32 KB, patch)
2011-11-04 13:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Proposed patch (23.30 KB, patch)
2011-12-02 15:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Proposed patch (23.38 KB, patch)
2011-12-05 08:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7154: Modification in borrower attributes (23.42 KB, patch)
2011-12-21 20:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
Displaying patron attributes (18.56 KB, image/png)
2012-01-06 10:59 UTC, Magnus Enger
Details
Editing patron attributes (29.01 KB, image/png)
2012-01-06 10:59 UTC, Magnus Enger
Details
7154: Followup: Adds tables on modify + class filled wih AV (16.74 KB, patch)
2012-01-06 15:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7154: Followup: Change link to patron category (14.70 KB, patch)
2012-01-06 17:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Rebased and Squashed version (33.78 KB, patch)
2012-02-10 09:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7154: Modification in borrower attributes (33.13 KB, patch)
2012-03-20 09:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7154 Modification in borrower attributes (33.13 KB, patch)
2012-03-20 13:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7154 Modification in borrower attributes (33.13 KB, patch)
2012-03-20 13:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7154: Modification in borrower attributes (33.13 KB, patch)
2012-03-20 13:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7154 Modification in borrower attributes (33.33 KB, patch)
2012-03-21 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7154 Modification in borrower attributes (33.31 KB, patch)
2012-03-21 10:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7154 Modification in borrower attributes (33.41 KB, patch)
2012-03-21 13:21 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 Jonathan Druart 2011-11-04 13:33:53 UTC
In borrower attributes, we would like to :

* group some attributes for a more friendly display
* allow a link between a borrower category and an attribute to display it only for the specified category
Comment 1 Jonathan Druart 2011-11-04 13:50:07 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2011-11-04 14:02:56 UTC
ref BibLibre: MT5699
Comment 3 Owen Leonard 2011-11-04 15:24:03 UTC
I've done some tests on this and ran into a couple of problems. First, the updatedatabase process got stuck in a loop. The update completed, but I kept getting  I'm not sure why, but changing:

    $dbh->do(qq{ALTER TABLE borrower_attribute_types ADD COLUMN category_type VARCHAR(1)  NOT NULL DEFAULT '' AFTER `display_checkout`});
    $dbh->do(qq{ALTER TABLE borrower_attribute_types ADD COLUMN class VARCHAR(255)  NOT NULL DEFAULT '' AFTER `category_type`});

To:

    $dbh->do("ALTER TABLE borrower_attribute_types ADD COLUMN category_type VARCHAR(1)  NOT NULL DEFAULT '' AFTER `display_checkout`;");
    $dbh->do("ALTER TABLE borrower_attribute_types ADD COLUMN class VARCHAR(255)  NOT NULL DEFAULT '' AFTER `category_type`;");

...made the process complete properly.

The other problem I found was that setting a category type limit seems to block that attribute from appearing for *any* patrons. I set an "Adult" limit on one of my attributes and found that the attribute didn't appear on Adult or Child entry forms.
Comment 4 Nicole C. Engard 2011-11-05 20:39:15 UTC
I'd add to the test plan that we need to make sure that searchable attributes are still searchable - seems like every time we change something related to attributes we lose the ability to search them.

Nicole
Comment 5 Jonathan Druart 2011-12-02 15:23:34 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2011-12-03 21:08:31 UTC
It seems the new field 'class' is missing from kohastructure.sql:

+++ b/installer/data/mysql/kohastructure.sql
@@ -286,6 +286,7 @@ CREATE TABLE `borrower_attribute_types` ( -- definitions for custom patron field
   `staff_searchable` tinyint(1) NOT NULL default 0, -- defines if this field is searchable via the patron search in the staff client (1 for yes, 0 for no)
   `authorised_value_category` varchar(10) default NULL, -- foreign key from authorised_values that links this custom field to an authorized value category
   `display_checkout` tinyint(1) NOT NULL default 0,-- defines if this field displays in checkout screens
+  `category_type` VARCHAR(1) NOT NULL DEFAULT '',-- defines a category for an attribute_type
   PRIMARY KEY  (`code`),
   KEY `auth_val_cat_idx` (`authorised_value_category`)
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8;
Comment 7 Jonathan Druart 2011-12-05 08:50:13 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2011-12-21 20:22:22 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2011-12-21 20:22:46 UTC
Fixed conflict in updatedatabase.pl
Comment 10 Magnus Enger 2012-01-06 10:55:22 UTC
Nice enhancements! Just a couple of questions/suggestions: 

1. Could it be a good idea to use authorized values for class names? That would leave less room for typos and make it possible to change the name of a class without changing every attribute that uses that class. 

2. When a patron is viewed with e.g. members/moremember.pl?borrowernumber=3 the attributes that have a class are displayed in a separate table, which is nice. But in the edit screens they are all in the same table, sorted by Type. Could it be an idea to sort the table by Class instead, to make the attributes with the same class appear more close together? 

I'll attach screenshots of what the display looks like.
Comment 11 Magnus Enger 2012-01-06 10:59:18 UTC
Created attachment 7066 [details]
Displaying patron attributes
Comment 12 Magnus Enger 2012-01-06 10:59:46 UTC
Created attachment 7067 [details]
Editing patron attributes
Comment 13 Jonathan Druart 2012-01-06 15:07:32 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2012-01-06 15:21:45 UTC
I have a question about this :)

Why was patron category type (adult, staff, etc.) chosen instead of patron category? I think it would allow for a lot more flexibility. 

For example we use only 'adult' and 'staff' for academic libraries at the moment.
Comment 15 Jonathan Druart 2012-01-06 17:11:18 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2012-01-06 22:19:27 UTC
Afaik category type is an attribute for patron categories - I don't think you can use Koha without creating patron categories. You can define more than 1 patron category for each category type and don't use some of them at all.

I think both things are named too similar, this is quite complicated to get right :)
Comment 17 Owen Leonard 2012-02-07 20:45:54 UTC
Could we get a rebased and squashed version of this?
Comment 18 Jonathan Druart 2012-02-10 09:01:54 UTC Comment hidden (obsolete)
Comment 19 MathildeF 2012-03-20 09:16:49 UTC
i cant test with sandbox

CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
Auto-merging members/memberentry.pl
Auto-merging members/moremember.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 7154: Modification in borrower attributes
Comment 20 Jonathan Druart 2012-03-20 09:51:38 UTC Comment hidden (obsolete)
Comment 21 Julien Sicot 2012-03-20 10:56:45 UTC
Patch is not working. 
After i tried to create a new patron attribute type, i've got message "Added patron attribute type..." but this one doesn't appear in the list or in add patron form :(
Comment 22 Jonathan Druart 2012-03-20 11:17:59 UTC
The sandbox seems to not launch the updatedatabase. Please check me when you retest.
Comment 23 Jonathan Druart 2012-03-20 13:08:12 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2012-03-20 13:09:39 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2012-03-20 13:16:08 UTC Comment hidden (obsolete)
Comment 26 Julien Sicot 2012-03-20 15:06:46 UTC
Hi again,

I tested again and everything seems ok. 
I created several patron attributes with or without link between a borrower category. I also used the AV "PA_CLASS" to group attributes.

The options "make attribute staff_searchable in the staff patron search", "show attribute in patron check-out" and "display attribute on a patron's details page in the OPAC" work well.

Just one suggestion:

- Could it be possible to display the "Description" or "Description OPAC" rather than the code "Authorized value" to group attributes in patron's details page (cgi-bin/koha/members/moremember.pl)and Patron Attribute Types page (cgi-bin/koha/admin/patron-attr-types.pl)

Thx
Julien
Comment 27 Jonathan Druart 2012-03-21 10:04:46 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2012-03-21 10:48:41 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2012-03-21 13:21:51 UTC
Created attachment 8407 [details] [review]
Bug 7154 Modification in borrower attributes

This patch is rebased and add the description if exists rather than code in memberentry.pl
Comment 30 Julien Sicot 2012-03-21 14:06:26 UTC
I applied the patch again and it works correct.
Tested on sandbox.
Comment 31 Paul Poulain 2012-03-22 15:14:44 UTC
QA comments:
* perlcritic unchanged (still 1 error, that was here before the patch)
* feature nicely documented on the screen (for the PA_CLASS authorised values)
* feature works well & is great !

Passed QA

possible enhancement = put patron infos in tabs, as the patron display is more and more full of informations ! but that will be for another bug !