Bug 22844 - Simplify the process of selecting database columns for system preferences
Summary: Simplify the process of selecting database columns for system preferences
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Owen Leonard
QA Contact: Marcel de Rooy
URL:
Keywords:
: 4030 (view as bug list)
Depends on: 22867 23410
Blocks: 17364 22728 23124 26283 27261 27378 28236 28392 28936
  Show dependency treegraph
 
Reported: 2019-05-03 18:01 UTC by Owen Leonard
Modified: 2022-12-12 21:23 UTC (History)
17 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement introduces a new way to select database columns for selected system preferences like BorrowerMandatoryField. Currently, this requires manually adding the database field names. The enhancement lets you select from a list of available fields in a new window, and also select and clear all fields. This is implemented for these system preferences: - BorrowerMandatoryField - BorrowerUnwantedField - PatronQuickAddFields - PatronSelfModificationBorrowerUnwantedField - PatronSelfRegistrationBorrowerMandatoryField - PatronSelfRegistrationBorrowerUnwantedField - StatisticsFields - UniqueItemFields
Version(s) released in:
20.11.00


Attachments
Screenshot showing column selection in a modal window (60.33 KB, image/png)
2019-05-03 18:01 UTC, Owen Leonard
Details
Bug 22844: Simplify the process of selecting database columns for system preferences (16.30 KB, patch)
2019-05-03 18:53 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22844: Simplify the process of selecting database columns for system preferences (16.08 KB, patch)
2019-05-06 18:08 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22844: Simplify the process of selecting database columns for system preferences (16.19 KB, patch)
2019-05-07 02:38 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 22844: Simplify the process of selecting database columns for system preferences (16.29 KB, patch)
2019-05-10 09:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22844: (QA follow-up) Add pref PatronQuickAddFields too (1.85 KB, patch)
2019-05-10 09:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22844: POC - Add patron attributes to the list (3.75 KB, patch)
2019-05-14 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Add the new DB column borrower_attribute_types.mandatory (2.03 KB, patch)
2019-05-17 14:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Add the new entry to the patron attr types form (6.48 KB, patch)
2019-05-17 14:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Make the attribute mandatory when editing a patron (3.42 KB, patch)
2019-05-17 14:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: (QA follow-up) Build items.json for UniqueItemFields (4.34 KB, patch)
2019-06-07 06:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22844: (follow-up) Make the attribute mandatory when editing a patron (2.71 KB, patch)
2019-12-05 10:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Simplify the process of selecting database columns for system preferences (16.30 KB, patch)
2020-01-30 10:07 UTC, David Nind
Details | Diff | Splinter Review
Bug 22844: (QA follow-up) Add pref PatronQuickAddFields too (1.86 KB, patch)
2020-01-30 10:07 UTC, David Nind
Details | Diff | Splinter Review
Bug 22844: Add the new DB column borrower_attribute_types.mandatory (2.07 KB, patch)
2020-01-30 10:08 UTC, David Nind
Details | Diff | Splinter Review
Bug 22844: Add the new entry to the patron attr types form (6.53 KB, patch)
2020-01-30 10:08 UTC, David Nind
Details | Diff | Splinter Review
Bug 22844: Make the attribute mandatory when editing a patron (3.47 KB, patch)
2020-01-30 10:08 UTC, David Nind
Details | Diff | Splinter Review
Bug 22844: (QA follow-up) Build items.json for UniqueItemFields (4.40 KB, patch)
2020-01-30 10:08 UTC, David Nind
Details | Diff | Splinter Review
Bug 22844: (follow-up) Make the attribute mandatory when editing a patron (2.76 KB, patch)
2020-01-30 10:08 UTC, David Nind
Details | Diff | Splinter Review
Bug 22844: DBIC Schema changes (1.49 KB, patch)
2020-02-03 13:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Add is_boolean flag for new 'mandatory' column (920 bytes, patch)
2020-02-03 13:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Simplify the process of selecting database columns for system preferences (16.13 KB, patch)
2020-03-10 11:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: (QA follow-up) Add pref PatronQuickAddFields too (1.85 KB, patch)
2020-03-10 11:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Add the new DB column borrower_attribute_types.mandatory (2.07 KB, patch)
2020-03-10 11:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Add the new entry to the patron attr types form (6.51 KB, patch)
2020-03-10 11:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Make the attribute mandatory when editing a patron (6.80 KB, patch)
2020-03-10 11:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: (QA follow-up) Build items.json for UniqueItemFields (4.38 KB, patch)
2020-03-10 11:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: DBIC Schema changes (1.49 KB, patch)
2020-03-10 11:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Add is_boolean flag for new 'mandatory' column (920 bytes, patch)
2020-03-10 11:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Simplify the process of selecting database columns for system preferences (16.13 KB, patch)
2020-03-20 14:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: (QA follow-up) Add pref PatronQuickAddFields too (2.08 KB, patch)
2020-03-20 14:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Add the new DB column borrower_attribute_types.mandatory (2.07 KB, patch)
2020-03-20 14:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Add the new entry to the patron attr types form (6.51 KB, patch)
2020-03-20 14:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Make the attribute mandatory when editing a patron (6.83 KB, patch)
2020-03-20 14:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: (QA follow-up) Build items.json for UniqueItemFields (4.38 KB, patch)
2020-03-20 14:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: DBIC Schema changes (1.49 KB, patch)
2020-03-20 14:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Add is_boolean flag for new 'mandatory' column (920 bytes, patch)
2020-03-20 14:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Simplify the process of selecting database columns for system preferences (16.20 KB, patch)
2020-03-20 17:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22844: Simplify the process of selecting database columns for system preferences (16.20 KB, patch)
2020-03-23 18:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22844: Simplify the process of selecting database columns for system preferences (16.20 KB, patch)
2020-03-23 18:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22844: (QA follow-up) Add pref PatronQuickAddFields too (2.15 KB, patch)
2020-03-23 18:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22844: Add the new DB column borrower_attribute_types.mandatory (2.15 KB, patch)
2020-03-23 18:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22844: Add the new entry to the patron attr types form (6.58 KB, patch)
2020-03-23 18:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22844: Make the attribute mandatory when editing a patron (6.91 KB, patch)
2020-03-23 18:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22844: (QA follow-up) Build items.json for UniqueItemFields (4.46 KB, patch)
2020-03-23 18:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22844: DBIC Schema changes (1.56 KB, patch)
2020-03-23 18:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22844: Add is_boolean flag for new 'mandatory' column (993 bytes, patch)
2020-03-23 18:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22844: Simplify the process of selecting database columns for system preferences (16.18 KB, patch)
2020-03-24 16:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: (QA follow-up) Add pref PatronQuickAddFields too (2.13 KB, patch)
2020-03-24 16:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Add the new DB column borrower_attribute_types.mandatory (2.13 KB, patch)
2020-03-24 16:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Add the new entry to the patron attr types form (3.23 KB, patch)
2020-03-24 16:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Make the attribute mandatory when editing a patron (6.89 KB, patch)
2020-03-24 16:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: (QA follow-up) Build items.json for UniqueItemFields (4.44 KB, patch)
2020-03-24 16:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: DBIC Schema changes (1.54 KB, patch)
2020-03-24 16:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Add is_boolean flag for new 'mandatory' column (975 bytes, patch)
2020-03-24 16:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Simplify the process of selecting database columns for system preferences (16.17 KB, patch)
2020-04-28 11:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: (QA follow-up) Add pref PatronQuickAddFields too (1.98 KB, patch)
2020-04-28 11:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Add the new DB column borrower_attribute_types.mandatory (2.13 KB, patch)
2020-04-28 11:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Add the new entry to the patron attr types form (3.25 KB, patch)
2020-04-28 11:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Make the attribute mandatory when editing a patron (6.85 KB, patch)
2020-04-28 11:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: (QA follow-up) Build items.json for UniqueItemFields (4.44 KB, patch)
2020-04-28 11:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: DBIC Schema changes (1.54 KB, patch)
2020-04-28 11:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Add is_boolean flag for new 'mandatory' column (975 bytes, patch)
2020-04-28 11:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Make the patron's attribute mandatory at the OPAC (4.87 KB, patch)
2020-04-28 11:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Remove 'separate columns with |' from prefs description (1.52 KB, patch)
2020-04-29 08:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22844: Simplify the process of selecting database columns for system preferences (16.29 KB, patch)
2020-04-29 16:36 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 22844: (QA follow-up) Add pref PatronQuickAddFields too (2.04 KB, patch)
2020-04-29 16:36 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 22844: Add the new DB column borrower_attribute_types.mandatory (2.19 KB, patch)
2020-04-29 16:37 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 22844: Add the new entry to the patron attr types form (3.30 KB, patch)
2020-04-29 16:37 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 22844: Make the attribute mandatory when editing a patron (6.91 KB, patch)
2020-04-29 16:37 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 22844: (QA follow-up) Build items.json for UniqueItemFields (4.50 KB, patch)
2020-04-29 16:37 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 22844: DBIC Schema changes (1.60 KB, patch)
2020-04-29 16:37 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 22844: Add is_boolean flag for new 'mandatory' column (1.01 KB, patch)
2020-04-29 16:37 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 22844: Make the patron's attribute mandatory at the OPAC (4.93 KB, patch)
2020-04-29 16:37 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 22844: Remove 'separate columns with |' from prefs description (1.57 KB, patch)
2020-04-29 16:37 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 22844: Simplify the process of selecting database columns for system preferences (16.40 KB, patch)
2020-07-05 10:59 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22844: (QA follow-up) Add pref PatronQuickAddFields too (2.11 KB, patch)
2020-07-05 10:59 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22844: Add the new DB column borrower_attribute_types.mandatory (2.24 KB, patch)
2020-07-05 10:59 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22844: Add the new entry to the patron attr types form (3.38 KB, patch)
2020-07-05 10:59 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22844: Make the attribute mandatory when editing a patron (6.98 KB, patch)
2020-07-05 10:59 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22844: (QA follow-up) Build items.json for UniqueItemFields (4.57 KB, patch)
2020-07-05 10:59 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22844: DBIC Schema changes (1.66 KB, patch)
2020-07-05 10:59 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22844: Add is_boolean flag for new 'mandatory' column (1.07 KB, patch)
2020-07-05 10:59 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22844: Make the patron's attribute mandatory at the OPAC (5.00 KB, patch)
2020-07-05 10:59 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22844: Remove 'separate columns with |' from prefs description (1.64 KB, patch)
2020-07-05 11:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22844: Fix yaml_valid.t - don't pick .json files (1.38 KB, patch)
2020-07-21 07:48 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 Owen Leonard 2019-05-03 18:01:24 UTC
Created attachment 89322 [details]
Screenshot showing column selection in a modal window

Right now the process of hand-typing the names of database columns in system preferences like BorrowerMandatoryField is cumbersome and error-prone. I would like to change to a system where the user can select from a predefined list of database columns
Comment 1 Owen Leonard 2019-05-03 18:53:18 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2019-05-06 18:08:25 UTC Comment hidden (obsolete)
Comment 3 Hayley Pelham 2019-05-07 02:38:31 UTC Comment hidden (obsolete)
Comment 4 Liz Rea 2019-05-07 16:04:06 UTC
I love this so much <heart eyes>
Comment 5 Jonathan Druart 2019-05-08 17:54:58 UTC
Owen, did you investigate columns.def?
Comment 6 Marcel de Rooy 2019-05-10 09:13:27 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2019-05-10 09:13:31 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2019-05-10 09:15:45 UTC
(In reply to Jonathan Druart from comment #5)
> Owen, did you investigate columns.def?

Good point, but no blocker for me at feature freeze date.
We probably should merge these things. Note that the items columns are no longer up to date in columns.def; see also 22867.
Comment 9 Owen Leonard 2019-05-10 11:56:41 UTC
(In reply to Marcel de Rooy from comment #8)
> (In reply to Jonathan Druart from comment #5)
> > Owen, did you investigate columns.def?
> 
> Good point, but no blocker for me at feature freeze date.
> We probably should merge these things.

I did investigate columns.def but I was looking for a solution which wouldn't require an additional script for converting columns.def into something which could be easily parsed by JavaScript. A unified solution would certainly be preferable. Do we need a script which has the ability to output JSON? Or would it be better to use JSON everywhere and modify the translation process to include JSON?
Comment 10 Jonathan Druart 2019-05-10 16:26:57 UTC
Quick note while testing: the "Save all XXX preferences" buttons do not behave correctly.
Comment 11 Jonathan Druart 2019-05-10 16:49:20 UTC
Actually there is something weird in master as well, the buttons are not disabled when clicked. Sounds like a regression, isn't it?
Comment 12 Nick Clemens 2019-05-10 19:15:45 UTC
I ran out of time for testing etc today, but this seems a culmination of a lot of work we have done on improving this interface this cycle, I would like to include it if we can resolve issues in the next few days
Comment 13 Jonathan Druart 2019-05-11 14:30:39 UTC
(In reply to Nick Clemens from comment #12)
> I ran out of time for testing etc today, but this seems a culmination of a
> lot of work we have done on improving this interface this cycle, I would
> like to include it if we can resolve issues in the next few days

We can push it and fix later, issues are minors.
Comment 14 Katrin Fischer 2019-05-13 17:56:41 UTC
Just remembered something: There is an underdocumented feature for making patron attributes mandatory, see bug 22728. I think that would no longer be possible with the new interface :(
Comment 15 Jonathan Druart 2019-05-14 14:49:48 UTC
210     $(".modalselect").on("click", function(){

Should be .preference I think.
Comment 16 Jonathan Druart 2019-05-14 14:51:27 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2019-05-14 14:52:14 UTC
IMO This is not ready for 19.05.00, could be part of 19.05.01 however.
Letting the decision to RM.
Comment 18 Nick Clemens 2019-05-15 17:17:52 UTC
I really like these, would like to see Joubu's patches tested so holding off for 19.05

One note too: With this patch I cannot see the full value of the pref or copy the text of the pref out - it would be nice to have the helper and also be able to directly edit the field. Not a blocker
Comment 19 Jonathan Druart 2019-05-17 14:07:43 UTC
Working on this.
Comment 20 Jonathan Druart 2019-05-17 14:34:24 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2019-05-17 14:34:27 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2019-05-17 14:34:31 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2019-05-17 14:48:26 UTC
Not sure what is the correct status here.

Either it's in Discussion and someone else gives a shoot at a follow-up for my patches.
Or we can also drop the "hidden feature", we obsolete my last 3 patches and switch back to PQA.

Or wait for a more complete and tested approach :)
(I personally vote for the later)
Comment 24 Marcel de Rooy 2019-05-22 14:23:03 UTC
(In reply to Nick Clemens from comment #18)
> One note too: With this patch I cannot see the full value of the pref or
> copy the text of the pref out - it would be nice to have the helper and also
> be able to directly edit the field. Not a blocker

+1
Comment 25 Marcel de Rooy 2019-05-22 14:25:15 UTC
(In reply to Jonathan Druart from comment #23)
> Or wait for a more complete and tested approach :)
> (I personally vote for the later)

+1 too
Comment 26 Martin Renvoize 2019-06-06 16:37:35 UTC
This introduces JSON files to the /preferences directory.. as such we'll need to update xt/yaml_valid.t to be a little more intelligent.
Comment 27 Martin Renvoize 2019-06-07 06:53:30 UTC Comment hidden (obsolete)
Comment 28 Martin Renvoize 2019-06-07 07:30:37 UTC
It feels like we're starting to mix concerns a bit here.

I would suggest splitting the `BorrowerMandatoryField` handling back out of this bug and putting it in bug 22728 and letting this bug start moving forward again.

Perhaps we could add the free text box itself into the bottom of the modal and track it's content to match the checkbox selection with some clever JS?
Comment 29 Owen Leonard 2019-10-01 18:59:22 UTC
(In reply to Martin Renvoize from comment #28)

> I would suggest splitting the `BorrowerMandatoryField` handling back out of
> this bug

So: I modify my original patch and remove the changes which modify the behavior of the BorrowerMandatoryField field and make the other patches obsolete.

> Perhaps we could add the free text box itself into the bottom of the modal

Is there any reason to do so in this patch if we're not concerned about patron attributes?
Comment 30 Jonathan Druart 2019-12-05 10:28:38 UTC Comment hidden (obsolete)
Comment 31 Jonathan Druart 2019-12-05 10:29:01 UTC
I think this is ready for testing!
Comment 32 David Nind 2020-01-30 10:07:52 UTC Comment hidden (obsolete)
Comment 33 David Nind 2020-01-30 10:07:56 UTC Comment hidden (obsolete)
Comment 34 David Nind 2020-01-30 10:08:00 UTC Comment hidden (obsolete)
Comment 35 David Nind 2020-01-30 10:08:04 UTC Comment hidden (obsolete)
Comment 36 David Nind 2020-01-30 10:08:08 UTC Comment hidden (obsolete)
Comment 37 David Nind 2020-01-30 10:08:12 UTC Comment hidden (obsolete)
Comment 38 David Nind 2020-01-30 10:08:16 UTC Comment hidden (obsolete)
Comment 39 David Nind 2020-01-30 10:09:29 UTC
Hope I tested this correctly - followed steps in comment 6 and 27, and everything seemed to work nicely!
Comment 40 Jonathan Druart 2020-02-03 13:50:47 UTC Comment hidden (obsolete)
Comment 41 Jonathan Druart 2020-02-03 13:50:51 UTC Comment hidden (obsolete)
Comment 42 Katrin Fischer 2020-02-29 22:55:55 UTC
Hi Owen, can you please rebase?
Comment 43 Jonathan Druart 2020-03-10 11:01:34 UTC Comment hidden (obsolete)
Comment 44 Jonathan Druart 2020-03-10 11:01:40 UTC Comment hidden (obsolete)
Comment 45 Jonathan Druart 2020-03-10 11:01:46 UTC Comment hidden (obsolete)
Comment 46 Jonathan Druart 2020-03-10 11:01:52 UTC Comment hidden (obsolete)
Comment 47 Jonathan Druart 2020-03-10 11:02:02 UTC Comment hidden (obsolete)
Comment 48 Jonathan Druart 2020-03-10 11:02:09 UTC Comment hidden (obsolete)
Comment 49 Jonathan Druart 2020-03-10 11:02:16 UTC Comment hidden (obsolete)
Comment 50 Jonathan Druart 2020-03-10 11:02:23 UTC Comment hidden (obsolete)
Comment 51 Jonathan Druart 2020-03-20 14:54:44 UTC Comment hidden (obsolete)
Comment 52 Jonathan Druart 2020-03-20 14:54:48 UTC Comment hidden (obsolete)
Comment 53 Jonathan Druart 2020-03-20 14:54:52 UTC Comment hidden (obsolete)
Comment 54 Jonathan Druart 2020-03-20 14:54:56 UTC Comment hidden (obsolete)
Comment 55 Jonathan Druart 2020-03-20 14:54:59 UTC Comment hidden (obsolete)
Comment 56 Jonathan Druart 2020-03-20 14:55:04 UTC Comment hidden (obsolete)
Comment 57 Jonathan Druart 2020-03-20 14:55:07 UTC Comment hidden (obsolete)
Comment 58 Jonathan Druart 2020-03-20 14:55:11 UTC Comment hidden (obsolete)
Comment 59 Kyle M Hall 2020-03-20 17:35:07 UTC Comment hidden (obsolete)
Comment 60 Kyle M Hall 2020-03-23 18:17:53 UTC Comment hidden (obsolete)
Comment 61 Kyle M Hall 2020-03-23 18:18:09 UTC
Created attachment 101481 [details] [review]
Bug 22844: Simplify the process of selecting database columns for system preferences

This patch introduces a new way for users to select database columns for
system preferences like BorrowerMandatoryField, which currently
require hand-typing of database names.

This new system uses a JSON file containing label:column pairs for
database columns which are relevant to preferences which reference
borrower table columns. My intention was to have user-friendly values as
the labels, but embedding English strings in JSON would make them
untranslatable.

The following preferences are affected:

 - BorrowerMandatoryField
 - BorrowerUnwantedField
 - PatronSelfModificationBorrowerUnwantedField
 - PatronSelfRegistrationBorrowerMandatoryField
 - PatronSelfRegistrationBorrowerUnwantedField

To test, apply the patch and regenerate the staff client CSS.
restart_all to make sure the updated .pref file is used.

 - Go to Administration -> System preferences, and search for
   "PatronSelf"
 - The input fields for PatronSelfModificationBorrowerUnwantedField,
   PatronSelfRegistrationBorrowerMandatoryField, and
   PatronSelfRegistrationBorrowerUnwantedField should appear as "locked"
   (read-only) inputs.
 - Clicking the input field should trigger a modal window with
   checkboxes for each available column from the borrowers table.
 - Test that the "select all" and "clear all" links work correctly.
 - Test that the "cancel" link closes the modal without saving your
   selections.
 - Test that the "Save" button closes the modal, copies your selections
   to the form field, and triggers the preference-saving function (this
   eliminates the need to click a save button again after closing the
   modal).
   - Test this process by making modifications to all three different
     preferences, confirming that the right data is preselected each
     time the modal is shown and the right data is saved to the right
     field each time.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 62 Kyle M Hall 2020-03-23 18:18:22 UTC
Created attachment 101482 [details] [review]
Bug 22844: (QA follow-up) Add pref PatronQuickAddFields too

Description slightly adjusted.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 63 Kyle M Hall 2020-03-23 18:18:25 UTC
Created attachment 101483 [details] [review]
Bug 22844: Add the new DB column borrower_attribute_types.mandatory

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 64 Kyle M Hall 2020-03-23 18:18:28 UTC
Created attachment 101484 [details] [review]
Bug 22844: Add the new entry to the patron attr types form

Will conflict with bug 20443 \o/

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 65 Kyle M Hall 2020-03-23 18:18:31 UTC
Created attachment 101485 [details] [review]
Bug 22844: Make the attribute mandatory when editing a patron

Note: I am not confident with this patch, I think it's not polished. I
will not have time to improve it to make it ready for 19.05.00
1. Conflict with bug 20443 (which would have make this change way much
easier!)
2. It does not work :) You will be able to submit the memberentry form
even if the patron attribute is marked as mandatory (??)
3. What about the OPAC?
4. What about repeatable fields? We certainly will need JS code here
5. What about the "Quick add" feature? (I had trouble in the past to not
introduce regression when we played with this template...)

Do not forget to run updatedatabase.pl and regenerate DBIC schema if you
want to play with this patchset.

Signed-off-by: David Nind <david@davidnind.com>

Bug 22844: (follow-up) Make the attribute mandatory when editing a patron

Previous patch forgot the most important, adding the required attribute
to the select/textarea

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 66 Kyle M Hall 2020-03-23 18:18:35 UTC
Created attachment 101486 [details] [review]
Bug 22844: (QA follow-up) Build items.json for UniqueItemFields

Adding items.json, built from latest kohastructure.sql.
Control UniqueItemFields with this source file.
And add StatisticsFields.

Test plan:
Go to Preferences, play with UniqueItemFields.
Adjust StatisticsFields and look at patron record, statistics tab.

Moved-from: Bug 22867
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 67 Kyle M Hall 2020-03-23 18:18:39 UTC
Created attachment 101487 [details] [review]
Bug 22844: DBIC Schema changes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 68 Kyle M Hall 2020-03-23 18:18:43 UTC
Created attachment 101488 [details] [review]
Bug 22844: Add is_boolean flag for new 'mandatory' column

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 69 Martin Renvoize 2020-03-24 11:20:21 UTC
Sorry guys.. this one no longer applies.. I think the rebase is fairly trivial but I'd be more confident if it was done by someone closer to the working code here..

Jonathan or Owen perhaps?
Comment 70 Jonathan Druart 2020-03-24 11:56:38 UTC
I will!
Comment 71 Jonathan Druart 2020-03-24 16:08:41 UTC
Created attachment 101617 [details] [review]
Bug 22844: Simplify the process of selecting database columns for system preferences

This patch introduces a new way for users to select database columns for
system preferences like BorrowerMandatoryField, which currently
require hand-typing of database names.

This new system uses a JSON file containing label:column pairs for
database columns which are relevant to preferences which reference
borrower table columns. My intention was to have user-friendly values as
the labels, but embedding English strings in JSON would make them
untranslatable.

The following preferences are affected:

 - BorrowerMandatoryField
 - BorrowerUnwantedField
 - PatronSelfModificationBorrowerUnwantedField
 - PatronSelfRegistrationBorrowerMandatoryField
 - PatronSelfRegistrationBorrowerUnwantedField

To test, apply the patch and regenerate the staff client CSS.
restart_all to make sure the updated .pref file is used.

 - Go to Administration -> System preferences, and search for
   "PatronSelf"
 - The input fields for PatronSelfModificationBorrowerUnwantedField,
   PatronSelfRegistrationBorrowerMandatoryField, and
   PatronSelfRegistrationBorrowerUnwantedField should appear as "locked"
   (read-only) inputs.
 - Clicking the input field should trigger a modal window with
   checkboxes for each available column from the borrowers table.
 - Test that the "select all" and "clear all" links work correctly.
 - Test that the "cancel" link closes the modal without saving your
   selections.
 - Test that the "Save" button closes the modal, copies your selections
   to the form field, and triggers the preference-saving function (this
   eliminates the need to click a save button again after closing the
   modal).
   - Test this process by making modifications to all three different
     preferences, confirming that the right data is preselected each
     time the modal is shown and the right data is saved to the right
     field each time.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 72 Jonathan Druart 2020-03-24 16:08:46 UTC
Created attachment 101618 [details] [review]
Bug 22844: (QA follow-up) Add pref PatronQuickAddFields too

Description slightly adjusted.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 73 Jonathan Druart 2020-03-24 16:08:52 UTC
Created attachment 101619 [details] [review]
Bug 22844: Add the new DB column borrower_attribute_types.mandatory

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 74 Jonathan Druart 2020-03-24 16:08:57 UTC
Created attachment 101620 [details] [review]
Bug 22844: Add the new entry to the patron attr types form

Will conflict with bug 20443 \o/

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 75 Jonathan Druart 2020-03-24 16:09:03 UTC
Created attachment 101621 [details] [review]
Bug 22844: Make the attribute mandatory when editing a patron

Note: I am not confident with this patch, I think it's not polished. I
will not have time to improve it to make it ready for 19.05.00
1. Conflict with bug 20443 (which would have make this change way much
easier!)
2. It does not work :) You will be able to submit the memberentry form
even if the patron attribute is marked as mandatory (??)
3. What about the OPAC?
4. What about repeatable fields? We certainly will need JS code here
5. What about the "Quick add" feature? (I had trouble in the past to not
introduce regression when we played with this template...)

Do not forget to run updatedatabase.pl and regenerate DBIC schema if you
want to play with this patchset.

Signed-off-by: David Nind <david@davidnind.com>

Bug 22844: (follow-up) Make the attribute mandatory when editing a patron

Previous patch forgot the most important, adding the required attribute
to the select/textarea

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 76 Jonathan Druart 2020-03-24 16:09:09 UTC
Created attachment 101622 [details] [review]
Bug 22844: (QA follow-up) Build items.json for UniqueItemFields

Adding items.json, built from latest kohastructure.sql.
Control UniqueItemFields with this source file.
And add StatisticsFields.

Test plan:
Go to Preferences, play with UniqueItemFields.
Adjust StatisticsFields and look at patron record, statistics tab.

Moved-from: Bug 22867
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 77 Jonathan Druart 2020-03-24 16:09:15 UTC
Created attachment 101623 [details] [review]
Bug 22844: DBIC Schema changes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 78 Jonathan Druart 2020-03-24 16:09:20 UTC
Created attachment 101624 [details] [review]
Bug 22844: Add is_boolean flag for new 'mandatory' column

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 79 Jonathan Druart 2020-03-24 16:18:35 UTC
Switching this back to PQA, but I have the feeling that we are missing something.
Regarding the commit message from patch "Bug 22844: Make the attribute mandatory when editing a patron", there are still some stuffs missing.
Comment 80 Jonathan Druart 2020-03-31 10:01:10 UTC
Removing from PQA, waiting an answer on the different points of the commit message:

Note: I am not confident with this patch, I think it's not polished. I
will not have time to improve it to make it ready for 19.05.00
1. Conflict with bug 20443 (which would have make this change way much
easier!)
2. It does not work :) You will be able to submit the memberentry form
even if the patron attribute is marked as mandatory (??)
3. What about the OPAC?
4. What about repeatable fields? We certainly will need JS code here
5. What about the "Quick add" feature? (I had trouble in the past to not
introduce regression when we played with this template...)

You can forget about 1, I rebased the patches on top of it (which is pushed)
Comment 81 Jonathan Druart 2020-04-28 11:21:22 UTC
Created attachment 103842 [details] [review]
Bug 22844: Simplify the process of selecting database columns for system preferences

This patch introduces a new way for users to select database columns for
system preferences like BorrowerMandatoryField, which currently
require hand-typing of database names.

This new system uses a JSON file containing label:column pairs for
database columns which are relevant to preferences which reference
borrower table columns. My intention was to have user-friendly values as
the labels, but embedding English strings in JSON would make them
untranslatable.

The following preferences are affected:

 - BorrowerMandatoryField
 - BorrowerUnwantedField
 - PatronSelfModificationBorrowerUnwantedField
 - PatronSelfRegistrationBorrowerMandatoryField
 - PatronSelfRegistrationBorrowerUnwantedField

To test, apply the patch and regenerate the staff client CSS.
restart_all to make sure the updated .pref file is used.

 - Go to Administration -> System preferences, and search for
   "PatronSelf"
 - The input fields for PatronSelfModificationBorrowerUnwantedField,
   PatronSelfRegistrationBorrowerMandatoryField, and
   PatronSelfRegistrationBorrowerUnwantedField should appear as "locked"
   (read-only) inputs.
 - Clicking the input field should trigger a modal window with
   checkboxes for each available column from the borrowers table.
 - Test that the "select all" and "clear all" links work correctly.
 - Test that the "cancel" link closes the modal without saving your
   selections.
 - Test that the "Save" button closes the modal, copies your selections
   to the form field, and triggers the preference-saving function (this
   eliminates the need to click a save button again after closing the
   modal).
   - Test this process by making modifications to all three different
     preferences, confirming that the right data is preselected each
     time the modal is shown and the right data is saved to the right
     field each time.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 82 Jonathan Druart 2020-04-28 11:21:26 UTC
Created attachment 103843 [details] [review]
Bug 22844: (QA follow-up) Add pref PatronQuickAddFields too

Description slightly adjusted.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 83 Jonathan Druart 2020-04-28 11:21:30 UTC
Created attachment 103844 [details] [review]
Bug 22844: Add the new DB column borrower_attribute_types.mandatory

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 84 Jonathan Druart 2020-04-28 11:21:34 UTC
Created attachment 103845 [details] [review]
Bug 22844: Add the new entry to the patron attr types form

Will conflict with bug 20443 \o/

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 85 Jonathan Druart 2020-04-28 11:21:38 UTC
Created attachment 103846 [details] [review]
Bug 22844: Make the attribute mandatory when editing a patron

Note: I am not confident with this patch, I think it's not polished. I
will not have time to improve it to make it ready for 19.05.00
1. Conflict with bug 20443 (which would have make this change way much
easier!)
2. It does not work :) You will be able to submit the memberentry form
even if the patron attribute is marked as mandatory (??)
3. What about the OPAC?
4. What about repeatable fields? We certainly will need JS code here
5. What about the "Quick add" feature? (I had trouble in the past to not
introduce regression when we played with this template...)

Do not forget to run updatedatabase.pl and regenerate DBIC schema if you
want to play with this patchset.

Signed-off-by: David Nind <david@davidnind.com>

Bug 22844: (follow-up) Make the attribute mandatory when editing a patron

Previous patch forgot the most important, adding the required attribute
to the select/textarea

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 86 Jonathan Druart 2020-04-28 11:21:43 UTC
Created attachment 103847 [details] [review]
Bug 22844: (QA follow-up) Build items.json for UniqueItemFields

Adding items.json, built from latest kohastructure.sql.
Control UniqueItemFields with this source file.
And add StatisticsFields.

Test plan:
Go to Preferences, play with UniqueItemFields.
Adjust StatisticsFields and look at patron record, statistics tab.

Moved-from: Bug 22867
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 87 Jonathan Druart 2020-04-28 11:21:47 UTC
Created attachment 103848 [details] [review]
Bug 22844: DBIC Schema changes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 88 Jonathan Druart 2020-04-28 11:21:51 UTC
Created attachment 103849 [details] [review]
Bug 22844: Add is_boolean flag for new 'mandatory' column

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 89 Jonathan Druart 2020-04-28 11:21:55 UTC
Created attachment 103850 [details] [review]
Bug 22844: Make the patron's attribute mandatory at the OPAC

Test plan:
- Set 1+ patron's attribute(s) mandatory
- Use the self-registration feature and confirm that you cannot selfreg
if the attribute has no value (or empty string)
- Same with the modification form (logged in)
Comment 90 Jonathan Druart 2020-04-28 11:22:56 UTC
Could this whole patch set be retested?
The last patch should fix the OPAC behaviours (self-reg and modification)
Comment 91 David Nind 2020-04-28 13:04:15 UTC
Just a couple of things from running through the testing again:

0) The mandatorty fields for self registration and editing worked for me!

1) The image attached to the bug shows the human readable field name then the system field name in brackets. With the patches only the system field name is shown. Not a show stopper to get this really nice feature into the next release, but from a 'user' perspective having it the way in the image looks much nicer to me.

2) These system preferences need the text removed about separating the columns with an |:
   - BorrowerUnwantedField (separate columns with |) 
   - PatronQuickAddFields (separate columns with |)

My other notes from testing using koha-testing-docker (so I remember to do in the right order):
- apply patches
- updatedatabase
- dbic
- updated CSS: npm install -E then yarn build
- flush_memcached
- restart_all
Comment 92 Jonathan Druart 2020-04-28 13:12:19 UTC
(In reply to David Nind from comment #91)
> Just a couple of things from running through the testing again:
> 
> 0) The mandatorty fields for self registration and editing worked for me!

The patron's attributes? Not that's not possible it worked before the last patch (from today). The "usual" mandatory fields worked, yes.

> 1) The image attached to the bug shows the human readable field name then
> the system field name in brackets. With the patches only the system field
> name is shown. Not a show stopper to get this really nice feature into the
> next release, but from a 'user' perspective having it the way in the image
> looks much nicer to me.

You should regenerate the css file with `yarn build css`.

> 2) These system preferences need the text removed about separating the
> columns with an |:
>    - BorrowerUnwantedField (separate columns with |) 
>    - PatronQuickAddFields (separate columns with |)

Indeed, I will attach a follow-up later.
Comment 93 Jonathan Druart 2020-04-28 13:17:38 UTC
David, reading the last comments I noticed it may not be obvious:

This patch set has been delayed because of the patron's attributes. In current master there is a hidden feature: you can insert patron's attribute codes in the syspref, and it makes the patron's attribute mandatory in the form.
With only the first patches (and the pref), we dropped this feature: only the fields from the borrowers table were displayed.

With the next iteration we add the ability to define a patron's attribute mandatory (on the edit form of the patron's attribute).

The last patch should make the mandatory patron's attributes mandatory at the OPAC (self-registration and modification).

Let me know if you have any questions!
Comment 94 Victor Grousset/tuxayo 2020-04-29 04:18:44 UTC
Great enhancement! :D

I tested everything I think.
The 3 test plans of the patchset:
- comment 81
- comment 86
- comment 89
The additional test notes from
- comment 85
- comment 91

Anything missing to sign it off?

David:
>> 1) The image attached to the bug shows the human readable field name then
>> the system field name in brackets. With the patches only the system field
>> name is shown. Not a show stopper to get this really nice feature into the
>> next release, but from a 'user' perspective having it the way in the image
>> looks much nicer to me.


Jonathan:
> You should regenerate the css file with `yarn build css`.

I'm also getting only the system field name.

Not a show stopper IMHO.
Comment 95 Jonathan Druart 2020-04-29 08:38:04 UTC
Created attachment 103904 [details] [review]
Bug 22844: Remove 'separate columns with |' from prefs description
Comment 96 Jonathan Druart 2020-04-29 08:38:26 UTC
(In reply to Victor Grousset/tuxayo from comment #94)
> David:
> >> 1) The image attached to the bug shows the human readable field name then
> >> the system field name in brackets. With the patches only the system field
> >> name is shown. Not a show stopper to get this really nice feature into the
> >> next release, but from a 'user' perspective having it the way in the image
> >> looks much nicer to me.
> 
> 
> Jonathan:
> > You should regenerate the css file with `yarn build css`.
> 
> I'm also getting only the system field name.
> 
> Not a show stopper IMHO.

I read too quickly, that's the expected behaviour indeed. Could be improved later if needed.
Comment 97 Victor Grousset/tuxayo 2020-04-29 16:36:50 UTC
Created attachment 103960 [details] [review]
Bug 22844: Simplify the process of selecting database columns for system preferences

This patch introduces a new way for users to select database columns for
system preferences like BorrowerMandatoryField, which currently
require hand-typing of database names.

This new system uses a JSON file containing label:column pairs for
database columns which are relevant to preferences which reference
borrower table columns. My intention was to have user-friendly values as
the labels, but embedding English strings in JSON would make them
untranslatable.

The following preferences are affected:

 - BorrowerMandatoryField
 - BorrowerUnwantedField
 - PatronSelfModificationBorrowerUnwantedField
 - PatronSelfRegistrationBorrowerMandatoryField
 - PatronSelfRegistrationBorrowerUnwantedField

== Test plan ==
 - apply the patches
 - regenerate the staff client CSS (yarn build)
 - updatedatabase
 - dbic
 - flush_memcached
 - restart_all to make sure the updated .pref file is used

 - Go to Administration -> System preferences, and search for
   "PatronSelf"
 - The input fields for PatronSelfModificationBorrowerUnwantedField,
   PatronSelfRegistrationBorrowerMandatoryField, and
   PatronSelfRegistrationBorrowerUnwantedField should appear as "locked"
   (read-only) inputs.
 - Clicking the input field should trigger a modal window with
   checkboxes for each available column from the borrowers table.
 - Test that the "select all" and "clear all" links work correctly.
 - Test that the "cancel" link closes the modal without saving your
   selections.
 - Test that the "Save" button closes the modal, copies your selections
   to the form field, and triggers the preference-saving function (this
   eliminates the need to click a save button again after closing the
   modal).
   - Test this process by making modifications to all three different
     preferences, confirming that the right data is preselected each
     time the modal is shown and the right data is saved to the right
     field each time.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 98 Victor Grousset/tuxayo 2020-04-29 16:36:56 UTC
Created attachment 103961 [details] [review]
Bug 22844: (QA follow-up) Add pref PatronQuickAddFields too

Description slightly adjusted.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 99 Victor Grousset/tuxayo 2020-04-29 16:37:01 UTC
Created attachment 103962 [details] [review]
Bug 22844: Add the new DB column borrower_attribute_types.mandatory

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 100 Victor Grousset/tuxayo 2020-04-29 16:37:06 UTC
Created attachment 103963 [details] [review]
Bug 22844: Add the new entry to the patron attr types form

Will conflict with bug 20443 \o/

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 101 Victor Grousset/tuxayo 2020-04-29 16:37:10 UTC
Created attachment 103964 [details] [review]
Bug 22844: Make the attribute mandatory when editing a patron

Note: I am not confident with this patch, I think it's not polished. I
will not have time to improve it to make it ready for 19.05.00
1. Conflict with bug 20443 (which would have make this change way much
easier!)
2. It does not work :) You will be able to submit the memberentry form
even if the patron attribute is marked as mandatory (??)
3. What about the OPAC?
4. What about repeatable fields? We certainly will need JS code here
5. What about the "Quick add" feature? (I had trouble in the past to not
introduce regression when we played with this template...)

Do not forget to run updatedatabase.pl and regenerate DBIC schema if you
want to play with this patchset.

Signed-off-by: David Nind <david@davidnind.com>

Bug 22844: (follow-up) Make the attribute mandatory when editing a patron

Previous patch forgot the most important, adding the required attribute
to the select/textarea

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 102 Victor Grousset/tuxayo 2020-04-29 16:37:15 UTC
Created attachment 103965 [details] [review]
Bug 22844: (QA follow-up) Build items.json for UniqueItemFields

Adding items.json, built from latest kohastructure.sql.
Control UniqueItemFields with this source file.
And add StatisticsFields.

Test plan:
Go to Preferences, play with UniqueItemFields.
Adjust StatisticsFields and look at patron record, statistics tab.

Moved-from: Bug 22867
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 103 Victor Grousset/tuxayo 2020-04-29 16:37:20 UTC
Created attachment 103966 [details] [review]
Bug 22844: DBIC Schema changes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 104 Victor Grousset/tuxayo 2020-04-29 16:37:25 UTC
Created attachment 103967 [details] [review]
Bug 22844: Add is_boolean flag for new 'mandatory' column

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 105 Victor Grousset/tuxayo 2020-04-29 16:37:30 UTC
Created attachment 103968 [details] [review]
Bug 22844: Make the patron's attribute mandatory at the OPAC

Test plan:
- Set 1+ patron's attribute(s) mandatory
- Use the self-registration feature and confirm that you cannot selfreg
if the attribute has no value (or empty string)
- Same with the modification form (logged in)

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 106 Victor Grousset/tuxayo 2020-04-29 16:37:35 UTC
Created attachment 103969 [details] [review]
Bug 22844: Remove 'separate columns with |' from prefs description

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 107 Victor Grousset/tuxayo 2020-04-29 16:43:02 UTC
Thanks for the fix Jonathan, let's sign off!

Some items have been added to the test plan of the first patch. It now looks like this:
>     - apply the patches
>     - regenerate the staff client CSS (yarn build)
>     - updatedatabase
>     - dbic
>     - flush_memcached
>     - restart_all to make sure the updated .pref file is used
Comment 108 David Nind 2020-04-29 17:47:57 UTC
Hi Jonathan and Victor.

Apologies for the delay in replying - it got crazy busy at work in the last day or so.

Thanks for the follow-up patch for the system preference text.

I had regenerated the CSS, so the change was displaying correctly. What I was trying to say was it looked nicer showing the field label with the field name in brackets after it (as per the original screenshot), but that it wasn't a show stopper and doing that latter is fine.

For making patron attributes mandatory in self registration, I used the wrong term I think. I only tried making standard patron fields mandatory for self registration and editing - which worked as expected. I hadn't tried with custom patron attributes.

Glad to see it is now signed off, as it is a really nice feature!

David
Comment 109 Katrin Fischer 2020-07-05 10:59:07 UTC
Created attachment 106545 [details] [review]
Bug 22844: Simplify the process of selecting database columns for system preferences

This patch introduces a new way for users to select database columns for
system preferences like BorrowerMandatoryField, which currently
require hand-typing of database names.

This new system uses a JSON file containing label:column pairs for
database columns which are relevant to preferences which reference
borrower table columns. My intention was to have user-friendly values as
the labels, but embedding English strings in JSON would make them
untranslatable.

The following preferences are affected:

 - BorrowerMandatoryField
 - BorrowerUnwantedField
 - PatronSelfModificationBorrowerUnwantedField
 - PatronSelfRegistrationBorrowerMandatoryField
 - PatronSelfRegistrationBorrowerUnwantedField

== Test plan ==
 - apply the patches
 - regenerate the staff client CSS (yarn build)
 - updatedatabase
 - dbic
 - flush_memcached
 - restart_all to make sure the updated .pref file is used

 - Go to Administration -> System preferences, and search for
   "PatronSelf"
 - The input fields for PatronSelfModificationBorrowerUnwantedField,
   PatronSelfRegistrationBorrowerMandatoryField, and
   PatronSelfRegistrationBorrowerUnwantedField should appear as "locked"
   (read-only) inputs.
 - Clicking the input field should trigger a modal window with
   checkboxes for each available column from the borrowers table.
 - Test that the "select all" and "clear all" links work correctly.
 - Test that the "cancel" link closes the modal without saving your
   selections.
 - Test that the "Save" button closes the modal, copies your selections
   to the form field, and triggers the preference-saving function (this
   eliminates the need to click a save button again after closing the
   modal).
   - Test this process by making modifications to all three different
     preferences, confirming that the right data is preselected each
     time the modal is shown and the right data is saved to the right
     field each time.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 110 Katrin Fischer 2020-07-05 10:59:13 UTC
Created attachment 106546 [details] [review]
Bug 22844: (QA follow-up) Add pref PatronQuickAddFields too

Description slightly adjusted.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 111 Katrin Fischer 2020-07-05 10:59:18 UTC
Created attachment 106547 [details] [review]
Bug 22844: Add the new DB column borrower_attribute_types.mandatory

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 112 Katrin Fischer 2020-07-05 10:59:23 UTC
Created attachment 106548 [details] [review]
Bug 22844: Add the new entry to the patron attr types form

Will conflict with bug 20443 \o/

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 113 Katrin Fischer 2020-07-05 10:59:30 UTC
Created attachment 106549 [details] [review]
Bug 22844: Make the attribute mandatory when editing a patron

Note: I am not confident with this patch, I think it's not polished. I
will not have time to improve it to make it ready for 19.05.00
1. Conflict with bug 20443 (which would have make this change way much
easier!)
2. It does not work :) You will be able to submit the memberentry form
even if the patron attribute is marked as mandatory (??)
3. What about the OPAC?
4. What about repeatable fields? We certainly will need JS code here
5. What about the "Quick add" feature? (I had trouble in the past to not
introduce regression when we played with this template...)

Do not forget to run updatedatabase.pl and regenerate DBIC schema if you
want to play with this patchset.

Signed-off-by: David Nind <david@davidnind.com>

Bug 22844: (follow-up) Make the attribute mandatory when editing a patron

Previous patch forgot the most important, adding the required attribute
to the select/textarea

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 114 Katrin Fischer 2020-07-05 10:59:36 UTC
Created attachment 106550 [details] [review]
Bug 22844: (QA follow-up) Build items.json for UniqueItemFields

Adding items.json, built from latest kohastructure.sql.
Control UniqueItemFields with this source file.
And add StatisticsFields.

Test plan:
Go to Preferences, play with UniqueItemFields.
Adjust StatisticsFields and look at patron record, statistics tab.

Moved-from: Bug 22867
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 115 Katrin Fischer 2020-07-05 10:59:42 UTC
Created attachment 106551 [details] [review]
Bug 22844: DBIC Schema changes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 116 Katrin Fischer 2020-07-05 10:59:48 UTC
Created attachment 106552 [details] [review]
Bug 22844: Add is_boolean flag for new 'mandatory' column

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 117 Katrin Fischer 2020-07-05 10:59:54 UTC
Created attachment 106553 [details] [review]
Bug 22844: Make the patron's attribute mandatory at the OPAC

Test plan:
- Set 1+ patron's attribute(s) mandatory
- Use the self-registration feature and confirm that you cannot selfreg
if the attribute has no value (or empty string)
- Same with the modification form (logged in)

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 118 Katrin Fischer 2020-07-05 11:00:00 UTC
Created attachment 106554 [details] [review]
Bug 22844: Remove 'separate columns with |' from prefs description

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 119 Katrin Fischer 2020-07-05 11:01:06 UTC
I really like the suggested solution with the mandatory flag on the patron attribute configuration page - it fits really well there with the other things. I'd like to see this new feature advertised a bit more for the release notes.
Comment 120 Katrin Fischer 2020-07-06 17:03:15 UTC
*** Bug 4030 has been marked as a duplicate of this bug. ***
Comment 121 Jonathan Druart 2020-07-20 07:36:18 UTC
Katrin, can you confirm you have tested mandatory patron's attributes?
See comment 93.
Comment 122 Katrin Fischer 2020-07-20 07:49:25 UTC
(In reply to Jonathan Druart from comment #121)
> Katrin, can you confirm you have tested mandatory patron's attributes?
> See comment 93.

I believe I did, but it's been too long now. Do you have a specific case you need tested?
Comment 123 Jonathan Druart 2020-07-20 09:44:13 UTC
I listed some ideas in comment 80.
Comment 124 Jonathan Druart 2020-07-20 15:47:55 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 125 Jonathan Druart 2020-07-21 07:48:25 UTC
Created attachment 107126 [details] [review]
Bug 22844: Fix yaml_valid.t - don't pick .json files

Only .pref are considered yaml files.

kohadev-koha@kohadevbox:/kohadevbox/koha$ time prove xt/yaml_valid.t
xt/yaml_valid.t .. 1/19
 #   Failed test 'borrowers.json is YAML'
 #   at xt/yaml_valid.t line 39.

 #   Failed test 'items.json is YAML'
 #   at xt/yaml_valid.t line 39.
 # Looks like you failed 2 tests of 19.
Comment 126 Jonathan Druart 2020-07-21 10:24:05 UTC
Follow-up pushed to master for 20.11
Comment 127 Lucas Gass 2020-07-25 18:31:24 UTC
Not backporting this enhancement to 20.05.x series.