Bug 7628 - Required format is not enforced for Patron Categories
Summary: Required format is not enforced for Patron Categories
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: 3.8
Hardware: All All
: P3 normal (vote)
Assignee: Marc Véron
QA Contact: Paul Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-01 12:10 UTC by Marc Véron
Modified: 2013-12-05 19:57 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7628 - Required format is not enforced for Patron Categories (2.05 KB, patch)
2012-07-18 06:00 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 7628 - Required format is not enforced for Patron Categories (2.10 KB, patch)
2012-07-18 07:08 UTC, David Cook
Details | Diff | Splinter Review
Bug 7628 - Required format is not enforced for Patron Categories (2.10 KB, patch)
2012-07-18 07:13 UTC, David Cook
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7628 - Required format is not enforced for Patron Categories (2.10 KB, patch)
2012-07-18 07:19 UTC, David Cook
Details | Diff | Splinter Review
Bug 7628: Escape characters for categorycode [alternative patch] (2.04 KB, patch)
2012-07-20 08:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7628: Escape '+' characters for categorycode (2.36 KB, patch)
2012-07-23 09:09 UTC, Marc Véron
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Véron 2012-03-01 12:10:48 UTC
Problem:
--------
If you use characters like '+' in category code, like '+TEST', you can not edit or delete the category. Required format is not enforced.

Solution:
---------
In categorie.tt, function Check(ff), trim user input and test it for unwanted characters.

Instead of:
-----------
if (ff.categorycode.value.length==0) {
   ok=1;
   _alertString += _("- categorycode missing") + "\n";
}

Put:
----

ff.categorycode.value = ff.categorycode.value.trim();
if (ff.categorycode.value.length==0) {
   ok=1;
   _alertString += _("- categorycode missing") + "\n";
}
else{
   var patt=/^[a-zA-Z0-9\-_]+$/g;	
   if ( !patt.test(ff.categorycode.value) ) {
      ok=1;
      _alertString += _("- category code can only contain the following characters: letters, numbers, - and _") + "\n";			
   }			
}
Comment 1 Marc Véron 2012-07-18 06:00:51 UTC Comment hidden (obsolete)
Comment 2 David Cook 2012-07-18 07:08:59 UTC Comment hidden (obsolete)
Comment 3 David Cook 2012-07-18 07:13:40 UTC Comment hidden (obsolete)
Comment 4 David Cook 2012-07-18 07:15:43 UTC
Small patch that does what it says. 

I'm not sure why someone would put a "+" into a category code, but data validation is always a good thing, and without it you stumble into the problems that Marc has outlined.
Comment 5 David Cook 2012-07-18 07:19:15 UTC
Created attachment 10923 [details] [review]
[SIGNED-OFF] Bug 7628 - Required format is not enforced for Patron Categories

Enforces category code with allowed characters only.

Test plan:

- Before applying patch, create a category code like Test+

- Try to delete category with this code -> does not work

- Apply patch

- Try to create category with code containg other characters than letters, numbers, - and _

- Result: You are not allowed to do so, appropriate message pops up

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 6 Jonathan Druart 2012-07-20 08:52:58 UTC Comment hidden (obsolete)
Comment 7 Marc Véron 2012-07-20 11:00:48 UTC
Jonathan,

When our librarians tested Koha for the first time they wanted to have some categories similar to "KIDS 4 +"

Then they wanted to delete the code and did not succeed. 

My patch would have prevented them to enter "illegal" characters (and to have a user experience like 'It does not work'). 

And if we do not allow to enter illegal characters there is no need to delete entries with illegal characters.

I think the main question is: What characters should be / are allowed with category codes? 

I had a look at the manual for 3.8:
http://manual.koha-community.org/3.8/en/patscirc.html#patcats
-Snip---------------
The 'Category Code' is an identifier for your new code.
    Important
    The category code is limited to 10 characters (numbers and letters)
-End snip---------------

Your patch assumes that &, @, /, ;, :, =, +, ? and $ are legal characters. Are they (without having side effects at other places in Koha)? Should all characters be allowed? What was (historically) the reason for the restriction in the manual? Why are the codes upper cased by the existing JavaScript?

If there are no side effects, your patch is the better solution, because it gives the useres more flexiblity to choose a 'speaking' code. (Manual should then be updated, though). If we have to expect side effects, I would prefer my restricion on data entry.

Marc
Comment 8 Jonathan Druart 2012-07-20 11:55:00 UTC
The solution may be a combination of these 2 patchs. It would allow restriction (from documention) to always be guaranted and existing wrong-formatted category code to be deleted.
Comment 9 Marc Véron 2012-07-20 13:03:02 UTC
Agree.
Comment 10 Marc Véron 2012-07-23 09:09:38 UTC
Created attachment 11072 [details] [review]
[SIGNED-OFF] Bug 7628: Escape '+' characters for categorycode

uri escape the following characters:
    &, @, /, ;, :, =, +, ? and $

Signed-off-by: Marc Veron <veron@veron.ch>

Test and sign off second patch (independently from first patch).

Added categories like '+zwoelf' or '? Test' before applying the patch.
Deletion was not possible.

After applying patch deletion is possible, patch works as expected.
Comment 11 Paul Poulain 2012-08-03 13:05:09 UTC
QA comment: small patch, test-qa.pl OK, passes QA
Comment 12 Paul Poulain 2012-08-03 13:07:00 UTC
follow-up comment: would be good to add the same control for itemtypes and branches, because I'm not sure at all having them with a special character will work...
Comment 13 Chris Cormack 2012-08-04 08:31:10 UTC
Pushed to 3.8.x, will be in 3.8.4