Special Characters in Branchcode cause the user to be unable to delete the branch. Replicate: 1. Administration->Libraries and Groups 2. Click on New Library button 3. Create a new branch use the & or # character in the branchcode 4. Branch is created. 5. Click on Delete (under Actions) for that branch 6. Verify you wish to delete the branch. 7. Error message appears An error occurred when deleting this library. Check the logs. Can this field be validated when data is entered? We should not allow users to create branchcodes that use special characters.
Created attachment 69062 [details] [review] Bug 19560: Unable to delete library when branchcode contains special characters This patch modifies the library administration page so that the library delete button submits a form using POST rather than linking to the delete operation with GET variables. This patch also adds validation to the library entry form so that it will only accept letters, numbers, hyphen, and underscore. The validation method for this already existed in categories.js, so it has been moved to staff-global.js and the validator warning message has been moved to validator-strings.inc so it is included globally. To test: - Add a library with a branchcode containing characters like & or #. - Apply the patch and clear your browser cache if necessary. - Try to delete the new library. It should delete successfully. - Add a library again and try to include spaces and/or special characters in the branchcode. The form validation script should prevent you from submitting the form. - Go do Administration -> Patron categories and add a new patron category. The category code field should prevent you from entering a code with spaces or special characters.
Created attachment 69097 [details] [review] Bug 19560: Unable to delete library when branchcode contains special characters This patch modifies the library administration page so that the library delete button submits a form using POST rather than linking to the delete operation with GET variables. This patch also adds validation to the library entry form so that it will only accept letters, numbers, hyphen, and underscore. The validation method for this already existed in categories.js, so it has been moved to staff-global.js and the validator warning message has been moved to validator-strings.inc so it is included globally. To test: - Add a library with a branchcode containing characters like & or #. - Apply the patch and clear your browser cache if necessary. - Try to delete the new library. It should delete successfully. - Add a library again and try to include spaces and/or special characters in the branchcode. The form validation script should prevent you from submitting the form. - Go do Administration -> Patron categories and add a new patron category. The category code field should prevent you from entering a code with spaces or special characters. Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
QA tool passed Works as described, but also prevents accentuated characters like é or à. For category codes I think this is acceptable/preferable.
Created attachment 69130 [details] [review] Bug 19560: Unable to delete library when branchcode contains special characters This patch modifies the library administration page so that the library delete button submits a form using POST rather than linking to the delete operation with GET variables. This patch also adds validation to the library entry form so that it will only accept letters, numbers, hyphen, and underscore. The validation method for this already existed in categories.js, so it has been moved to staff-global.js and the validator warning message has been moved to validator-strings.inc so it is included globally. To test: - Add a library with a branchcode containing characters like & or #. - Apply the patch and clear your browser cache if necessary. - Try to delete the new library. It should delete successfully. - Add a library again and try to include spaces and/or special characters in the branchcode. The form validation script should prevent you from submitting the form. - Go do Administration -> Patron categories and add a new patron category. The category code field should prevent you from entering a code with spaces or special characters. Signed-off-by: David Bourgault <david.bourgault@inlibro.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
I think this is a good start. We probably want these restrictions on authorised values too. We will also need a warning on the about page for pre-existing values
(In reply to Nick Clemens from comment #5) > I think this is a good start. We probably want these restrictions on > authorised values too. > > We will also need a warning on the about page for pre-existing values We didn't do this on authorised values because of things like MAN_INV - this has the display text in the field for the code :(
This patch does 2 things: 1. Only accept few chars for branchcode (accentuated chars are rejected) 2. Fix the deletion problem if a branchcode contains special characters It may be a problem for non latin alphabets to restrict as possiblities. Do we have known problems with branchcode containing special chars?
Jonathon, I encountered this because a library was adding a branch for "Art & Architecture" as A&A When they decided to go with a different naming convention, they were unable to delete the branch. joy (In reply to Jonathan Druart from comment #7) > This patch does 2 things: > 1. Only accept few chars for branchcode (accentuated chars are rejected) > 2. Fix the deletion problem if a branchcode contains special characters > > It may be a problem for non latin alphabets to restrict as possiblities. Do > we have known problems with branchcode containing special chars?
Created attachment 69423 [details] [review] Bug 19560: Correctly escape branchcode in admin/branches.pl
Joy, What I tried to say is: If we escape correctly the variable, we will not have problem to delete a library with branchcode=A&A, see the follow-up. I am so wondering if it is really useful to restrict the code to a-Z0-9_-
Ah! I see. Yes, escaping or restricting would work. I can't say I have a preference in the matter. Thanks! joy (In reply to Jonathan Druart from comment #10) > Joy, What I tried to say is: If we escape correctly the variable, we will > not have problem to delete a library with branchcode=A&A, see the follow-up. > I am so wondering if it is really useful to restrict the code to a-Z0-9_-
As far as I can tell, JavaScript regular expressions will not recognize non-ascii "word" characters as such, and thus client-side validation can't work here unless we restrict the options quite a bit. I appreciate that this limitation with my patch has bee pointed out because I don't think it's okay that we make it so strict if we don't have to. Is it a limitation of branches.pl that it fails to accept a string like this? /cgi-bin/koha/admin/branches.pl?op=add_form&branchcode=Ö×Ø&ÙÚ
Created attachment 69490 [details] [review] Bug 19560: Correctly escape branchcode in admin/branches.pl
(In reply to Owen Leonard from comment #12) > Is it a limitation of branches.pl that it fails to accept a string like this? > > /cgi-bin/koha/admin/branches.pl?op=add_form&branchcode=Ö×Ø&ÙÚ Fixed with the last patch.
IMO the escaping is better solution - we need it anyway for prevent XSS and it is enough to solve this issue - so no reason to restrict the letters in branchcode.
Should we still reject brachcodes with whitespace characters?
(In reply to Owen Leonard from comment #16) > Should we still reject brachcodes with whitespace characters? Yes I think so (for command line scripts)
(In reply to Jonathan Druart from comment #17) > (In reply to Owen Leonard from comment #16) > > Should we still reject brachcodes with whitespace characters? > > Yes I think so (for command line scripts) I agree
Created attachment 69700 [details] [review] Bug 19560: Correctly escape branchcode in admin/branches.pl Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Jonathan's patch is all that is needed. I've rebased it on master and signed off.
Created attachment 69701 [details] [review] Bug 19607: Move admin templates JavaScript to the footer: Basic parameters This patch modifies the staff client catalog-related administration templates so that JavaScript is included in the footer instead of the header. To test, apply the patch and test the JavaScript-driven features of each modified template: All button controls, DataTables functionality, tabs, etc. Signed-off-by: Simon Pouchol <simon.pouchol@biblibre.com> Edit: Rebased on Bug 19560
Comment on attachment 69701 [details] [review] Bug 19607: Move admin templates JavaScript to the footer: Basic parameters Sorry, attached in the wrong place!
Comment on attachment 69700 [details] [review] Bug 19560: Correctly escape branchcode in admin/branches.pl Review of attachment 69700 [details] [review]: ----------------------------------------------------------------- Just a quick look at the code: ::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/branches.tt @@ +5,4 @@ > [% ELSIF op == 'delete_confirm_category' %] > › Confirm deletion of group [% category.categorycode %] > [% ELSIF op == 'add_form' %] > + ›[% IF library %]Modify library[% ELSE %]New library [% library.branchcode | html %][% END %] Shouldn't it be? ›[% IF library %]Modify library [% library.branchcode | html %][% ELSE %]New library[% END %] looks like this is bug in current master... @@ +67,4 @@ > [% ELSIF op == 'delete_confirm_category' %] > › Confirm deletion of group [% category.categorycode %] > [% ELSIF op == 'add_form' %] > +› [% IF library %]Modify library[% ELSE %]New library [% library.branchcode | html %][% END %] The same
(In reply to Josef Moravec from comment #23) > looks like this is bug in current master... Yes, definitely
Created attachment 69735 [details] [review] Bug 19560: Correctly escape branchcode in admin/branches.pl Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 69736 [details] [review] Bug 19560: Correctly escape branchcode in admin/branches.pl Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 69775 [details] [review] Bug 19560: Correctly escape branchcode in admin/branches.pl Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Pushed to master for 18.05, thanks to everybody involved!
Awesome work all! Pushed to stable for 17.11.01
Owen, you modified the original patch to use a POST (form) instead of a GET (link), could you explain why? It makes the button hard to locate from selenium tests and hide the url to the user.
Pushed to 17.05.x for v17.05.09