Bug 36908 - Clarify and standardize the behavior of AutoLocation/ StaffLoginBranchBasedOnIP system preferences
Summary: Clarify and standardize the behavior of AutoLocation/ StaffLoginBranchBasedOn...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact: Martin Renvoize
URL:
Keywords: rel_24_05_candidate
Depends on: 36665
Blocks: 26176
  Show dependency treegraph
 
Reported: 2024-05-21 11:28 UTC by Nick Clemens (kidclamp)
Modified: 2024-05-31 15:28 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.06,23.05.12


Attachments
Bug 36908: Expand, clarify, and tidy tests for AutoLocation (3.88 KB, patch)
2024-05-21 12:54 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36908: Additional unit tests to identify flaw when two branches have same IP (3.28 KB, patch)
2024-05-21 12:54 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36908: Sort branches based on branchcode (6.54 KB, patch)
2024-05-21 12:54 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36908: Expand, clarify, and tidy tests for AutoLocation (4.44 KB, patch)
2024-05-22 14:21 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36908: Additional unit tests to identify flaw when two branches have same IP (3.26 KB, patch)
2024-05-22 14:21 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36908: Sort branches based on branchcode (8.24 KB, patch)
2024-05-22 14:21 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36908: Expand, clarify, and tidy tests for AutoLocation (4.51 KB, patch)
2024-05-23 07:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36908: Additional unit tests to identify flaw when two branches have same IP (3.34 KB, patch)
2024-05-23 07:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36908: Sort branches based on branchcode (8.32 KB, patch)
2024-05-23 07:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36908: Expand, clarify, and tidy tests for AutoLocation (4.57 KB, patch)
2024-05-23 09:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36908: Additional unit tests to identify flaw when two branches have same IP (3.40 KB, patch)
2024-05-23 09:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36908: Sort branches based on branchcode (8.38 KB, patch)
2024-05-23 09:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36908: (QA follow-up) Proposed improvement to prefernce description (2.64 KB, patch)
2024-05-23 09:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36908: (QA follow-up) Proposed improvement to prefernce description (2.69 KB, patch)
2024-05-23 09:55 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2024-05-21 11:28:58 UTC
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
Comment 1 Nick Clemens (kidclamp) 2024-05-21 12:54:03 UTC
Created attachment 166966 [details] [review]
Bug 36908: Expand, clarify, and tidy tests for AutoLocation
Comment 2 Nick Clemens (kidclamp) 2024-05-21 12:54:05 UTC
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 ));'
Comment 3 Nick Clemens (kidclamp) 2024-05-21 12:54:07 UTC
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
Comment 4 Jonathan Druart 2024-05-22 12:49:39 UTC Comment hidden (obsolete)
Comment 5 Nick Clemens (kidclamp) 2024-05-22 14:21:00 UTC
Created attachment 167044 [details] [review]
Bug 36908: Expand, clarify, and tidy tests for AutoLocation
Comment 6 Nick Clemens (kidclamp) 2024-05-22 14:21:02 UTC
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 ));'
Comment 7 Nick Clemens (kidclamp) 2024-05-22 14:21:04 UTC
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.
Comment 8 Jonathan Druart 2024-05-23 07:19:05 UTC
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>
Comment 9 Jonathan Druart 2024-05-23 07:19:08 UTC
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>
Comment 10 Jonathan Druart 2024-05-23 07:19:11 UTC
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>
Comment 11 Martin Renvoize 2024-05-23 09:38:55 UTC
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>
Comment 12 Martin Renvoize 2024-05-23 09:38:57 UTC
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>
Comment 13 Martin Renvoize 2024-05-23 09:39:00 UTC
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>
Comment 14 Martin Renvoize 2024-05-23 09:39:03 UTC
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>
Comment 15 Martin Renvoize 2024-05-23 09:41:08 UTC
I'm happy to PQA this.. but I think we would like Nicks final +1 on the proposed follow-up.
Comment 16 Nick Clemens (kidclamp) 2024-05-23 09:55:06 UTC
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>
Comment 17 Nick Clemens (kidclamp) 2024-05-23 09:55:35 UTC
(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
Comment 18 Katrin Fischer 2024-05-23 15:57:01 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 19 Fridolin Somers 2024-05-29 15:04:07 UTC
Pushed to 23.11.x for 23.11.06
Comment 20 Lucas Gass 2024-05-31 15:28:56 UTC
Backported to 23.05.x for upcoming 23.05.12