The existing AutoLocation system preference (to be renamed in bug 26176) and the new system preference StaffLoginBranchBasedOnIP both attempt to assign the correct branch for the user during login based on the current IP If multiple branches match the IP, or if the user specifies a branch during login the behavior can verify. We should ensure the behavior is consistent and clarify how it works in the syspref descriptions
Created attachment 166966 [details] [review] Bug 36908: Expand, clarify, and tidy tests for AutoLocation
Created attachment 166967 [details] [review] Bug 36908: Additional unit tests to identify flaw when two branches have same IP This could be considered a configuration flaw, but when: StaffLoginBranchBasedOnIP enabled and not AutoLocation or AutoLocation enabledand no IP set in user's branch AND two branches have the same IP set the user can be logged in randomly to one of the matching branches. These test often pass, but will also randomly fail Easier to verify with a one liner demonstrating current code: perl -e 'use Koha::Libraries; use List::MoreUtils qw(uniq); my $branches = { map { $_->branchcode => $_->unblessed } Koha::Libraries->search->as_list }; my $branchcode="CPL"; warn Data::Dumper::Dumper( uniq( $branchcode, keys %$branches ));'
Created attachment 166968 [details] [review] 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. System preference descriptions are updated as well
See bug 36665 comment 23. "AutoLocation" (login restriction by IP) is not working as the syspref's description is saying.
Created attachment 167044 [details] [review] Bug 36908: Expand, clarify, and tidy tests for AutoLocation
Created attachment 167045 [details] [review] Bug 36908: Additional unit tests to identify flaw when two branches have same IP This could be considered a configuration flaw, but when: StaffLoginBranchBasedOnIP enabled and not AutoLocation or AutoLocation enabledand no IP set in user's branch AND two branches have the same IP set the user can be logged in randomly to one of the matching branches. These test often pass, but will also randomly fail Easier to verify with a one liner demonstrating current code: perl -e 'use Koha::Libraries; use List::MoreUtils qw(uniq); my $branches = { map { $_->branchcode => $_->unblessed } Koha::Libraries->search->as_list }; my $branchcode="CPL"; warn Data::Dumper::Dumper( uniq( $branchcode, keys %$branches ));'
Created attachment 167046 [details] [review] 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.
Created attachment 167073 [details] [review] Bug 36908: Expand, clarify, and tidy tests for AutoLocation Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 167074 [details] [review] Bug 36908: Additional unit tests to identify flaw when two branches have same IP This could be considered a configuration flaw, but when: StaffLoginBranchBasedOnIP enabled and not AutoLocation or AutoLocation enabledand no IP set in user's branch AND two branches have the same IP set the user can be logged in randomly to one of the matching branches. These test often pass, but will also randomly fail Easier to verify with a one liner demonstrating current code: perl -e 'use Koha::Libraries; use List::MoreUtils qw(uniq); my $branches = { map { $_->branchcode => $_->unblessed } Koha::Libraries->search->as_list }; my $branchcode="CPL"; warn Data::Dumper::Dumper( uniq( $branchcode, keys %$branches ));' Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 167075 [details] [review] 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>
Created attachment 167084 [details] [review] Bug 36908: Expand, clarify, and tidy tests for AutoLocation Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 167085 [details] [review] Bug 36908: Additional unit tests to identify flaw when two branches have same IP This could be considered a configuration flaw, but when: StaffLoginBranchBasedOnIP enabled and not AutoLocation or AutoLocation enabledand no IP set in user's branch AND two branches have the same IP set the user can be logged in randomly to one of the matching branches. These test often pass, but will also randomly fail Easier to verify with a one liner demonstrating current code: perl -e 'use Koha::Libraries; use List::MoreUtils qw(uniq); my $branches = { map { $_->branchcode => $_->unblessed } Koha::Libraries->search->as_list }; my $branchcode="CPL"; warn Data::Dumper::Dumper( uniq( $branchcode, keys %$branches ));' Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 167086 [details] [review] 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>
Created attachment 167087 [details] [review] Bug 36908: (QA follow-up) Proposed improvement to prefernce description I found the initial addition of 'or the branch chosen at login' to the system preference description more misleading than the original. After discussion on mattermost and with training staff here at PTFS the best alternative we could come up with is proposed here. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I'm happy to PQA this.. but I think we would like Nicks final +1 on the proposed follow-up.
Created attachment 167090 [details] [review] Bug 36908: (QA follow-up) Proposed improvement to prefernce description I found the initial addition of 'or the branch chosen at login' to the system preference description more misleading than the original. After discussion on mattermost and with training staff here at PTFS the best alternative we could come up with is proposed here. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to Martin Renvoize from comment #15) > I'm happy to PQA this.. but I think we would like Nicks final +1 on the > proposed follow-up. Looks good to me - I corrected a typo in my SO patch
Pushed for 24.05! Well done everyone, thank you!
Pushed to 23.11.x for 23.11.06
Backported to 23.05.x for upcoming 23.05.12