Created attachment 89323 [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 ("Secondary email": "emailpro") for database columns which are relevant to preferences which reference borrower table columns. 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. Created attachment 89389 [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. 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> I love this so much <heart eyes> Owen, did you investigate columns.def? 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> 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> (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. (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? Quick note while testing: the "Save all XXX preferences" buttons do not behave correctly. Actually there is something weird in master as well, the buttons are not disabled when clicked. Sounds like a regression, isn't it? 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 (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. 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 :( 210 $(".modalselect").on("click", function(){
Should be .preference I think.
Created attachment 89749 [details] [review] Bug 22844: POC - Add patron attributes to the list TODO: - Handle 'checked' flag - Update code in memberentry.tt IMO This is not ready for 19.05.00, could be part of 19.05.01 however. Letting the decision to RM. 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 Working on this. Created attachment 89903 [details] [review] Bug 22844: Add the new DB column borrower_attribute_types.mandatory Created attachment 89904 [details] [review] Bug 22844: Add the new entry to the patron attr types form Will conflict with bug 20443 \o/ 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. 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) (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 (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 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. 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> 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? (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? Created attachment 96007 [details] [review] 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 I think this is ready for testing! Created attachment 98145 [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> Created attachment 98146 [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> Created attachment 98147 [details] [review] Bug 22844: Add the new DB column borrower_attribute_types.mandatory Signed-off-by: David Nind <david@davidnind.com> Created attachment 98148 [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> Created attachment 98149 [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> Created attachment 98150 [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> Created attachment 98151 [details] [review] 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> Hope I tested this correctly - followed steps in comment 6 and 27, and everything seemed to work nicely! Created attachment 98297 [details] [review] Bug 22844: DBIC Schema changes Created attachment 98298 [details] [review] Bug 22844: Add is_boolean flag for new 'mandatory' column Hi Owen, can you please rebase? Created attachment 100440 [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> Created attachment 100441 [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> Created attachment 100442 [details] [review] Bug 22844: Add the new DB column borrower_attribute_types.mandatory Signed-off-by: David Nind <david@davidnind.com> Created attachment 100443 [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> Created attachment 100444 [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> Created attachment 100445 [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> Created attachment 100446 [details] [review] Bug 22844: DBIC Schema changes Created attachment 100447 [details] [review] Bug 22844: Add is_boolean flag for new 'mandatory' column Created attachment 101198 [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> Created attachment 101199 [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> Created attachment 101200 [details] [review] Bug 22844: Add the new DB column borrower_attribute_types.mandatory Signed-off-by: David Nind <david@davidnind.com> Created attachment 101201 [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> Created attachment 101202 [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> Created attachment 101203 [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> Created attachment 101204 [details] [review] Bug 22844: DBIC Schema changes Created attachment 101205 [details] [review] Bug 22844: Add is_boolean flag for new 'mandatory' column Created attachment 101302 [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> Created attachment 101480 [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> 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> 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> 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> 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> 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> 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> Created attachment 101487 [details] [review] Bug 22844: DBIC Schema changes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 101488 [details] [review] Bug 22844: Add is_boolean flag for new 'mandatory' column Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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? I will! 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> 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> 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> 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> 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> 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> Created attachment 101623 [details] [review] Bug 22844: DBIC Schema changes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 101624 [details] [review] Bug 22844: Add is_boolean flag for new 'mandatory' column Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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. 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) 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> 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> 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> 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> 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> 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> Created attachment 103848 [details] [review] Bug 22844: DBIC Schema changes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 103849 [details] [review] Bug 22844: Add is_boolean flag for new 'mandatory' column Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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) Could this whole patch set be retested? The last patch should fix the OPAC behaviours (self-reg and modification) 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 (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. 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! 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. Created attachment 103904 [details] [review] Bug 22844: Remove 'separate columns with |' from prefs description (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. 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> 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> 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> 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> 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> 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> 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> 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> 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> Created attachment 103969 [details] [review] Bug 22844: Remove 'separate columns with |' from prefs description Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> 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
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 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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. *** Bug 4030 has been marked as a duplicate of this bug. *** Katrin, can you confirm you have tested mandatory patron's attributes? See comment 93. (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? I listed some ideas in comment 80. Pushed to master for 20.11, thanks to everybody involved! 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. Follow-up pushed to master for 20.11 Not backporting this enhancement to 20.05.x series. |
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