Bug 35008

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

The bug title is a bit confusing but I see what you mean. I'm going to come back to this one...

(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.

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>

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)

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>

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.

(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.

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.

(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...

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...

Created attachment 158780 [details] [review]
Bug 35008: (QA follow-up) Add selenium unit tests

(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!

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>

Pushed to master for 23.11.

Nice work everyone, thanks!

Small enhancement, nearly a bug. I choose to backport.

Pushed to 23.05.x for 23.05.06

No changes to the manual required.