Bug 32203 - Refactor checkauth() auth rejected to improve maintainability
Summary: Refactor checkauth() auth rejected to improve maintainability
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-11-14 23:51 UTC by David Cook
Modified: 2023-09-20 02:42 UTC (History)
1 user (show)

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


Attachments
Bug 32203: Add render_auth_page() function (9.18 KB, patch)
2022-11-15 01:13 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2022-11-14 23:51:14 UTC
checkauth() appears to be 706 lines at the time of this writing.

The "AUTH rejected" section is 154 lines or over 20% of the function. 

It's a section devoted mainly to setting up the authentication template, so it's a very low risk part to change, and for the most part easy to change, since you just need to check the display on a failed auth.
Comment 1 David Cook 2022-11-15 01:13:18 UTC
Created attachment 143888 [details] [review]
Bug 32203: Add render_auth_page() function

This change adds a "render_auth_page()" function to C4::Auth for
rendering the login page for a failed auth/login.

Test plan:
0. Don't apply patch yet
1. In new tab, go to http://localhost:8081/ and do a failed login
2. In new tab, go to http://localhost:8080/ and do a failed login
3. Apply patch
4. In new tab, go to http://localhost:8081/ and do a failed login
5. In new tab, go to http://localhost:8080/ and do a failed login
6. Note how the failed login screens look the same after the patch
Comment 2 David Cook 2022-11-15 01:16:25 UTC
I think I let my excitement to refactor C4::Auth get the better of me. 

Resetting this one to default assignee as I've run out of steam. This patch probably needs to be split out into 30-50 smaller patches to make the amount of testing actually workable.
Comment 3 David Cook 2022-11-15 01:29:14 UTC
Actually, I think I made a mistake taking out "suggestion" as it doesn't use Koha.Preference() in the masthead, so the purchase suggestions wouldn't show up correctly.
Comment 4 David Cook 2023-09-20 02:42:26 UTC
This one is still worthwhile but I think it would be a good idea to break it up into multiple more easily testable bug reports.