Summary: | The in_ipset C4::Auth function name can be confusing | ||
---|---|---|---|
Product: | Koha | Reporter: | Martin Renvoize (ashimema) <martin.renvoize> |
Component: | Architecture, internals, and plumbing | Assignee: | Hayley Pelham <hayleypelham> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | minor | ||
Priority: | P5 - low | CC: | 1joynelson, dcook, gmcharlt, hayleypelham, lucas, m.de.rooy, martin.renvoize, mtompset |
Version: | unspecified | Keywords: | Academy |
Hardware: | All | ||
OS: | All | ||
GIT URL: | 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
|
Circulation function: | |
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 (ashimema)
2019-05-21 06:12:18 UTC
I think it's a function name rather than a system preference, but otherwise I totally agree. I think it is confusingly named. 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 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? (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. Thanks for your comments Mark and David. Patch still needs signoff 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> 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> My apologies, I'd missed that you had submitted a patch here Hayley... Signing off :) 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> 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> 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> Nice work everyone! Pushed to master for 20.05 Backported to 19.11.x branch for 19.11.05 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 Ignore my last comment, I failed to add the QA follow-up. Backported to 19.05.x for 19.05.10 Dependency not in 18.11.x, not backporting to 18.11.x series |