Bug 17364 - branchcode in BorrowerUnwantedField causes software error when saving patron record
Summary: branchcode in BorrowerUnwantedField causes software error when saving patron ...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Nick Clemens (kidclamp)
QA Contact: Testopia
URL:
Keywords: Manual
: 16865 (view as bug list)
Depends on: 22844
Blocks: 27261
  Show dependency tree / graph
 
Reported: 2016-09-27 20:42 UTC by Barton Chittenden
Modified: 2023-07-10 20:50 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
A more user friendly interface for selecting database columns for some system preferences (such as BorrowerUnwantedField) was added in Koha 20.11 (Bug 22844). Some database columns should be excluded from selection as they can cause server errors. For example, branchcode in BorrowerUnwantedField is required for adding patrons - if selected it causes a server error and you can't add a patron, so it should not be selectable. This bug fixes the issue by: - allowing developers to define the database columns to exclude from selection in the .pref system preference definition file using "exclusions: " - disabling the selection of the excluded database columns in the staff interface when configuring system preferences that allow selecting database columns - updating the BorrowerUnwantedField system preference to exclude branchcode
Version(s) released in:
21.05.00,20.11.03
Circulation function:

Attachments
Bug 17364: Add ability to exclude some columns from selection for system preferences (5.72 KB, patch)
2020-11-06 11:22 UTC, Nick Clemens (kidclamp) 		Details | Diff | Splinter Review
Bug 17364: Add ability to exclude some columns from selection for system preferences (5.14 KB, patch)
2020-11-06 11:31 UTC, Nick Clemens (kidclamp) 		Details | Diff | Splinter Review
Bug 17364: (follow-up) Add missing filter (1.89 KB, patch)
2020-12-15 13:55 UTC, Nick Clemens (kidclamp) 		Details | Diff | Splinter Review
Bug 17364: (follow-up) Do not check excluded fields (1.19 KB, patch)
2020-12-17 08:07 UTC, Fridolin Somers 		Details | Diff | Splinter Review
Bug 17364: Add ability to exclude some columns from selection for system preferences (5.17 KB, patch)
2020-12-17 08:10 UTC, Fridolin Somers 		Details | Diff | Splinter Review
Bug 17364: (follow-up) Add missing filter (1.95 KB, patch)
2020-12-17 08:10 UTC, Fridolin Somers 		Details | Diff | Splinter Review
Bug 17364: (follow-up) Do not check excluded fields (1.19 KB, patch)
2020-12-17 08:11 UTC, Fridolin Somers 		Details | Diff | Splinter Review
Bug 17364: (follow-up) Do not check excluded fields (1.25 KB, patch)
2020-12-17 11:26 UTC, Nick Clemens (kidclamp) 		Details | Diff | Splinter Review
Bug 17364: Add ability to exclude some columns from selection for system preferences (5.26 KB, patch)
2021-01-31 14:27 UTC, Katrin Fischer 		Details | Diff | Splinter Review
Bug 17364: (follow-up) Add missing filter (2.01 KB, patch)
2021-01-31 14:27 UTC, Katrin Fischer 		Details | Diff | Splinter Review
Bug 17364: (follow-up) Do not check excluded fields (1.30 KB, patch)
2021-01-31 14:27 UTC, Katrin Fischer 		Details | Diff | Splinter Review
Show Obsolete (8) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Barton Chittenden 2016-09-27 20:42:45 UTC 
If branchcode is among the fields in BorrowerUnwantedField, saving the borrower record in members/memberentry.pl will generate an Internal Server Error because there is a foreign key constraint on borrowers.branchcode.

To replicate the issue:

1/ Set the syspref BorrowerUnwantedField so that it contains branchcode
2/ Create a patron on members/memberentry.pl
3/ Save the patron, you will get "Internal Server Error".

It follows that PatronSelfRegistrationBorrowerUnwantedField should not contain branchcode either -- it's necessary to create a record in the borrowers table, so we shouldn't be able to exclude it.

Details for those who like that sort of thing:

If you're watching the plack error logs,you'll see something like this:

DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_salinas`.`borrowers`, CONSTRAINT `borrowers_ibfk_2` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`)) [for Statement "INSERT INTO `borrowers` ( `address`, `address2`, `borrowernotes`, `cardnumber`, `categorycode`, `city`, `contactnote`, `country`, `dateenrolled`, `dateexpiry`, `dateofbirth`, `debarred`, `email`, `emailpro`, `fax`, `firstname`, `initials`, `mobile`, `opacnote`, `othernames`, `password`, `phone`, `phonepro`, `privacy`, `sex`, `sort1`, `sort2`, `state`, `surname`, `title`, `userid`, `zipcode`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? )" with ParamValues: 0="1234 MAIN ST", 1="", 2="", 3="TESTYTESTERSON", 4="STAFF", 5="Bradley, CA", 6="", 7="", 8='2016-09-27', 9='2024-12-27', 10=undef, 11=undef, 12="", 13="", 14="", 15="TESTY", 16="", 17="", 18="", 19="", 20='!', 21="555 555 1212", 22="", 23=1, 24="", 25="C", 26="", 27="", 28="TESTERSON", 29="", 30='testy.testerson', 31="93426"] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832.
DBIx::Class::Storage::DBI::_dbh_execute(): Cannot add or update a child row: a foreign key constraint fails (`koha_salinas`.`borrowers`, CONSTRAINT `borrowers_ibfk_2` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`)) at /usr/share/koha/lib/C4/Members.pm line 769
Comment 1 Nick Clemens (kidclamp) 2020-11-06 11:22:20 UTC 
Created attachment 113238 [details] [review]
Bug 17364: Add ability to exclude some columns from selection for system preferences

Bug 22844 Added the option to provide a modal for selecting database columns for system preferences

Some system preferences are limited to a subset of the columns allowed. For instance, setting branchcode
in BorrowerUnwantedFields prevents adding new patrons

This patch allows for defining exclusions in the .pref file

To test:
1 - Alter BorroweUnwantedFields and select branchcode
2 - Attempt to add a new patron
3 - Get a message like "Something went wrong. Check the logs"
4 - Apply patches
5 - Alter the preference again
6 - Note that branchcode is unchecked and disabled
7 - Save the pref
8 - Add a patron
9 - It succeeds
Comment 2 Nick Clemens (kidclamp) 2020-11-06 11:31:49 UTC 
Created attachment 113239 [details] [review]
Bug 17364: Add ability to exclude some columns from selection for system preferences

Bug 22844 Added the option to provide a modal for selecting database columns for system preferences

Some system preferences are limited to a subset of the columns allowed. For instance, setting branchcode
in BorrowerUnwantedFields prevents adding new patrons

This patch allows for defining exclusions in the .pref file

To test:
1 - Alter BorroweUnwantedFields and select branchcode
2 - Attempt to add a new patron
3 - Get a message like "Something went wrong. Check the logs"
4 - Apply patches
5 - Alter the preference again
6 - Note that branchcode is unchecked and disabled
7 - Save the pref
8 - Add a patron
9 - It succeeds
Comment 3 Fridolin Somers 2020-12-11 15:26:32 UTC 
Patch works well :D

But :
There is a missing html filter in  : data-exclusions="[% CHUNK.exclusions %]

Most important : 
Clicking on "Select all" selects the branchcode :O
Comment 4 Nick Clemens (kidclamp) 2020-12-15 13:55:49 UTC 
Created attachment 114409 [details] [review]
Bug 17364: (follow-up) Add missing filter
Comment 5 Fridolin Somers 2020-12-15 15:12:42 UTC 
(In reply to Fridolin Somers from comment #3)
> 
> Most important : 
> Clicking on "Select all" selects the branchcode :O

Do you reproduce this ?
Its a major issue no ?
Comment 6 Lucas Gass (lukeg) 2020-12-16 22:58:38 UTC 
(In reply to Fridolin Somers from comment #5)
> (In reply to Fridolin Somers from comment #3)
> > 
> > Most important : 
> > Clicking on "Select all" selects the branchcode :O
> 
> Do you reproduce this ?
> Its a major issue no ?

I cannot reproduce this. When I click 'Select All' the branchcode option does appear to be select (it gets a check) but if I load the registration form I am able to complete it. Also if I click 'Select All' and save then immediately reopen  the modal you will see that branchcode is not selected.
Comment 7 Lucas Gass (lukeg) 2020-12-16 23:05:18 UTC 
Can we do the same for PatronSelfRegistrationBorrowerUnwantedField? With all the hard work Nick did it would not be easy to add easily add the 'exclusions: branchcode' line to opac.pref now
Comment 8 Lucas Gass (lukeg) 2020-12-16 23:06:27 UTC 
(In reply to Lucas Gass from comment #7)
> Can we do the same for PatronSelfRegistrationBorrowerUnwantedField? With all
> the hard work Nick did it would not be easy to add easily add the
> 'exclusions: branchcode' line to opac.pref now

it would be easy*
Comment 9 Fridolin Somers 2020-12-17 08:00:22 UTC 
(In reply to Lucas Gass from comment #6)
> (In reply to Fridolin Somers from comment #5)
> > (In reply to Fridolin Somers from comment #3)
> > > 
> > > Most important : 
> > > Clicking on "Select all" selects the branchcode :O
> > 
> > Do you reproduce this ?
> > Its a major issue no ?
> 
> I cannot reproduce this. When I click 'Select All' the branchcode option
> does appear to be select (it gets a check) but if I load the registration
> form I am able to complete it. Also if I click 'Select All' and save then
> immediately reopen  the modal you will see that branchcode is not selected.

Indeed, disabled inputs are not posted in forms.
But its very disturbing to see it checked in my opinion.

I try to create a fix
Comment 10 Fridolin Somers 2020-12-17 08:07:10 UTC 
Created attachment 114470 [details] [review]
Bug 17364: (follow-up) Do not check excluded fields

When clicking on "Select all" excluded fields are selected.
Even if they are not saved into preference, this is disturbing.

Test plan :
1) Edit BorroweUnwantedFields system preference
2) Click "Select all"
3) See that branchcode is not selected
Comment 11 Fridolin Somers 2020-12-17 08:08:31 UTC 
Was easy ^^

Maybe we need some CSS on excluded fields name, grey color for example.
Comment 12 Fridolin Somers 2020-12-17 08:10:36 UTC 
Created attachment 114471 [details] [review]
Bug 17364: Add ability to exclude some columns from selection for system preferences

Bug 22844 Added the option to provide a modal for selecting database columns for system preferences

Some system preferences are limited to a subset of the columns allowed. For instance, setting branchcode
in BorrowerUnwantedFields prevents adding new patrons

This patch allows for defining exclusions in the .pref file

To test:
1 - Alter BorroweUnwantedFields and select branchcode
2 - Attempt to add a new patron
3 - Get a message like "Something went wrong. Check the logs"
4 - Apply patches
5 - Alter the preference again
6 - Note that branchcode is unchecked and disabled
7 - Save the pref
8 - Add a patron
9 - It succeeds

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 13 Fridolin Somers 2020-12-17 08:10:55 UTC 
Created attachment 114472 [details] [review]
Bug 17364: (follow-up) Add missing filter

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 14 Fridolin Somers 2020-12-17 08:11:27 UTC 
Created attachment 114473 [details] [review]
Bug 17364: (follow-up) Do not check excluded fields

When clicking on "Select all" excluded fields are selected.
Even if they are not saved into preference, this is disturbing.

Test plan :
1) Edit BorroweUnwantedFields system preference
2) Click "Select all"
3) See that branchcode is not selected
Comment 15 Fridolin Somers 2020-12-17 08:12:03 UTC 
I signed 2 first patches, leaving NSO for my followup
Comment 16 Nick Clemens (kidclamp) 2020-12-17 11:26:50 UTC 
Created attachment 114475 [details] [review]
Bug 17364: (follow-up) Do not check excluded fields

When clicking on "Select all" excluded fields are selected.
Even if they are not saved into preference, this is disturbing.

Test plan :
1) Edit BorroweUnwantedFields system preference
2) Click "Select all"
3) See that branchcode is not selected

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Katrin Fischer 2021-01-12 22:43:07 UTC 
The option branchcode cannot be selected with this patch, but it's not visually clear. Could we make option appear greyed out or similar the GUI easier to understand?

And would it not be easier to not show the option at all if it's not available for a specific preference?

Please review!
Comment 18 Katrin Fischer 2021-01-12 22:44:42 UTC 
I found bug 27321 now - hope we can get a quick sign-off as this could solve the issue here!
Comment 19 David Nind 2021-01-30 02:22:51 UTC 
I've signed off bug 27321
Comment 20 Katrin Fischer 2021-01-31 14:27:29 UTC 
Created attachment 116114 [details] [review]
Bug 17364: Add ability to exclude some columns from selection for system preferences

Bug 22844 Added the option to provide a modal for selecting database columns for system preferences

Some system preferences are limited to a subset of the columns allowed. For instance, setting branchcode
in BorrowerUnwantedFields prevents adding new patrons

This patch allows for defining exclusions in the .pref file

To test:
1 - Alter BorroweUnwantedFields and select branchcode
2 - Attempt to add a new patron
3 - Get a message like "Something went wrong. Check the logs"
4 - Apply patches
5 - Alter the preference again
6 - Note that branchcode is unchecked and disabled
7 - Save the pref
8 - Add a patron
9 - It succeeds

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2021-01-31 14:27:33 UTC 
Created attachment 116115 [details] [review]
Bug 17364: (follow-up) Add missing filter

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Katrin Fischer 2021-01-31 14:27:38 UTC 
Created attachment 116116 [details] [review]
Bug 17364: (follow-up) Do not check excluded fields

When clicking on "Select all" excluded fields are selected.
Even if they are not saved into preference, this is disturbing.

Test plan :
1) Edit BorroweUnwantedFields system preference
2) Click "Select all"
3) See that branchcode is not selected

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Jonathan Druart 2021-02-01 16:26:15 UTC 
Pushed to master for 21.05, thanks to everybody involved!
Comment 24 Fridolin Somers 2021-02-05 15:43:25 UTC 
Pushed to 20.11.x for 20.11.03
Comment 25 Andrew Fuerste-Henry 2021-02-09 12:46:10 UTC 
Missing dependency, not backported to 20.05
Comment 26 Emily Lamancusa (emlam) 2023-07-10 20:50:43 UTC 
*** Bug 16865 has been marked as a duplicate of this bug. ***