Bug 22844 - Simplify the process of selecting database columns for system preferences
Summary: Simplify the process of selecting database columns for system preferences
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Owen Leonard
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords: RM_priority
Depends on: 22867
Blocks: 22728 23124
  Show dependency treegraph
 
Reported: 2019-05-03 18:01 UTC by Owen Leonard
Modified: 2019-06-20 00:18 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


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 Mapley
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

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 Mapley 2019-05-07 02:38:31 UTC
Created attachment 89403 [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>
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
Created attachment 89530 [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>
Comment 7 Marcel de Rooy 2019-05-10 09:13:31 UTC
Created attachment 89531 [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>
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
Created attachment 89749 [details] [review]
Bug 22844: POC - Add patron attributes to the list

TODO:
- Handle 'checked' flag
- Update code in memberentry.tt
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
Created attachment 89903 [details] [review]
Bug 22844: Add the new DB column borrower_attribute_types.mandatory
Comment 21 Jonathan Druart 2019-05-17 14:34:27 UTC
Created attachment 89904 [details] [review]
Bug 22844: Add the new entry to the patron attr types form

Will conflict with bug 20443 \o/
Comment 22 Jonathan Druart 2019-05-17 14:34:31 UTC
Created attachment 89905 [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.
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
Created attachment 90407 [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>
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?