Bug 22943

Summary: The in_ipset C4::Auth function name can be confusing
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Architecture, internals, and plumbingAssignee: Hayley Pelham <hayleypelham>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: 1joynelson, dcook, gmcharlt, hayleypelham, katrin.fischer, lucas, m.de.rooy, martin.renvoize, mtompset
Version: unspecifiedKeywords: Academy
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.05, 19.05.10
Bug Depends on: 14407    
Bug Blocks:    
Attachments: Bug 22943: Renames the in_ipset function to in_iprange in C4::Auth
Bug 22943: Renames the in_ipset function to in_iprange in C4::Auth
Bug 22943: (QA follow-up) Add new cases
Bug 22943: Renames the in_ipset function to in_iprange in C4::Auth
Bug 22943: (QA follow-up) Add new cases
Bug 22943: Remove unused line from Auth.t

Description Martin Renvoize 2019-05-21 06:12:18 UTC
In bug 14407 we introduced a new system preference to allow limiting the online self checkout system to an IP or IP Range. The preference introduced, however, is called in_ipset which can be confusing (as for those in the know there is a tool in linux called ipset which isn't related to the original bug).

We should therefore rename the system preference to in_iprange to more accurately describe it's function.
Comment 1 David Cook 2019-05-22 06:10:50 UTC
I think it's a function name rather than a system preference, but otherwise I totally agree. I think it is confusingly named.
Comment 2 Hayley Pelham 2019-05-27 02:03:10 UTC
Created attachment 90100 [details] [review]
Bug 22943: Renames the in_ipset function to in_iprange in C4::Auth

Bug 14407 introduced a new system preference to allow limiting the
online self checkout system to an IP or IP Range. The function that
handles this is called in_ipset, which is the name of a linux tool. To
stop confusion, this patch renames the function to 'in_iprange', and the
variable 'ipset' within it to 'iprange'.

To test, follow the test plans outlined in Bug 14407 and confirm that
everything works as expected.

Sponsored-by: Catalyst IT
Comment 3 Mark Tompsett 2019-06-21 03:50:17 UTC
I am wondering if the reason the name in_ipset was chosen was because multiple, disjoined IP ranges are allowed, not just a single one?
Comment 4 David Cook 2019-06-21 09:22:43 UTC
(In reply to M. Tompsett from comment #3)
> I am wondering if the reason the name in_ipset was chosen was because
> multiple, disjoined IP ranges are allowed, not just a single one?

This probably actually say "in_ipranges" that's true. If you look at the code in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14407, you'll see lots of mentions of ranges instead of set.
Comment 5 Hayley Pelham 2020-01-24 00:18:44 UTC
Thanks for your comments Mark and David. Patch still needs signoff
Comment 6 Martin Renvoize 2020-03-09 09:20:08 UTC
Created attachment 100323 [details] [review]
Bug 22943: Renames the in_ipset function to in_iprange in C4::Auth

Bug 14407 introduced a new system preference to allow limiting the
online self checkout system to an IP or IP Range. The function that
handles this is called in_ipset, which is the name of a linux tool. To
stop confusion, this patch renames the function to 'in_iprange', and the
variable 'ipset' within it to 'iprange'.

To test, follow the test plans outlined in Bug 14407 and confirm that
everything works as expected.

Sponsored-by: Catalyst IT
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2020-03-09 09:20:11 UTC
Created attachment 100324 [details] [review]
Bug 22943: (QA follow-up) Add new cases

A few additional cases were introduced since the original bug was
authored. This patch simply catches those cases and makes the same
change.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2020-03-09 09:20:40 UTC
My apologies, I'd missed that you had submitted a patch here Hayley... Signing off :)
Comment 9 Katrin Fischer 2020-03-15 14:55:48 UTC
Created attachment 100702 [details] [review]
Bug 22943: Renames the in_ipset function to in_iprange in C4::Auth

Bug 14407 introduced a new system preference to allow limiting the
online self checkout system to an IP or IP Range. The function that
handles this is called in_ipset, which is the name of a linux tool. To
stop confusion, this patch renames the function to 'in_iprange', and the
variable 'ipset' within it to 'iprange'.

To test, follow the test plans outlined in Bug 14407 and confirm that
everything works as expected.

Sponsored-by: Catalyst IT
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Katrin Fischer 2020-03-15 14:55:52 UTC
Created attachment 100703 [details] [review]
Bug 22943: (QA follow-up) Add new cases

A few additional cases were introduced since the original bug was
authored. This patch simply catches those cases and makes the same
change.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Katrin Fischer 2020-03-15 14:55:56 UTC
Created attachment 100704 [details] [review]
Bug 22943: Remove unused line from Auth.t

Removes an unnecessary variable declaration from Auth.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Martin Renvoize 2020-03-16 11:03:07 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 13 Joy Nelson 2020-04-03 17:36:34 UTC
Backported to 19.11.x branch for 19.11.05
Comment 14 Lucas Gass 2020-04-10 16:28:43 UTC
when applied to 19.05.x QA tool yells at me:

 FAIL	opac/sco/printslip.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   FAIL	  valid
		"in_ipset" is not exported by the C4::Auth module
		Can't continue after import errors
Comment 15 Lucas Gass 2020-04-10 16:40:57 UTC
Ignore my last comment, I failed to add the QA follow-up.

Backported to 19.05.x for 19.05.10
Comment 16 Hayley Pelham 2020-04-14 23:54:14 UTC
Dependency not in 18.11.x, not backporting to 18.11.x series