Bug 40622

Summary: Bug 38233 not properly applied to 24.11.x, 25.05.x, and main
Product: Koha Reporter: David Cook <dcook>
Component: Web servicesAssignee: David Cook <dcook>
Status: Signed Off --- QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: fridolin.somers, lucas, m.de.rooy
Version: MainKeywords: rel_24_11_candidate, rel_25_05_candidate
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
ILS-DI GetRecords will now show the OPAC version of "marcxml".
Version(s) released in:
Circulation function:
Bug Depends on: 38233, 31224    
Bug Blocks:    
Attachments: Bug 40622: Add missing interface parameter
Bug 40622: Use OPAC MARCXML for ILS-DI GetRecords
Bug 40622: Add missing interface parameter
Bug 40622: Use OPAC MARCXML for ILS-DI GetRecords

Description David Cook 2025-08-08 06:40:05 UTC
A close examination of the code in "C4/ILSDI/Services.pm" shows that only half the patch from bug 38233 was applied to 24.11.00.

It was properly backported to 24.05.06, so anyone upgrading from 24.05.06 to 24.11.00 would experience a regression.
Comment 1 David Cook 2025-08-08 06:42:04 UTC
I suppose I'll say the Version is Main and then it would need to be backported back to 24.11.x...
Comment 2 David Cook 2025-08-08 06:54:34 UTC
Since bug 38233 and bug 31224 went in around the same time, it looks like there was probably a merge conflict and the fix wasn't done correctly.
Comment 3 David Cook 2025-08-08 07:06:08 UTC
Created attachment 185262 [details] [review]
Bug 40622: Add missing interface parameter

This change adds the missing interface parameter which
Bug 38233 initially added.
Comment 4 David Cook 2025-08-08 07:06:36 UTC
Leaving as ASSIGNED as I'll need to come back to this with a test plan, verify it, and see about adding a unit test.
Comment 5 Fridolin Somers 2025-08-11 06:35:47 UTC
Indeed, the original bug unit test does not cover this change.
We must add a test on item datas in the MARCXML content.
Comment 6 David Cook 2025-08-20 06:39:39 UTC
(In reply to Fridolin Somers from comment #5)
> Indeed, the original bug unit test does not cover this change.
> We must add a test on item datas in the MARCXML content.

So it doesn't need to be item data per se.

/cgi-bin/koha/ilsdi.pl?service=GetRecords&id=1+2+99999 also returns the "marcxml".
Comment 7 David Cook 2025-08-20 07:13:00 UTC
(In reply to David Cook from comment #6)
> (In reply to Fridolin Somers from comment #5)
> > Indeed, the original bug unit test does not cover this change.
> > We must add a test on item datas in the MARCXML content.
> 
> So it doesn't need to be item data per se.
> 
> /cgi-bin/koha/ilsdi.pl?service=GetRecords&id=1+2+99999 also returns the
> "marcxml".

I've run out of time today, but I've nearly got a unit test that will work...

It's fiddly but we'll get there...
Comment 8 David Cook 2025-08-21 04:01:07 UTC
Created attachment 185617 [details] [review]
Bug 40622: Use OPAC MARCXML for ILS-DI GetRecords

This change makes it so that the ILS-DI GetRecords output for
"marcxml" will contain the OPAC version of the record.

Test plan:
0. Apply the patches
1. prove t/db_dependent/ILSDI_Services.t
2. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=29
3. Click "Settings" to change the framework to Default, and click "Save"
4. Go to http://localhost:8081/cgi-bin/koha/admin/marc_subfields_structure.pl
?op=add_form&tagfield=245&tagsubfield=a&frameworkcode=#subafield
5. Untick the "OPAC" box and click "Save changes"
6. Go to system preferences
7. Change "ILS-DI" to "Enable"
8. Change "ILS-DI:AuthorizedIPs" to 0.0.0.0/0
9. Go to http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=GetRecords&id=29
10. Note that you cannot see the title in the MARCXML.
Comment 9 David Cook 2025-08-21 04:01:39 UTC
Okie dokie... we've got a manual test and a unit test. Huzzah!
Comment 10 David Nind 2025-09-01 03:00:34 UTC
Created attachment 186000 [details] [review]
Bug 40622: Add missing interface parameter

This change adds the missing interface parameter which
Bug 38233 initially added.

Signed-off-by: David Nind <david@davidnind.com>
Comment 11 David Nind 2025-09-01 03:00:37 UTC
Created attachment 186001 [details] [review]
Bug 40622: Use OPAC MARCXML for ILS-DI GetRecords

This change makes it so that the ILS-DI GetRecords output for
"marcxml" will contain the OPAC version of the record.

Test plan:
0. Apply the patches
1. prove t/db_dependent/ILSDI_Services.t
2. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=29
3. Click "Settings" to change the framework to Default, and click "Save"
4. Go to http://localhost:8081/cgi-bin/koha/admin/marc_subfields_structure.pl
?op=add_form&tagfield=245&tagsubfield=a&frameworkcode=#subafield
5. Untick the "OPAC" box and click "Save changes"
6. Go to system preferences
7. Change "ILS-DI" to "Enable"
8. Change "ILS-DI:AuthorizedIPs" to 0.0.0.0/0
9. Go to http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=GetRecords&id=29
10. Note that you cannot see the title in the MARCXML.

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 Marcel de Rooy 2025-09-26 07:06:19 UTC
Moving this to the next queue. Not for main.
Comment 13 David Cook 2025-09-28 23:29:05 UTC
(In reply to Marcel de Rooy from comment #12)
> Moving this to the next queue. Not for main.

No, it's needed for main too. 

24.11, 25.05, and main all still need it. 

24.05 is the only version that got it.
Comment 14 Marcel de Rooy 2025-09-29 07:41:43 UTC
(In reply to David Cook from comment #13)
> (In reply to Marcel de Rooy from comment #12)
> > Moving this to the next queue. Not for main.
> 
> No, it's needed for main too. 
> 
> 24.11, 25.05, and main all still need it. 
> 
> 24.05 is the only version that got it.

Okay, the title is bit misleading :)
Could you change title and status accordingly?
Thanks
Comment 15 Fridolin Somers 2025-09-29 08:17:28 UTC
I nearly applyed on 24.11.x but seen last comments.
I wait ;)
Comment 16 David Cook 2025-09-30 00:55:50 UTC
(In reply to Marcel de Rooy from comment #14)
> (In reply to David Cook from comment #13)
> > (In reply to Marcel de Rooy from comment #12)
> > > Moving this to the next queue. Not for main.
> > 
> > No, it's needed for main too. 
> > 
> > 24.11, 25.05, and main all still need it. 
> > 
> > 24.05 is the only version that got it.
> 
> Okay, the title is bit misleading :)
> Could you change title and status accordingly?
> Thanks

I thought it made sense because it would be all versions from 24.11 onward, but I'll try to rephrase the title :)

Not sure what you mean about changing the status though?
Comment 17 David Cook 2025-09-30 00:57:33 UTC
(In reply to Fridolin Somers from comment #15)
> I nearly applyed on 24.11.x but seen last comments.
> I wait ;)

Basically, Bug 38233 was added to 24.05.06 and 24.11.00 (main at that time).

However, there was a merge error in 24.11.00 (main at that time), so it wasn't properly applied.

That means that the only version that has the fix for Bug 38233 is 24.05.06.

So main needs this fix, and it should be backported to 25.05 and 24.11 (but not 24.05).
Comment 18 David Cook 2025-09-30 01:01:25 UTC
(In reply to Marcel de Rooy from comment #14)
> Okay, the title is bit misleading :)
> Could you change title and status accordingly?

I think that I understand what you mean now. It must've sounded like it was just needed for the older versions because I used the version numbers specifically. Hopefully this re-worded one is better?
Comment 19 David Cook 2025-09-30 01:02:08 UTC
(In reply to Marcel de Rooy from comment #14)
> Okay, the title is bit misleading :)
> Could you change title and status accordingly?

Regarding the status, do you mean it should go back to "Signed Off" and you're not Passing QA on it?
Comment 20 Marcel de Rooy 2025-09-30 11:08:03 UTC
(In reply to David Cook from comment #19)
> (In reply to Marcel de Rooy from comment #14)
> > Okay, the title is bit misleading :)
> > Could you change title and status accordingly?
> 
> Regarding the status, do you mean it should go back to "Signed Off" and
> you're not Passing QA on it?

It didnt really pass QA yet. It was just moved out of SO. But your explanation qualifies it for QA shortly.