Bug 26176 - AutoLocation is badly named
Summary: AutoLocation is badly named
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal with 10 votes (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact: Testopia
URL:
Keywords: rel_24_05_candidate
Depends on: 36908
Blocks: 36933 36934
  Show dependency treegraph
 
Reported: 2020-08-07 11:41 UTC by Martin Renvoize
Modified: 2024-05-24 13:40 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


Attachments
Bug 26176: Rename AutoLocation to StaffLoginRestrictBranchByIP (14.33 KB, patch)
2024-05-21 13:59 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 26176: (follow-up) Prevent random failures from too long branchcode (1.65 KB, patch)
2024-05-21 13:59 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 26176: Rename AutoLocation to StaffLoginRestrictBranchByIP (15.28 KB, patch)
2024-05-22 11:06 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 26176: (follow-up) Prevent random failures from too long branchcode (1.65 KB, patch)
2024-05-22 11:06 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 26176: Rename AutoLocation to StaffLoginRestrictBranchByIP (15.05 KB, patch)
2024-05-23 07:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26176: Rename AutoLocation to StaffLoginRestrictBranchByIP (15.15 KB, patch)
2024-05-23 10:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26176: (QA follow-up) Branch -> Library (14.32 KB, patch)
2024-05-23 10:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26176: (QA follow-up) Also rename StaffLoginBranchBasedOnIP (14.74 KB, patch)
2024-05-23 10:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26176: (follow-up) Fix one more occurrence (1.56 KB, patch)
2024-05-23 10:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 26176: (follow-up) Fix one more occurrence (1.00 KB, patch)
2024-05-23 10:58 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 26176: (follow-up) Fix one more occurrence (1.00 KB, patch)
2024-05-23 10:59 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 26176: (follow-up) Use suggested patturn in update (1.46 KB, patch)
2024-05-23 14:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26176: (follow-up) Replace branch with library in syspref description (1.60 KB, patch)
2024-05-24 11:31 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 Martin Renvoize 2020-08-07 11:41:17 UTC
As far as I can tell AutoLocation isn't about koha picking a library branch automatically from your login IP address as much as it's a security feature to prevent you from logging into another branch from outside an IP range.

As such, I feel the preference is either misnamed, or the description is incorrect.. either way it needs some clarification adding.
Comment 1 Katrin Fischer 2023-09-22 19:17:53 UTC
Do you have an idea for a good name?

RestrictStaffAccessByIP?
Comment 2 Martin Renvoize 2023-09-22 21:59:31 UTC
Adding some Cc's for opinions
Comment 3 Caroline Cyr La Rose 2023-09-25 13:35:10 UTC
I like RestrictStaffAccessByIP

The other ones I thought of are basically variations of the same name

- RestrictAccessByIP

- AccessRestrictionByIP (very similar to/based on SessionRestrictionByIP, maybe too similar?)

- StaffAllowByIP (based on SelfCheckAllowByIPRanges)

- ControlAccessByIP

- IPStaffAccessControl

- IPStaffAccessRestriction

If you change it, can you also change the sentence to 

"[Don't require|Require] staff to log in from a computer in the IP address range specified by their library (if any)"

(instead of .... [No|Yes]) ?
Comment 4 Aude Charillon 2023-09-29 07:54:39 UTC
I also like RestrictStaffAccessByIP - does what it says! Or, if we want to go for something a little bit shorter: ControlAccessByIP from among Caroline's suggestions.
Comment 5 Fridolin Somers 2024-04-22 09:43:37 UTC
Maybe "StaffRestrictAccessByIP" in case we add "OPACRestrictAccessByIP"
Comment 6 Nick Clemens (kidclamp) 2024-05-21 13:59:10 UTC
Created attachment 166975 [details] [review]
Bug 26176: Rename AutoLocation to StaffLoginRestrictBranchByIP

This patch sets AutoLocation to be called StaffLoginRestrictBranchByIP.
The new name is chosen to reflect the new pref StaffLoginBranchBasedOnIP.

Also this patch corrects the order of sysprefs in installer file.

To test:
Follow test plans on bug 36665 and bug 35890 and confirm that the preferences
continue to work as expected
Confirm the descriptions of the prefs in the staff interface match the behaviors expected
Comment 7 Nick Clemens (kidclamp) 2024-05-21 13:59:13 UTC
Created attachment 166976 [details] [review]
Bug 26176: (follow-up) Prevent random failures from too long branchcode
Comment 8 Nick Clemens (kidclamp) 2024-05-22 11:06:26 UTC
Created attachment 167019 [details] [review]
Bug 26176: Rename AutoLocation to StaffLoginRestrictBranchByIP

This patch sets AutoLocation to be called StaffLoginRestrictBranchByIP.
The new name is chosen to reflect the new pref StaffLoginBranchBasedOnIP.

Also this patch corrects the order of sysprefs in installer file.

To test:
Follow test plans on bug 36665 and bug 35890 and confirm that the preferences
continue to work as expected
Confirm the descriptions of the prefs in the staff interface match the behaviors expected
Comment 9 Nick Clemens (kidclamp) 2024-05-22 11:06:29 UTC
Created attachment 167020 [details] [review]
Bug 26176: (follow-up) Prevent random failures from too long branchcode
Comment 10 Jonathan Druart 2024-05-23 07:18:14 UTC
Created attachment 167072 [details] [review]
Bug 26176: Rename AutoLocation to StaffLoginRestrictBranchByIP

This patch sets AutoLocation to be called StaffLoginRestrictBranchByIP.
The new name is chosen to reflect the new pref StaffLoginBranchBasedOnIP.

Also this patch corrects the order of sysprefs in installer file.

To test:
Follow test plans on bug 36665 and bug 35890 and confirm that the preferences
continue to work as expected
Confirm the descriptions of the prefs in the staff interface match the behaviors expected

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Jonathan Druart 2024-05-23 07:18:41 UTC
I had trouble with the rebase (had to apply --reject). Second patch seems not needed, the changes are already in "Bug 36908: Sort branches based on branchcode"
Comment 12 Nick Clemens (kidclamp) 2024-05-23 09:58:16 UTC
(In reply to Jonathan Druart from comment #11)
> I had trouble with the rebase (had to apply --reject). Second patch seems
> not needed, the changes are already in "Bug 36908: Sort branches based on
> branchcode"

Yeah, I picked that patch back on the other bug when rebasing as that's when the problem was introduced
Comment 13 Martin Renvoize 2024-05-23 10:07:31 UTC
Created attachment 167091 [details] [review]
Bug 26176: Rename AutoLocation to StaffLoginRestrictBranchByIP

This patch sets AutoLocation to be called StaffLoginRestrictBranchByIP.
The new name is chosen to reflect the new pref StaffLoginBranchBasedOnIP.

Also this patch corrects the order of sysprefs in installer file.

To test:
Follow test plans on bug 36665 and bug 35890 and confirm that the preferences
continue to work as expected
Confirm the descriptions of the prefs in the staff interface match the behaviors expected

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 10:07:34 UTC
Created attachment 167092 [details] [review]
Bug 26176: (QA follow-up) Branch -> Library

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2024-05-23 10:07:37 UTC
Created attachment 167093 [details] [review]
Bug 26176: (QA follow-up) Also rename StaffLoginBranchBasedOnIP

This patch also renames StaffLoginBranchBasedOnIP to
StaffLoginLibraryBasedOnIP to be consistent with our terminology
guidelines

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Jonathan Druart 2024-05-23 10:13:26 UTC
(In reply to Martin Renvoize from comment #15)
> Created attachment 167093 [details] [review] [review]
> Bug 26176: (QA follow-up) Also rename StaffLoginBranchBasedOnIP
> 
> This patch also renames StaffLoginBranchBasedOnIP to
> StaffLoginLibraryBasedOnIP to be consistent with our terminology
> guidelines
> 
> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

I would suggest to use this pattern:

$dbh->do(q{ UPDATE }) == 1 && say_success(q{Updated!});
Comment 17 Nick Clemens (kidclamp) 2024-05-23 10:41:22 UTC
Created attachment 167094 [details] [review]
Bug 26176: (follow-up) Fix one more occurrence
Comment 18 Nick Clemens (kidclamp) 2024-05-23 10:58:14 UTC
Created attachment 167095 [details] [review]
Bug 26176: (follow-up) Fix one more occurrence
Comment 19 Nick Clemens (kidclamp) 2024-05-23 10:59:29 UTC
Created attachment 167096 [details] [review]
Bug 26176: (follow-up) Fix one more occurrence
Comment 20 Martin Renvoize 2024-05-23 14:34:43 UTC
Created attachment 167114 [details] [review]
Bug 26176: (follow-up) Use suggested patturn in update
Comment 21 Katrin Fischer 2024-05-23 15:56:50 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 22 Nick Clemens (kidclamp) 2024-05-24 11:31:06 UTC
Created attachment 167162 [details] [review]
Bug 26176: (follow-up) Replace branch with library in syspref description
Comment 23 Katrin Fischer 2024-05-24 13:40:10 UTC
Pushed for 24.05!

Well done everyone, thank you!