Bug 9457 - Ordering branches should be case independent (2)
Summary: Ordering branches should be case independent (2)
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bernardo Gonzalez Kriegel
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-23 13:37 UTC by Bernardo Gonzalez Kriegel
Modified: 2014-05-26 21:04 UTC (History)
2 users (show)

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


Attachments
Bug 9457: [ENH] Ordering branches should be case independent (2) (29.24 KB, patch)
2013-01-23 15:13 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 9457: [ENH] Ordering branches should be case independent (2) (29.33 KB, patch)
2013-02-08 17:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9457 - Followup - Ordering branches should be case independent (2) (3.27 KB, patch)
2013-03-15 20:36 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 9457: [ENH] Ordering branches should be case independent (2) (29.33 KB, patch)
2013-03-18 09:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9457 - Followup - Ordering branches should be case independent (2) (3.33 KB, patch)
2013-03-18 09:13 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Bernardo Gonzalez Kriegel 2013-01-23 13:37:17 UTC
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.
Comment 1 Bernardo Gonzalez Kriegel 2013-01-23 15:13:03 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-02-08 17:40:39 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2013-02-08 17:41:23 UTC
Bernardo++ for getting rid of CGI::scroll_list's
Comment 4 Jonathan Druart 2013-03-15 15:17:08 UTC
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.
Comment 5 Bernardo Gonzalez Kriegel 2013-03-15 19:46:58 UTC
(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 :)
Comment 6 Bernardo Gonzalez Kriegel 2013-03-15 20:36:32 UTC Comment hidden (obsolete)
Comment 7 Bernardo Gonzalez Kriegel 2013-03-15 20:41:54 UTC
I change this to Needs Signoff. Followup is small and only add 4 checks.
Comment 8 Jonathan Druart 2013-03-18 09:06:42 UTC
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.
Comment 9 Jonathan Druart 2013-03-18 09:07:38 UTC
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>
Comment 10 Jonathan Druart 2013-03-18 09:13:11 UTC
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>
Comment 11 Jared Camins-Esakov 2013-03-20 19:37:02 UTC
This patch has been pushed to master.