Bug 35008 - ILS-DI should not ask for login with OpacPublic disabled
Summary: ILS-DI should not ask for login with OpacPublic disabled
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Aleisha Amohia
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-10-08 21:58 UTC by Aleisha Amohia
Modified: 2023-11-14 15:20 UTC (History)
5 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.06


Attachments
Bug 35008: Make ILS-DI not require auth when OpacPublic disabled (907 bytes, patch)
2023-10-08 22:01 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 35008: Make ILS-DI not require auth when OpacPublic disabled (1.55 KB, patch)
2023-10-09 20:41 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 35008: Make ILS-DI not require auth when OpacPublic disabled (1.65 KB, patch)
2023-11-03 07:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35008: (QA follow-up) Add selenium unit tests (5.52 KB, patch)
2023-11-10 01:08 UTC, David Cook
Details | Diff | Splinter Review
Bug 35008: (QA follow-up) Add selenium unit tests (5.62 KB, patch)
2023-11-10 13:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2023-10-08 21:58:41 UTC
The ILS-DI script should still be accessible if OpacPublic is disabled
Comment 1 Aleisha Amohia 2023-10-08 22:01:04 UTC
Created attachment 156714 [details] [review]
Bug 35008: Make ILS-DI not require auth when OpacPublic disabled

To test, disable OpacPublic and ensure a call to ilsdi.pl will still return expected results from a private browser, not logged into the OPAC.

Sponsored-by: Auckland University of Technology
Comment 2 David Cook 2023-10-09 00:29:34 UTC
The bug title is a bit confusing but I see what you mean. I'm going to come back to this one...
Comment 3 Katrin Fischer 2023-10-09 20:16:08 UTC
(In reply to David Cook from comment #2)
> The bug title is a bit confusing but I see what you mean. I'm going to come
> back to this one...

I also got a little confused here - made an attempt to improve the bug description a bit.
Comment 4 Katrin Fischer 2023-10-09 20:41:26 UTC
Created attachment 156768 [details] [review]
Bug 35008: Make ILS-DI not require auth when OpacPublic disabled

To test, disable OpacPublic and ensure a call to ilsdi.pl will still return expected results from a private browser, not logged into the OPAC.

Sponsored-by: Auckland University of Technology
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 5 Katrin Fischer 2023-10-09 20:41:59 UTC
Please remember to run the latest version of the QA script, I amended the patch to fix it:

 WARN	C4/Auth.pm
   WARN	  tidiness
		The file is less tidy than before (bad/messy lines before: 444, now: 445)
Comment 6 Marcel de Rooy 2023-11-03 07:44:51 UTC
Created attachment 158301 [details] [review]
Bug 35008: Make ILS-DI not require auth when OpacPublic disabled

To test, disable OpacPublic and ensure a call to ilsdi.pl will still return expected results from a private browser, not logged into the OPAC.

Sponsored-by: Auckland University of Technology
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2023-11-03 07:46:20 UTC
Just noting that we did not add a test here, but also that OpacPublic is not found at all in Auth.t. So no blocker for me.
Comment 8 Tomás Cohen Arazi 2023-11-08 14:00:02 UTC
(In reply to Marcel de Rooy from comment #7)
> Just noting that we did not add a test here, but also that OpacPublic is not
> found at all in Auth.t. So no blocker for me.

I don't feel comfortable making a change in Auth.pm and not having tests.
Comment 9 Katrin Fischer 2023-11-09 20:48:41 UTC
Most of the patch is white space changes. The actual change is adding another page to a list of pages where we already have applied this same behaviour. 

What would we test here?

t/db_dependent/Auth.pm still passes.
Comment 10 David Cook 2023-11-09 22:39:09 UTC
(In reply to Katrin Fischer from comment #9)
> Most of the patch is white space changes. The actual change is adding
> another page to a list of pages where we already have applied this same
> behaviour. 
> 
> What would we test here?
> 
> t/db_dependent/Auth.pm still passes.

I think a Selenium test would probably be good here. I wasn't planning on doing Koha work today but I'll see if I can knock together one quickly...
Comment 11 David Cook 2023-11-10 00:52:35 UTC
So I'm working on the selenium script for this, and it turns out this change works differently to how I thought...

Turns out the ILS-DI XML API is enabled even when OpacPublic is disabled.

It's just the HTML response that you don't get when OpacPublic is disabled...

In hindsight that's obvious but wasn't obvious from the description and comments...
Comment 12 David Cook 2023-11-10 01:08:05 UTC
Created attachment 158780 [details] [review]
Bug 35008: (QA follow-up) Add selenium unit tests
Comment 13 Tomás Cohen Arazi 2023-11-10 12:58:13 UTC
(In reply to David Cook from comment #12)
> Created attachment 158780 [details] [review] [review]
> Bug 35008: (QA follow-up) Add selenium unit tests

Thank you!
Comment 14 Tomás Cohen Arazi 2023-11-10 13:01:16 UTC
Created attachment 158804 [details] [review]
Bug 35008: (QA follow-up) Add selenium unit tests

This patch addds selenium tests for the ILS-DI feature.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2023-11-10 14:02:25 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 16 Fridolin Somers 2023-11-11 07:42:12 UTC
Small enhancement, nearly a bug. I choose to backport.

Pushed to 23.05.x for 23.05.06