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
Created attachment 6202 [details] [review] Proposed patch For testing : - Create borrower attributes, link with borrower category (or not) - Spécify a class (group of attributes) or not See admin/patron-attr-types.pl, members/memberentrygen.pl and members/moremember.pl
ref BibLibre: MT5699
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.
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
Created attachment 6536 [details] [review] Proposed patch @Owen: thanks for your return. This new patch should have to correct previous problems. However, I don't understand why you say qq{} syntax is not correct. Normally, it's the same as quotes. @Nicole: I don't modify search for patrons. I have test and it's ok for me. Needs signoff
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;
Created attachment 6575 [details] [review] Proposed patch
Created attachment 6909 [details] [review] Bug 7154: Modification in borrower attributes * 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 this patch add 2 fields in borrower_attribute_types : category_type class
Fixed conflict in updatedatabase.pl
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.
Created attachment 7066 [details] Displaying patron attributes
Created attachment 7067 [details] Editing patron attributes
Created attachment 7070 [details] [review] 7154: Followup: Adds tables on modify + class filled wih AV Hi Magnus, Thank you for returns. This patch contains modifications you requested. - FIX display/hide attr on memberentry.pl when category changes - Attr classes filled with AV 'PA_CLASS' category (you have to create AV with a new category 'PA_CLASS' to fill class list) - Use tables (classes separation) on member modification and attr management (admin). Theses array are sorted by name.
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.
Created attachment 7074 [details] [review] Bug 7154: Followup: Change link to patron category Hi Katrin, You are right, it's more flexible. I rapidly developed this patch. But it's not ok. It seems some librairies could use the category type without the patron category, is that right ? To test this patch, you need to delete the contents of the table borrower_attribute_types and add the modifications (cf updatedatabase, a foreign key is insered)
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 :)
Could we get a rebased and squashed version of this?
Created attachment 7559 [details] [review] Rebased and Squashed version Bug 7154: Modification in borrower attributes * 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 * Attr classes filled with AV 'PA_CLASS' category (you have to create AV with a new category 'PA_CLASS' to fill class list) this patch add 2 fields in borrower_attribute_types : category_type class
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
Created attachment 8337 [details] [review] Bug 7154: Modification in borrower attributes Rebased patch
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 :(
The sandbox seems to not launch the updatedatabase. Please check me when you retest.
Created attachment 8350 [details] [review] Bug 7154 Modification in borrower attributes FIX error in updatedatabase
Created attachment 8351 [details] [review] Bug 7154 Modification in borrower attributes FIX error in updatedatabase
Created attachment 8353 [details] [review] Bug 7154: Modification in borrower attributes rename missing category_type to category_code
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
Created attachment 8389 [details] [review] Bug 7154 Modification in borrower attributes Display description if exists rather than code (in cgi-bin/koha/members/moremember.pl and cgi-bin/koha/admin/patron-attr-types.pl). I don't change the status (signed off), easy to test.
Created attachment 8397 [details] [review] Bug 7154 Modification in borrower attributes Rebased patch
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
I applied the patch again and it works correct. Tested on sandbox.
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 !