Bug 22943 - The in_ipset C4::Auth function name can be confusing
Summary: The in_ipset C4::Auth function name can be confusing
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low minor (vote)
Assignee: Hayley Pelham
QA Contact: Testopia
URL:
Keywords: Academy
Depends on: 14407
Blocks:
  Show dependency treegraph
 
Reported: 2019-05-21 06:12 UTC by Martin Renvoize
Modified: 2020-11-30 21:46 UTC (History)
9 users (show)

See Also:
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


Attachments
Bug 22943: Renames the in_ipset function to in_iprange in C4::Auth (7.31 KB, patch)
2019-05-27 02:03 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 22943: Renames the in_ipset function to in_iprange in C4::Auth (7.36 KB, patch)
2020-03-09 09:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22943: (QA follow-up) Add new cases (1.53 KB, patch)
2020-03-09 09:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22943: Renames the in_ipset function to in_iprange in C4::Auth (7.42 KB, patch)
2020-03-15 14:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22943: (QA follow-up) Add new cases (1.59 KB, patch)
2020-03-15 14:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22943: Remove unused line from Auth.t (776 bytes, patch)
2020-03-15 14:55 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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