[Bug 39337] New: Use SQL field to apply OpacSuppression in OAI-PMH
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39337 Bug ID: 39337 Summary: Use SQL field to apply OpacSuppression in OAI-PMH Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@ptfs-europe.com QA Contact: testopia@bugs.koha-community.org Depends on: 37713, 38330 This is a follow-up to bug 37713 and bug 38330. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37713 [Bug 37713] OAI-PMH - Honour OpacSuppression syspref https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38330 [Bug 38330] Make bib-level suppression a biblio table field instead of part of a marc tag -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
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 #1 from David Cook <dcook@prosentient.com.au> --- This would be ideal from both a performance and record visibility perspective. The correct solution on bug 37713 is practical but not ideal and not consistent with the OPAC overall. -- 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 --- 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 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |olivia.reynolds@openfifth.c |ity.org |o.uk -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
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 #3 from Olivia Reynolds <olivia.reynolds@openfifth.co.uk> --- Created attachment 200886 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200886&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 -- 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 ---------------------------------------------------------------------------- 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 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 #5 from David Nind <david@davidnind.com> --- Created attachment 201133 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201133&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> -- 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 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Keywords| |release-notes-needed --- Comment #7 from David Nind <david@davidnind.com> --- Testing notes (KTD): 1. Enable the OAI-PMH system preference. 2. I applied both patches at once. ==> Test plan for the first patch (Add is_opac_suppressed helper to C4::Context) 3. After Step 6: I did a restart_all and clear browser cache to make sure setting changes were recognised. 4. For Step 5e: With OPAC suppression disabled (Don't hide) (and no IP address range restriction), I get: [ "application/marcxml+xml", "application/marc-in-json", "application/marc", "text/plain" ] 5. For Step 5f (with steps 6 and 7): With OPAC suppression enabled (Hide) (and no IP address range restriction), I get: Error There is no biblio record with this identifier 6. For Step 5g (with steps 6 and 7): With OPAC suppression enabled (Hide) (and no IP address range restriction), I get: there is no record listed for KOHA-OAI-TEST:2 ==> Test plan for the second patch (Filter out suppressed OAI records in SQL) 7. Everything works as per the test plan. -- 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 ---------------------------------------------------------------------------- Keywords|release-notes-needed | -- 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 #9 from David Nind <david@davidnind.com> --- Thanks Martin for the release notes! -- 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 #11 from David Cook <dcook@prosentient.com.au> --- Test plan missing the need to turn on system preference "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 --- 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 ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- 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 #13 from David Cook <dcook@prosentient.com.au> --- Created attachment 201687 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201687&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> -- 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 --- Comment #15 from David Cook <dcook@prosentient.com.au> --- Nice one, Olivia. This will be a useful change. Look forward to seeing more from you. -- 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 Olivia Reynolds <olivia.reynolds@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #17 from Olivia Reynolds <olivia.reynolds@openfifth.co.uk> --- Thanks for catching both of those issues, Pedro! -- 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 --- Comment #19 from Olivia Reynolds <olivia.reynolds@openfifth.co.uk> --- Created attachment 203937 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203937&action=edit Bug 39337: (follow-up) Default to not suppressing OAI records In the case when a biblio is deleted, we should not suppress its orphaned records. Therefore, invert the conditions in the WHERE clause to not suppress the record if no biblio could be found. Test plan: 1. Enable the system preferences "OAI-PMH" and "OpacSuppression". Ensure "OpacSuppressionByIPRange" is blank. 2. Navigate to /cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=oai_dc 3. Select a displayed record, and delete its corresponding biblio. 4. Refresh the OAI page, verify the record has vanished. 5. Apply patch. 6. Refresh the OAI page, verify the record now appears and is marked as deleted. It may have moved to the top of the list. 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 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #201687|0 |1 is obsolete| | Attachment #201688|0 |1 is obsolete| | Attachment #203936|0 |1 is obsolete| | Attachment #203937|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 #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 #21 from David Nind <david@davidnind.com> --- Created attachment 203941 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203941&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> 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 --- Comment #23 from David Nind <david@davidnind.com> --- Created attachment 203943 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203943&action=edit Bug 39337: (follow-up) Default to not suppressing OAI records In the case when a biblio is deleted, we should not suppress its orphaned records. Therefore, invert the conditions in the WHERE clause to not suppress the record if no biblio could be found. Test plan: 1. Enable the system preferences "OAI-PMH" and "OpacSuppression". Ensure "OpacSuppressionByIPRange" is blank. 2. Navigate to /cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=oai_dc 3. Select a displayed record, and delete its corresponding biblio. 4. Refresh the OAI page, verify the record has vanished. 5. Apply patch. 6. Refresh the OAI page, verify the record now appears and is marked as deleted. It may have moved to the top of the list. 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 ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #25 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Hi all, putting this back in the queue as the follow-ups are addressing QA concerns. -- 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 --- Comment #26 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Thanks everyone! Pushed to main for 26.11! -- 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 #28 from Olivia Reynolds <olivia.reynolds@openfifth.co.uk> --- Thanks for catching that, I've updated the test to match the new interface. After these couple of regressions, I've also rerun the tests with `prove -r t xt` and didn't see any relevant issues. -- 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 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | --- Comment #30 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Follow-up pushed. Thanks Olivia! -- 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.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org