https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 --- Comment #2 from David Cook <dcook@prosentient.com.au> --- I reckon this can probably be a dupe of bug 37713 really. But we'll tackle it after we're done with bug 38330 I think -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 Olivia Reynolds <olivia.reynolds@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Comma delimited| |Open Fifth list of Sponsors| | Sponsorship status|--- |Sponsored Patch complexity|--- |Medium patch Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 --- Comment #4 from Olivia Reynolds <olivia.reynolds@openfifth.co.uk> --- Created attachment 200887 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200887&action=edit Bug 39337: Filter out suppressed OAI records in SQL Rather than fetching each biblio before choosing to suppress it, this patch performs this filtering in the initial SQL query that fetches the biblionumbers. Note: for queries including items, this additional filtering is performed in each of the three subqueries, necessitating three quasi-joins against biblio. If we were to hoist this filtering to the outer layer, we would also need to remove the LIMIT from each subquery so as to allow each of them to be fairly weighted. As the preceding comment implies keeping the subqueries separate has been beneficial for performance, I've elected not to hoist the WHERE. Test plan (specific URLs given for KTD, with thanks to David Cook): 1. Apply patch. 2. Navigate to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=2 3. Set 942$n to "Yes" and save the record. 4. Disable the system preference "OpacSuppression", and ensure "OpacSuppressionByIPRange" is blank. 5a. Verify you can successfully access http://localhost:8080/cgi-bin/koha/oai.pl?verb=GetRecord&metadataPrefix=oai_dc&identifier=KOHA-OAI-TEST:2 5b. Verify KOHA-OAI-TEST-2 appears on http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=oai_dc 6. Enable the system preference "OpacSuppression", and ensure "OpacSuppressionByIPRange" is blank. 7. Repeat steps 5a-5b, verifying that: - the OAI GetRecord page returns "There is no biblio record with this identifier". - the OAI ListRecords page does not show KOHA-OAI-TEST-2. Sponsored-by: Open Fifth -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #200886|0 |1 is obsolete| | Attachment #200887|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 --- Comment #6 from David Nind <david@davidnind.com> --- Created attachment 201134 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201134&action=edit Bug 39337: Filter out suppressed OAI records in SQL Rather than fetching each biblio before choosing to suppress it, this patch performs this filtering in the initial SQL query that fetches the biblionumbers. Note: for queries including items, this additional filtering is performed in each of the three subqueries, necessitating three quasi-joins against biblio. If we were to hoist this filtering to the outer layer, we would also need to remove the LIMIT from each subquery so as to allow each of them to be fairly weighted. As the preceding comment implies keeping the subqueries separate has been beneficial for performance, I've elected not to hoist the WHERE. Test plan (specific URLs given for KTD, with thanks to David Cook): 1. Apply patch. 2. Navigate to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=2 3. Set 942$n to "Yes" and save the record. 4. Disable the system preference "OpacSuppression", and ensure "OpacSuppressionByIPRange" is blank. 5a. Verify you can successfully access http://localhost:8080/cgi-bin/koha/oai.pl?verb=GetRecord&metadataPrefix=oai_dc&identifier=KOHA-OAI-TEST:2 5b. Verify KOHA-OAI-TEST-2 appears on http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=oai_dc 6. Enable the system preference "OpacSuppression", and ensure "OpacSuppressionByIPRange" is blank. 7. Repeat steps 5a-5b, verifying that: - the OAI GetRecord page returns "There is no biblio record with this identifier". - the OAI ListRecords page does not show KOHA-OAI-TEST-2. Sponsored-by: Open Fifth Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |Records marked for OPAC release notes| |suppression (942$n) are now | |correctly hidden from | |OAI-PMH responses when the | |`OpacSuppression` system | |preference is enabled. | |Previously, suppressed | |records could still appear | |in OAI-PMH `ListRecords` | |and `GetRecord` outputs, | |leaking catalogue data to | |harvesters. Suppression is | |now applied consistently | |across the OPAC, REST API, | |and OAI-PMH, and the | |`OpacSuppressionByIPRange` | |preference is also honoured | |by OAI-PMH. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |dcook@prosentient.com.au |y.org | --- Comment #8 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Great work.. love the consistency improvement of the first patch.. 'Today I learned!' I'm going to ask David Cook to QA this one for us :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 --- Comment #10 from David Cook <dcook@prosentient.com.au> --- Hmm seems like there's been some scope creep here. The bug title is "Use SQL field to apply OpacSuppression in OAI-PMH" but it looks like this is touching a lot of unrelated places: C4/Context.pm | 20 ++++++++++++++++++++ C4/Output.pm | 20 ++++---------------- Koha/OAI/Server/GetRecord.pm | 22 ++++++++++++++-------- Koha/OAI/Server/ListBase.pm | 4 ++++ Koha/OAI/Server/Repository.pm | 5 +---- Koha/REST/V1/Biblios.pm | 2 +- opac/opac-ISBDdetail.pl | 3 +-- opac/opac-MARCdetail.pl | 3 +-- opac/opac-detail.pl | 3 +-- opac/opac-search.pl | 13 +------------ t/Context.t | 32 +++++++++++++++++++++++++++++++- 11 files changed, 79 insertions(+), 48 deletions(-) But... it is all related... ok. Quick code review is looking good. I like the change to Koha::OAI::Server::GetRecord for reusing the WHERE clause. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 --- Comment #12 from David Cook <dcook@prosentient.com.au> --- (In reply to Martin Renvoize (ashimema) from comment #8)

Great work.. love the consistency improvement of the first patch.. 'Today I learned!'

I'm going to ask David Cook to QA this one for us :)

I think I forgot about that part. Yep, all in all I think this does the trick. It would be interesting to see how much this improves performance for the OAI-PMH server. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #201133|0 |1 is obsolete| | Attachment #201134|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 --- Comment #14 from David Cook <dcook@prosentient.com.au> --- Created attachment 201688 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201688&action=edit Bug 39337: Filter out suppressed OAI records in SQL Rather than fetching each biblio before choosing to suppress it, this patch performs this filtering in the initial SQL query that fetches the biblionumbers. Note: for queries including items, this additional filtering is performed in each of the three subqueries, necessitating three quasi-joins against biblio. If we were to hoist this filtering to the outer layer, we would also need to remove the LIMIT from each subquery so as to allow each of them to be fairly weighted. As the preceding comment implies keeping the subqueries separate has been beneficial for performance, I've elected not to hoist the WHERE. Test plan (specific URLs given for KTD, with thanks to David Cook): 1. Apply patch. 2. Navigate to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=2 3. Set 942$n to "Yes" and save the record. 4. Disable the system preference "OpacSuppression", and ensure "OpacSuppressionByIPRange" is blank. 5a. Verify you can successfully access http://localhost:8080/cgi-bin/koha/oai.pl?verb=GetRecord&metadataPrefix=oai_dc&identifier=KOHA-OAI-TEST:2 5b. Verify KOHA-OAI-TEST-2 appears on http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=oai_dc 6. Enable the system preference "OpacSuppression", and ensure "OpacSuppressionByIPRange" is blank. 7. Repeat steps 5a-5b, verifying that: - the OAI GetRecord page returns "There is no biblio record with this identifier". - the OAI ListRecords page does not show KOHA-OAI-TEST-2. Sponsored-by: Open Fifth Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Cook <dcook@prosentient.com.au> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |pedro.amorim@openfifth.co.u | |k Status|Passed QA |Failed QA --- Comment #16 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Hi all, A) These patches are causing the following test file to fail: prove t/db_dependent/OAI/Server.t B) It appears there is an unintended behavior here: 1) Launch ktd without these patches 2) Enable OpacSuppression and OAI 3) Delete biblio 79 4) Access: http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=oai_dc&from=2000-01-01 5) Confirm record 79 shows as deleted 6) Apply patches, restart_all, repeat 4, verify it no longer shows as deleted. This would cause the OAI to serve stale information to anyone harvesting. Can you please revisit? Thanks in advance! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 --- Comment #18 from Olivia Reynolds <olivia.reynolds@openfifth.co.uk> --- Created attachment 203936 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203936&action=edit Bug 39337: (follow-up) Explicitly name tables As we're aliasing the primary tables as `main`, we need to use that name to refer to their columns. Test plan: 1. Apply patch. 2. Prove t/db_dependent/OAI/Server.t and verify no tests fail. Sponsored-by: Open Fifth -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 --- Comment #20 from David Nind <david@davidnind.com> --- Created attachment 203940 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203940&action=edit Bug 39337: Add is_opac_suppressed helper to C4::Context Previously, OPAC suppression was applied inconsistently, with only some callers taking OpacSuppressionByIPRange into account. This patch adds a new helper to C4::Context to determine whether OPAC suppression should be active for this request, and switches to using it everywhere that was previously manually checking the system preferences. Test plan (specific URLs given for KTD, with thanks to David Cook): 1. Apply patch. 2. Navigate to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=2 3. Set 942$n to "Yes" and save the record. 4. Disable the system preference "OpacSuppression", and ensure "OpacSuppressionByIPRange" is blank. 5a. Verify you can successfully access http://localhost:8080/cgi-bin/koha/opac-detail.pl?biblionumber=2 5b. Verify you can successfully access http://localhost:8080/cgi-bin/koha/opac-MARCdetail.pl?biblionumber=2 5c. Verify you can successfully access http://localhost:8080/cgi-bin/koha/opac-ISBDdetail.pl?biblionumber=2 5d. Verify you can successfully access http://localhost:8080/cgi-bin/koha/opac-search.pl?idx=&q=The+ideas+behind&weight_search=1 5e. Verify you can successfully access http://localhost:8080/api/v1/public/biblios/2 5f. Verify you can successfully access http://localhost:8080/cgi-bin/koha/oai.pl?verb=GetRecord&metadataPrefix=oai_dc&identifier=KOHA-OAI-TEST:2 5g. Verify KOHA-OAI-TEST-2 appears on http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=oai_dc 6. Enable the system preference "OpacSuppression", and ensure "OpacSuppressionByIPRange" is blank. 7. Repeat steps 5a-5g, verifying that: - the OPAC details pages return "Record blocked". - the OPAC search page returns "No results found". - the API returns "Bibliographic record not found". - the OAI GetRecord and ListRecords show "Record Status: deleted" for KOHA-OAI-TEST-2. 8. Keep "OpacSuppression" enabled, but set "OpacSuppressionByIPRange" to "." (to allow all IPs to bypass suppression). 9. Repeat steps 5a-5g, verifying that every page is successful again. 10. Run `prove t/Context.t`, and verify all tests are successful. Sponsored-by: Open Fifth Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 --- Comment #22 from David Nind <david@davidnind.com> --- Created attachment 203942 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203942&action=edit Bug 39337: (follow-up) Explicitly name tables As we're aliasing the primary tables as `main`, we need to use that name to refer to their columns. Test plan: 1. Apply patch. 2. Prove t/db_dependent/OAI/Server.t and verify no tests fail. Sponsored-by: Open Fifth Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the|Records marked for OPAC |Records marked for OPAC release notes|suppression (942$n) are now |suppression (942$n) are now |correctly hidden from |correctly hidden from |OAI-PMH responses when the |OAI-PMH responses when the |`OpacSuppression` system |`OpacSuppression` system |preference is enabled. |preference is enabled. |Previously, suppressed | |records could still appear |Suppression is now applied |in OAI-PMH `ListRecords` |consistently across the |and `GetRecord` outputs, |OPAC, REST API, and |leaking catalogue data to |OAI-PMH, and the |harvesters. Suppression is |`OpacSuppressionByIPRange` |now applied consistently |system preference is also |across the OPAC, REST API, |honoured by OAI-PMH. |and OAI-PMH, and the | |`OpacSuppressionByIPRange` |Previously, suppressed |preference is also honoured |records could still appear |by OAI-PMH. |in OAI-PMH `ListRecords` | |and `GetRecord` outputs, | |leaking catalogue data to | |harvesters. --- Comment #24 from David Nind <david@davidnind.com> --- Additional testing notes (using KTD): 1. Set OAI-PMH system preference to "Enable". 2. For steps 5f and 5g (first patch): I get blank pages in chromium-based browsers, but you can view the page source and see the XML; information appears fine when using Firefox-based browsers. 3. For step 5g (first patch) and 5b (second patch): KOHA-OAI-TEST-2 doesn't appear, but KOHA-OAI-TEST:2 does. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |26.11.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed --- Comment #27 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Hi all, it appears this broke the following tests: prove t/db_dependent/Output.t Can you please follow-up when convenient? Thanks in advance! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 --- Comment #29 from Olivia Reynolds <olivia.reynolds@openfifth.co.uk> --- Created attachment 206064 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206064&action=edit Bug 39337: (follow-up) Mock OpacSuppression in Output tests Before the refactoring in this bug, redirect_if_opac_suppressed was intended to be called after checking OpacSuppression was enabled. Now it performs this check internally, so to adapt these unit tests correspondingly, we mock OpacSuppression to be enabled, as was the implied state before. Test plan: 1. Prove t/db_dependent/Output.t and verify all tests pass. Sponsored-by: Open Fifth -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com --- Comment #31 from Jonathan Druart <jonathan.druart@gmail.com> --- This new method is_opac_suppressed does not belong to C4::Context. The rule should be: Don't add anything new to C4::Context. -- You are receiving this mail because: You are watching all bug changes.