On the footsteps of Bug 5634. "Some libraries use lower case & upper case in branch description. The list is ordered case sensitively, it's hard to read." Use C4::Branch::GetBranchesLoop() to get an ordered list of branches.
Created attachment 14787 [details] [review] Bug 9457: [ENH] Ordering branches should be case independent (2) This patch replace use of CGI::scroll_list() to show list of branches. In two files, marc21_linking_section.pl and unimarc_field_4XX.pl, the scrolling list is created but not used in the template file, so the code is removed. Also minor renaming/normalizing of variables. To test: 1) Install with some branches, records and patrons 2.1) Select a record, click 'Place hold', select user, there is a library pull-down next to 'Pickup at:', list is ordered case sensitive 2.2) Go to Reports > Average loan time, next to Library is a pull-down, list without order 2.3) Go to Reports > Catalog by item type, next to 'Select a library' is a pull-down, list is ordered case sensitive 2.4) This is tricky, go to Reports home, change last part of URL 'reports-home.pl' with 'manager.pl?report_name=issues_by_borrower_category' (can't find a direct link), next to 'Select a library' is a library pull-down, list without order 2.5) Edit/Add a patron, on section 'Library management' there is a library pull-down, case sensitive 2.6) OPAC, as logged user, make a suggestion or hold, there is library pull-down, correct order 3) Apply the patch 4.1) Repeat 2.1), correctly ordered list 4.2) Repeat 2.2), correctly ordered list 4.3) Repeat 2.3), correctly ordered list 4.4) Repeat 2.4), correctly ordered list 4.5) This is a bit more work There are 3 possible situations to test: A) No branches, must show a message that are no libraries defined B) New patron, must show a correctly ordered list of branches, current branch selected C) Edit patron, must show a correctly ordered list of branches, patron branch selected 4.6) Small changes on variable names, so retest 2.6)
Created attachment 15184 [details] [review] Bug 9457: [ENH] Ordering branches should be case independent (2) This patch replace use of CGI::scroll_list() to show list of branches. In two files, marc21_linking_section.pl and unimarc_field_4XX.pl, the scrolling list is created but not used in the template file, so the code is removed. Also minor renaming/normalizing of variables. To test: 1) Install with some branches, records and patrons 2.1) Select a record, click 'Place hold', select user, there is a library pull-down next to 'Pickup at:', list is ordered case sensitive 2.2) Go to Reports > Average loan time, next to Library is a pull-down, list without order 2.3) Go to Reports > Catalog by item type, next to 'Select a library' is a pull-down, list is ordered case sensitive 2.4) This is tricky, go to Reports home, change last part of URL 'reports-home.pl' with 'manager.pl?report_name=issues_by_borrower_category' (can't find a direct link), next to 'Select a library' is a library pull-down, list without order 2.5) Edit/Add a patron, on section 'Library management' there is a library pull-down, case sensitive 2.6) OPAC, as logged user, make a suggestion or hold, there is library pull-down, correct order 3) Apply the patch 4.1) Repeat 2.1), correctly ordered list 4.2) Repeat 2.2), correctly ordered list 4.3) Repeat 2.3), correctly ordered list 4.4) Repeat 2.4), correctly ordered list 4.5) This is a bit more work There are 3 possible situations to test: A) No branches, must show a message that are no libraries defined B) New patron, must show a correctly ordered list of branches, current branch selected C) Edit patron, must show a correctly ordered list of branches, patron branch selected 4.6) Small changes on variable names, so retest 2.6) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Bernardo++ for getting rid of CGI::scroll_list's
QA comment: Great patch Bernardo! And great test plan too, it is really pleasant to test. I just found a little (big?) stuff. You forget to test (at several places) if C4::Context->userenv exists before to access to the 'branch' key. Sometimes this hashref (C4::Context->userenv) is not defined and the user get a software error. I think there is just one case: if the insecure syspref is turn ON. *But* I suppose this feature is broken, so are there actually someone using it? (I suppose that because of there are already several places where this test does not exist (e.g. reserve/request.pl)). So there are 2 possibilities: 1/ You send a mail to koha-devel in order to know if this feature can be deleted (in this case, we open a new report for that). 2/ You submit a followup taking account of this syspref. Switch the status to in discussion.
(In reply to comment #4) > I just found a little (big?) stuff. You forget to test (at several places) > if C4::Context->userenv exists before to access to the 'branch' key. > Sometimes this hashref (C4::Context->userenv) is not defined and the user > get a software error. I think there is just one case: if the insecure > syspref is turn ON. Good point. I was not aware of this syspref, and do not check that there is a possibility that userenv could return undef. > *But* I suppose this feature is broken, so are there actually someone using > it? > (I suppose that because of there are already several places where this test > does not exist (e.g. reserve/request.pl)). It is broken. After turned on, could not display staff login page. Must revert value on database. > So there are 2 possibilities: > 1/ You send a mail to koha-devel in order to know if this feature can be > deleted (in this case, we open a new report for that). > 2/ You submit a followup taking account of this syspref. > I think is better to do both, to disentangle the feature and this bug. I'll send a mail to koha-devel and add a followup, just in case :)
Created attachment 16171 [details] [review] Bug 9457 - Followup - Ordering branches should be case independent (2) This follow up reinstates or add check for undef returned by C4::Context->userenv, only where previous patch touch code.
I change this to Needs Signoff. Followup is small and only add 4 checks.
QA comment: Last patch fixes the issue, thanks Bernardo. I followed the test plan and did not found any problem. Code looks good. Marked as Passed QA.
Created attachment 16209 [details] [review] Bug 9457: [ENH] Ordering branches should be case independent (2) This patch replace use of CGI::scroll_list() to show list of branches. In two files, marc21_linking_section.pl and unimarc_field_4XX.pl, the scrolling list is created but not used in the template file, so the code is removed. Also minor renaming/normalizing of variables. To test: 1) Install with some branches, records and patrons 2.1) Select a record, click 'Place hold', select user, there is a library pull-down next to 'Pickup at:', list is ordered case sensitive 2.2) Go to Reports > Average loan time, next to Library is a pull-down, list without order 2.3) Go to Reports > Catalog by item type, next to 'Select a library' is a pull-down, list is ordered case sensitive 2.4) This is tricky, go to Reports home, change last part of URL 'reports-home.pl' with 'manager.pl?report_name=issues_by_borrower_category' (can't find a direct link), next to 'Select a library' is a library pull-down, list without order 2.5) Edit/Add a patron, on section 'Library management' there is a library pull-down, case sensitive 2.6) OPAC, as logged user, make a suggestion or hold, there is library pull-down, correct order 3) Apply the patch 4.1) Repeat 2.1), correctly ordered list 4.2) Repeat 2.2), correctly ordered list 4.3) Repeat 2.3), correctly ordered list 4.4) Repeat 2.4), correctly ordered list 4.5) This is a bit more work There are 3 possible situations to test: A) No branches, must show a message that are no libraries defined B) New patron, must show a correctly ordered list of branches, current branch selected C) Edit patron, must show a correctly ordered list of branches, patron branch selected 4.6) Small changes on variable names, so retest 2.6) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 16210 [details] [review] Bug 9457 - Followup - Ordering branches should be case independent (2) This follow up reinstates or add check for undef returned by C4::Context->userenv, only where previous patch touch code. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
This patch has been pushed to master.