Bug 6190 - Custom field removal from patron entry screens
Summary: Custom field removal from patron entry screens
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Srdjan Jankovic
QA Contact: Marcel de Rooy
URL:
Keywords:
: 3656 (view as bug list)
Depends on: 7167
Blocks: 7559
  Show dependency treegraph
 
Reported: 2011-04-14 04:28 UTC by Srdjan Jankovic
Modified: 2013-12-05 20:01 UTC (History)
6 users (show)

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


Attachments
patch (26.33 KB, patch)
2011-04-14 06:03 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Patch fixed to apply to master (26.45 KB, patch)
2011-04-26 06:08 UTC, Katrin Fischer
Details | Diff | Splinter Review
patch (26.67 KB, patch)
2011-05-08 23:54 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (43.12 KB, patch)
2011-05-10 00:29 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (15.78 KB, patch)
2011-05-12 03:04 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (58.62 KB, patch)
2011-06-02 02:33 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Empty groups still showing title and box (41.17 KB, image/png)
2011-06-15 13:33 UTC, Magnus Enger
Details
Bug 6190: Followup fixing empty groups (5.70 KB, patch)
2011-08-01 19:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
bug_6190: BorrowerUnwantedField syspref and removing fields from the patron entry (53.64 KB, patch)
2011-08-01 19:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
patch (58.11 KB, patch)
2011-08-12 03:37 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (59.88 KB, patch)
2011-09-05 06:44 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (24.31 KB, patch)
2011-10-10 00:16 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6190: BorrowerUnwantedField syspref and removing fields from the patron entry (25.21 KB, patch)
2011-10-11 06:49 UTC, Katrin Fischer
Details | Diff | Splinter Review
Signed patch (25.74 KB, patch)
2011-11-14 12:44 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 Srdjan Jankovic 2011-04-14 04:28:42 UTC
Implement a mechanism for removing unwanted fields from the entry screens.
It will be a syspref, BorrowerUnwantedField, that will have the same format as BorrowerMandatoryField
Comment 1 Srdjan Jankovic 2011-04-14 06:03:34 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2011-04-14 06:05:52 UTC
*** Bug 3656 has been marked as a duplicate of this bug. ***
Comment 3 Katrin Fischer 2011-04-26 06:08:08 UTC Comment hidden (obsolete)
Comment 4 Srdjan Jankovic 2011-05-08 23:54:59 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2011-05-09 06:24:34 UTC
Hi Srdjan, thx for your work on this :) 

I agree hiding the flags might cause problems if libraries for example choose to debar patrons automatically. 

I still have 2 problems with your patch:

1) The translated sql files are missing the system preference. Until we make all languages use the en file this is still needed so people installing in another language are not missing system preferences. 

2) The feature to hide whole groups seems not complete and a little inconsistent to me:

The condition to hide alternate contact is:
+    [% UNLESS noaltcontactsurname && noaltcontactfirstname %]
So if you choose to hide the names to make it just another address field, the whole block will disappear.

And is seems there is no way to hide the main address group. I actually have libraries saving only firstname, surname and email address of a patron, because they rely on another database for address information.
Comment 6 Srdjan Jankovic 2011-05-10 00:29:50 UTC Comment hidden (obsolete)
Comment 7 Magnus Enger 2011-05-11 11:13:38 UTC
(In reply to comment #6)
> Created attachment 4071 [details] [review]
> patch
> 
> Added translations, better hiding

As far as I can see, this patch still misses installer/data/mysql/nb-NO/1-Obligatorisk/sysprefs.sql. 

It will be a great new feature, though!
Comment 8 Srdjan Jankovic 2011-05-12 03:04:16 UTC Comment hidden (obsolete)
Comment 9 Owen Leonard 2011-06-01 13:33:19 UTC
The first patch (attachment 4071 [details] [review]) did not apply for me:

Bug 6190 - Custom field removal from patron entry screens

patch
Apply? [yn] y

Applying: bug_6190: BorrowerUnwantedField syspref and removing fields from the patron entry
/home/oleonard/kohaclone/.git/rebase-apply/patch:733: trailing whitespace.
	$template->param( "no$_" => 1);    
error: patch failed: koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt:887
error: koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt: patch does not apply
Patch failed at 0001 bug_6190: BorrowerUnwantedField syspref and removing fields from the patron entry
Comment 10 Srdjan Jankovic 2011-06-02 02:33:32 UTC Comment hidden (obsolete)
Comment 11 Magnus Enger 2011-06-15 13:31:47 UTC
Marking this as failed QA for two reasons: 

1. Hiding all fields in a group does not hide the whole group - see attached screenshot. 

2. There is a problem with escaping in updatedatabase.pl - it looks like this: 

$dbh->do("INSERT INTO `systempreferences` (variable,value,explanation,options,type) VALUES('BorrowerUnwantedField','','Name the fields you don\'t need to store for a patron\'s account',NULL,'free')");

should be this: 

$dbh->do("INSERT INTO `systempreferences` (variable,value,explanation,options,type) VALUES('BorrowerUnwantedField','','Name the fields you don''t need to store for a patron''s account',NULL,'free')");

('' instead of \')

Can't wait to have this feature in place, though!
Comment 12 Magnus Enger 2011-06-15 13:33:30 UTC
Created attachment 4481 [details]
Empty groups still showing title and box

This is what the patron entry screen looks like when BorrowerUnwantedField is set to cardnumber|surname|firstname|title|othernames|initials|streetnumber|streettype|address|address2|city|state|zipcode|country|email|phone|mobile|fax|emailpro|phonepro|B_streetnumber|B_streettype|B_address|B_address2|B_city|B_state|B_zipcode|B_country|B_email|B_phone|dateofbirth|branchcode|categorycode|dateenrolled|dateexpiry|gonenoaddress|lost|debarred|contactname|contactfirstname|contacttitle|borrowernotes|relationship|sex|password|userid|opacnote|contactnote|sort1|sort2|altcontactfirstname|altcontactsurname|altcontactaddress1|altcontactaddress2|altcontactaddress3|altcontactstate|altcontactzipcode|altcontactcountry|altcontactphone
Comment 13 Katrin Fischer 2011-08-01 19:28:54 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2011-08-01 19:30:57 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2011-08-01 19:32:51 UTC
For testing: 

apply first:
bug_6190: BorrowerUnwantedField syspref and removing fields from the patron entry
This is the rebased patch from Srdjan.

apply second:
Bug 6190: Followup fixing empty groups 
This is a follow up fixing the groups and quoting in the updatedatabase.
Comment 16 Nicole C. Engard 2011-08-01 22:55:03 UTC
Srdjan,

Can you please put notes in your patches about what they do?  See Katrin's follow up for an example.  This will help get your patches tested properly and signed off on quicker.

Thanks a bunch!
Nicole
Comment 17 Katrin Fischer 2011-08-05 16:31:17 UTC
Hi Nicole, 
you can apply both patches together and test as I described on the bug. The feature will allow to hide fields on the patron entry screen without using jquery, but using a syspref instead = BorrowerUnwantedField.
Comment 18 Nicole C. Engard 2011-08-08 19:28:04 UTC
first patch does not apply:

Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: bug_6190: BorrowerUnwantedField syspref and removing fields from the patron entry
Using index info to reconstruct a base tree...
<stdin>:789: trailing whitespace.
        $template->param( "no$_" => 1);    
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/de-DE/mandatory/sysprefs.sql
Auto-merging installer/data/mysql/en/mandatory/sysprefs.sql
Auto-merging installer/data/mysql/fr-FR/1-Obligatoire/unimarc_standard_systemprefs.sql
Auto-merging installer/data/mysql/it-IT/necessari/sysprefs.sql
Auto-merging installer/data/mysql/nb-NO/1-Obligatorisk/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/nb-NO/1-Obligatorisk/sysprefs.sql
Auto-merging installer/data/mysql/pl-PL/mandatory/sysprefs.sql
Auto-merging installer/data/mysql/ru-RU/mandatory/system_preferences_full_optimal_for_install_only.sql
Auto-merging installer/data/mysql/uk-UA/mandatory/system_preferences_full_optimal_for_install_only.sql
Auto-merging installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
Auto-merging members/memberentry.pl
Auto-merging xt/syspref.t
CONFLICT (content): Merge conflict in xt/syspref.t
Failed to merge in the changes.
Patch failed at 0001 bug_6190: BorrowerUnwantedField syspref and removing fields from the patron entry
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
nengard@debian:~/kohaclone$
Comment 19 Nicole C. Engard 2011-08-08 19:28:56 UTC
PS. I assumed that the first patch was the one that didn't say 'followup'
Comment 20 Katrin Fischer 2011-08-08 20:30:00 UTC
I remembered a little to late that I should attach the rebased first patch. 
Seems another syspref has been pushed and it needs to rebased again. 

There is also one small thing that I think is a mistake - the change to xt/syspref.t.
Comment 21 Srdjan Jankovic 2011-08-12 03:37:29 UTC Comment hidden (obsolete)
Comment 22 Katrin Fischer 2011-08-12 05:13:30 UTC
Looks like Srdjan sqashed in my follow-up, but not entirely sure as it's not easy to compare all changes.
Comment 23 Srdjan Jankovic 2011-08-12 05:29:50 UTC
Yes, I thought it was easier to keep just one patch
Comment 24 Nicole C. Engard 2011-08-12 19:17:39 UTC
So what does this mean - I test the one patch? or does Katrin have to resubmit her follow up?
Comment 25 Srdjan Jankovic 2011-08-13 00:40:40 UTC
Just one patch, this one contains all relevant changes.
Comment 26 Nicole C. Engard 2011-08-25 00:25:26 UTC
Applying: bug_6190: BorrowerUnwantedField syspref and removing fields from the patron entry
/home/nengard/kohaclone/.git/rebase-apply/patch:348: trailing whitespace.
[% END # hide fieldset %]    
/home/nengard/kohaclone/.git/rebase-apply/patch:522: trailing whitespace.
[%END # hide fieldset %]  
/home/nengard/kohaclone/.git/rebase-apply/patch:841: trailing whitespace.
        $template->param( "no$_" => 1);    
error: patch failed: installer/data/mysql/updatedatabase.pl:4399
error: installer/data/mysql/updatedatabase.pl: patch does not apply
Patch failed at 0001 bug_6190: BorrowerUnwantedField syspref and removing fields from the patron entry
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Patch left in /tmp/patch-u_pyV4.patch
nengard@debian:~/kohaclone$
Comment 27 Nicole C. Engard 2011-08-25 01:11:56 UTC
Tested applying with using git bz and it still doesn't apply:


--------------------------
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: bug_6190: BorrowerUnwantedField syspref and removing fields from the patron entry
Using index info to reconstruct a base tree...
<stdin>:348: trailing whitespace.
[% END # hide fieldset %]    
<stdin>:522: trailing whitespace.
[%END # hide fieldset %]  
<stdin>:841: trailing whitespace.
        $template->param( "no$_" => 1);    
warning: 3 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
Auto-merging members/memberentry.pl
Failed to merge in the changes.
Patch failed at 0001 bug_6190: BorrowerUnwantedField syspref and removing fields from the patron entry
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
nengard@debian:~/kohaclone$
Comment 28 Srdjan Jankovic 2011-09-05 06:44:41 UTC Comment hidden (obsolete)
Comment 29 Owen Leonard 2011-10-07 13:41:52 UTC
Sorry, this patch needs to be rebased and resubmitted again :(
Comment 30 Srdjan Jankovic 2011-10-10 00:16:20 UTC Comment hidden (obsolete)
Comment 31 Katrin Fischer 2011-10-11 06:49:40 UTC Comment hidden (obsolete)
Comment 32 Paul Poulain 2011-10-24 11:40:12 UTC
Updating version : This ENH could be in Koha 3.8
Comment 33 Paul Poulain 2011-10-25 15:05:44 UTC
Bug versionned for master. entries will be made against rel_3_8 once the patch has been applied (see thread about that on koha-devel yesterday)
Comment 34 Marcel de Rooy 2011-11-14 12:44:27 UTC
Created attachment 6303 [details] [review]
Signed patch

Rebased signoff
Comment 35 Marcel de Rooy 2011-11-14 12:45:42 UTC
Some QA remarks (not blocking approval):
systempreferences.pl
This script is now only used for Local use. Adding a line for Patrons is no longer useful.

patrons.pref
Just a practical question: How does a user know the name of a field to exclude, e.g. B_address2 ? Could you show the fields in a popup when clicking on a link or so? I realize that the same holds for BorrowerMandatoryField. Maybe a small follow up?

memberentry.pl
regex, use of o modifier: Perl will not recompile the pattern unless an interpolated variable that it contains changes. You can force Perl to skip the test and never recompile by adding a /o (which stands for "once") after the trailing delimiter. The regex here does not contain a variable. Actually, I wonder if you need this regex at all here.

Thank you for work and numerous rebasing!
Comment 36 Katrin Fischer 2011-11-14 13:06:20 UTC
Hi Marcel, thx for QA :) We have the same problem for borrower import - the user does not know which fields correspond to which description in the user interface. Translation makes it even harder. I think the best we can do now is link to the database documentation - http://schema.koha-community.org/tables/borrowers.html
Comment 37 Paul Poulain 2012-01-06 16:32:56 UTC
Patch pushed, please test

Marcel & Katrin: i've added a link to the syspref description:
The following <a href:...>database columns</a> will not appear on the patron entry screen
it's a great idea !