Attachment 167086 Details for Bug 36908 – Bug 36908: Sort branches based on branchcode

[patch] Bug 36908: Sort branches based on branchcode

Bug-36908-Sort-branches-based-on-branchcode.patch (text/plain), 8.38 KB, created by Martin Renvoize (ashimema) on 2024-05-23 09:39:00 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2024-05-23 09:39:00 UTC

Size: 8.38 KB

patch

obsolete

>From a9759138a763b580940317db7671c30fa2a718c1 Mon Sep 17 00:00:00 2001
>From: Nick Clemens <nick@bywatersolutions.com>
>Date: Tue, 21 May 2024 12:44:25 +0000
>Subject: [PATCH] Bug 36908: Sort branches based on branchcode
>
>This adds a sort based on branchcode, it's a fallback for an edge case that should be rare
>so I think is acceptable, as long as documented.
>
>I added test coverage, but it may no longer be possible to encounter this scenario.
>
>System preference descriptions are updated as well.
>
>Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>---
> C4/Auth.pm                                    |  5 ++-
> .../en/modules/admin/preferences/admin.pref   |  6 +--
> t/db_dependent/Auth.t                         | 44 +++++++++++++++----
> 3 files changed, 41 insertions(+), 14 deletions(-)
>
>diff --git a/C4/Auth.pm b/C4/Auth.pm
>index f9484add7ce..a1019737c08 100644
>--- a/C4/Auth.pm
>+++ b/C4/Auth.pm
>@@ -1214,8 +1214,8 @@ sub checkauth {
>                         $register_id   = $register->id   if ($register);
>                         $register_name = $register->name if ($register);
>                     }
>-                    my $branches = { map { $_->branchcode => $_->unblessed } Koha::Libraries->search->as_list };
>                     if ( $type ne 'opac' ) {
>+                        my $branches = { map { $_->branchcode => $_->unblessed } Koha::Libraries->search->as_list };
>                         if ( C4::Context->preference('AutoLocation') ) {
>                             # we have to check they are coming from the right ip range
>                             my $domain = $branches->{$branchcode}->{'branchip'};
>@@ -1248,7 +1248,8 @@ sub checkauth {
>                                 && $branches->{$branchcode}->{'branchip'} )
>                             )
>                         {
>-                            foreach my $br ( uniq( $branchcode, keys %$branches ) ) {
>+                            my @branchcodes = sort { lc $a cmp lc $b } keys %$branches;
>+                            foreach my $br ( uniq( $branchcode, @branchcodes ) ) {
> 
>                                 #     now we work with the treatment of ip
>                                 my $domain = $branches->{$br}->{'branchip'};
>diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref
>index 7b226ea8762..efba78026b7 100644
>--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref
>+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref
>@@ -91,14 +91,14 @@ Administration:
>               class: integer
>             - Adding d will specify it in days, e.g. 1d is timeout of one day.
>         -
>-            - "Require staff to log in from a computer in the IP address range specified by their library (if any): "
>+            - "Require staff to log in from a computer in the IP address range specified by their library or the branch chosen at login (if any): "
>             - pref: AutoLocation
>               default: 0
>               choices:
>                   1: "Yes"
>                   0: "No"
>             - <a href="/cgi-bin/koha/admin/branches.pl">Link to library administration</a>
>-            - 'This setting will override the <a href="/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=StaffLoginBranchBasedOnIP">StaffLoginBranchBasedOnIP</a> system preference.'
>+            - 'Staff can only choose thier branch on login if they have "loggedinlibrary" permission. This setting will override the <a href="/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=StaffLoginBranchBasedOnIP">StaffLoginBranchBasedOnIP</a> system preference. In the event of multiple branches with matching IPs, branchcode (alphabetically) will be the tie breaker.'
>         -
>             - "Enable check for change in remote IP address for session security: "
>             - pref: SessionRestrictionByIP
>@@ -115,7 +115,7 @@ Administration:
>                   1: "Yes"
>                   0: "No"
>             - "Note: If IPs overlap, the first found match will be used."
>-            - 'This setting will be overridden by <a href="/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=AutoLocation">AutoLocation</a> system preference.'
>+            - 'This setting will be overridden by <a href="/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=AutoLocation">AutoLocation</a> system preference. In the event of multiple branches with matching IPs, branchcode (alphabetically) will be the tie breaker.'
>         # PostgreSQL is supported by CGI::Session but not by Koha.
>         -
>             - "Storage of login session information: "
>diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t
>index a721da0a8ba..bf092f87272 100755
>--- a/t/db_dependent/Auth.t
>+++ b/t/db_dependent/Auth.t
>@@ -1341,17 +1341,25 @@ subtest 'StaffLoginBranchBasedOnIP' => sub {
>     );
> 
>     t::lib::Mocks::mock_preference( 'AutoLocation', 0 );
>-    my $other_branch   = $builder->build_object( { class => 'Koha::Libraries', value => { branchip => "127.0.0.1" } } );
>+    my $other_branch = $builder->build_object(
>+        {
>+            class => 'Koha::Libraries',
>+            value => { branchip => "127.0.0.1", branchcode => substr( "z" . $branch->branchcode, 0, 10 ) }
>+        }
>+    );
>     ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth( $cgi, 0, { catalogue => 1 }, 'intranet' );
>     is( $userid, $patron->userid, "User successfully logged in" );
>     $session = C4::Auth::get_session($sessionID);
>-    is( $session->param('branch'), $branch->branchcode, "Logged in branch is set based which branch when two libraries have same IP?" );
>+    is(
>+        $session->param('branch'), $branch->branchcode,
>+        "Logged in branch is set based which branch when two libraries have same IP?"
>+    );
> 
> };
> 
> subtest 'AutoLocation' => sub {
> 
>-    plan tests => 11;
>+    plan tests => 12;
> 
>     $schema->storage->txn_begin;
> 
>@@ -1428,19 +1436,37 @@ subtest 'AutoLocation' => sub {
>     ( $userid, $cookie, $sessionID, $flags, $template ) =
>         C4::Auth::checkauth( $cgi, 0, { catalogue => 1 }, 'intranet' );
>     $session = C4::Auth::get_session($sessionID);
>-    is( $session->param('branch'), $noip_library->branchcode, "When a branch with no IP set is chosen, we respect the choice regardless of current IP" );
>+    is(
>+        $session->param('branch'), $noip_library->branchcode,
>+        "When a branch with no IP set is chosen, we respect the choice regardless of current IP"
>+    );
> 
>     $ENV{REMOTE_ADDR} = '129.0.0.1';          # Set current IP to match other_branch
>-    $cgi->param( 'branch', undef );           # Do not pass a branch
>+    $cgi->param( 'branch', '' );              # Do not pass a branch
>     $patron->library->branchip('')->store;    # Unset user branch IP, to allow IP matching on any branch
>-    # Add a second branch with same IP
>-    my $another_library = $builder->build_object( { class => 'Koha::Libraries', value => { branchip => '129.0.0.1' } } );
>+                                              # Add a second branch with same IP
>+    my $another_library = $builder->build_object(
>+        {
>+            class => 'Koha::Libraries',
>+            value => { branchip => "129.0.0.1", branchcode => substr( "z" . $other_library->branchcode, 0, 10 ) }
>+        }
>+    );
>     ( $userid, $cookie, $sessionID, $flags, $template ) =
>         C4::Auth::checkauth( $cgi, 0, { catalogue => 1 }, 'intranet', undef, undef, { do_not_print => 1 } );
>     $session = C4::Auth::get_session($sessionID);
>     is(
>-        $session->param('branch'), $other_library->branchcode,
>-        "Which branch is chosen when home branch has no IP and more than 1 library matches?"
>+        $session->param('branch'), $patron->library->branchcode,
>+        "When user branch has no IP, and no branch chosen, user is logged in to their homebranch"
>+    );
>+
>+    $cgi->param( 'branch', $another_library->branchcode )
>+        ;    # Choose branch with duplicate IP and alphabetically later branchcode
>+    ( $userid, $cookie, $sessionID, $flags, $template ) =
>+        C4::Auth::checkauth( $cgi, 0, { catalogue => 1 }, 'intranet', undef, undef, { do_not_print => 1 } );
>+    $session = C4::Auth::get_session($sessionID);
>+    is(
>+        $session->param('branch'), $another_library->branchcode,
>+        "When there is an IP conflict, we use the chosen branch if it matches"
>     );
> 
>     $schema->storage->txn_rollback;
>-- 
>2.45.1

Actions: View | Diff | Splinter Review

Attachments on bug 36908: 166966 | 166967 | 166968 | 167044 | 167045 | 167046 | 167073 | 167074 | 167075 | 167084 | 167085 | 167086 | 167087 | 167090