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.
I suppose I'll say the Version is Main and then it would need to be backported back to 24.11.x...
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.
Created attachment 185262 [details] [review] Bug 40622: Add missing interface parameter This change adds the missing interface parameter which Bug 38233 initially added.
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.
Indeed, the original bug unit test does not cover this change. We must add a test on item datas in the MARCXML content.
(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".
(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...
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.
Okie dokie... we've got a manual test and a unit test. Huzzah!
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>
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>
Moving this to the next queue. Not for main.
(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.
(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 nearly applyed on 24.11.x but seen last comments. I wait ;)
(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?
(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).
(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?
(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?
(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.