To reproduce: 1. Go to Koha Administration -> Koha bibliographic frameworks 2. View the MARC structure for your BKS framework (or something else) 3. Search for tag 583, edit subfields 4. Go to subfield 'x' - nonpublic note. Confirm the OPAC visibility checkbox is UNCHECKED. 5. Edit or create a record using the BKS framework. Put a note in the 583$x. 6. View this record in the OPAC 7. Go to the Title notes tab. Confirm the non-public note is showing, even though the framework says it should not be visible via the OPAC.
I haven't verified yet but that's interesting... Ah because of this sucker: my $marcnotesarray = $biblio->get_marc_notes({ marcflavour => $marcflavour, opac => 1 }); That function should have a "record" parameter. It would solve the problem (and improve efficiency by reducing the number of unnecessary database calls).
(In reply to David Cook from comment #1) > I haven't verified yet but that's interesting... > > Ah because of this sucker: > > my $marcnotesarray = $biblio->get_marc_notes({ marcflavour => $marcflavour, > opac => 1 }); > > That function should have a "record" parameter. It would solve the problem > (and improve efficiency by reducing the number of unnecessary database > calls). In fact, this could be a class method rather than an object method... for loops (like basket.pl and opac-basket.pl) that could add up...
Created attachment 135339 [details] [review] Bug 30744: Use RecordProcessor in get_marc_notes This patch utilises RecordProcessor to filter the MARC::Record for the right interface prior to constructing the marc notes array. We also remove the use of C4::XSLT for replacing AV values in the MARC fields in preference to using the RecordProcessor filter.
We had customers starting to notice this, so I've taken a pragmatic approach here for now.. I think this all needs a little more thought as to what level it should all sit at as fetching and processing the MARC data twice is a bit silly.
Created attachment 135354 [details] [review] Bug 30744: Use RecordProcessor in get_marc_notes This patch utilises RecordProcessor to filter the MARC::Record for the right interface prior to constructing the marc notes array. We also remove the use of C4::XSLT for replacing AV values in the MARC fields in preference to using the RecordProcessor filter.
(In reply to Martin Renvoize from comment #4) > We had customers starting to notice this, so I've taken a pragmatic approach > here for now.. > > I think this all needs a little more thought as to what level it should all > sit at as fetching and processing the MARC data twice is a bit silly. I don't like it but I do see the pragmatism... Yet, if we do this, I don't think we'll ever fix the performance of get_marc_notes() :/. Would a compromise be to add a "record" parameter to "get_marc_notes", and just update the call in opac/opac-detail.pl for now (we could open other tickets for the other Perl scripts)? That would fix Bug 30744 without duplicating processing and with less code.
I was actually just seeing this as a stepping stone.. I intend to work on bug 29652 and bug 29697 next and as part of those I'm thinking we cache filtered records much lower in the stack so we're not passing around a record but rather any method we call already has a shared copy to refer to.
Created attachment 135365 [details] [review] Bug 30744: Use RecordProcessor in get_marc_notes This patch utilises RecordProcessor to filter the MARC::Record for the right interface prior to constructing the marc notes array. We also remove the use of C4::XSLT for replacing AV values in the MARC fields in preference to using the RecordProcessor filter.
Created attachment 135415 [details] [review] Bug 30744: Use RecordProcessor in get_marc_notes This patch utilises RecordProcessor to filter the MARC::Record for the right interface prior to constructing the marc notes array. We also remove the use of C4::XSLT for replacing AV values in the MARC fields in preference to using the RecordProcessor filter. Signed-off-by: David Nind <david@davidnind.com>
Testing notes (koha-testing-docker): 1. I followed the steps in comment #1 2. After applying the patch, I refreshed the OPAC page and the nonpublic note was no longer visible. 3. I changed the framework so 583$x should be visible in the OPAC, and this works as expected.
I am a little worried that we won't be able to backport this looking at the dependency tree. Would it be possible to create a simple fix for the older versions in addition to this? I wonder if it might have been better to deal with expanding authorised values separately? Detail page explodes: Can't locate object method "search" via package "Koha::ClassSources" (perhaps you forgot to load "Koha::ClassSources"?) at /kohadevbox/koha/C4/Biblio.pm line 1510 Tests fail: t/db_dependent/Koha/Biblio.t .. 13/21 # Failed test 'Authorised value is correctly parsed to show description rather than code' # at t/db_dependent/Koha/Biblio.t line 719. # got: 'CODE' # expected: 'Description should show' # Failed test 'Authorised value is correctly parsed for OPAC to show description rather than code' # at t/db_dependent/Koha/Biblio.t line 727. # got: 'CODE' # expected: 'Description should show OPAC' # Looks like you failed 2 tests of 14. t/db_dependent/Koha/Biblio.t .. 15/21 # Failed test 'get_marc_notes() MARC21 tests' # at t/db_dependent/Koha/Biblio.t line 736. t/db_dependent/Koha/Biblio.t .. 21/21 # Looks like you failed 1 test of 21. t/db_dependent/Koha/Biblio.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/21 subtests Test Summary Report ------------------- t/db_dependent/Koha/Biblio.t (Wstat: 256 Tests: 21 Failed: 1) Failed test: 15 Non-zero exit status: 1 Files=1, Tests=21, 14 wallclock secs ( 0.05 usr 0.01 sys + 11.36 cusr 1.68 csys = 13.10 CPU) Result: FAIL
Created attachment 135757 [details] [review] Bug 30744: Use RecordProcessor in get_marc_notes This patch utilises RecordProcessor to filter the MARC::Record for the right interface prior to constructing the marc notes array. We also remove the use of C4::XSLT for replacing AV values in the MARC fields in preference to using the RecordProcessor filter. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 135758 [details] [review] Bug 30744: Optionally allow passing record to prevent refetch This patch adds the option to pass the marcrecord into the call to prevent the extra DB fetch of the record.
* Minor rebase for change of filter name in bug 30848 (to fix Katrins issue) * Minor follow-up to allow for David's request that we can pass a record along rather than refetching it.
I can create a backportable version of this easily enough too
Created attachment 135762 [details] [review] Bug 30744: Use RecordProcessor in get_marc_notes This patch utilises RecordProcessor to filter the MARC::Record for the right interface prior to constructing the marc notes array. We also remove the use of C4::XSLT for replacing AV values in the MARC fields in preference to using the RecordProcessor filter. Signed-off-by: David Nind <david@davidnind.com>
OK, after discussing with Tomas, I've split the bug.. the simple quick fix can now be found in bug 30918 and the work here can now be seen as the next steps to reduce the chance of leaking data in other displays.
What's required to test this, since the quick fix is in another bug? Or is more work still required?
Moved the original bugfix to it's own bug and changed this to an architectural one. I'm still working through this tree at the moment really so perhaps not worth testing. If you were to test, it's basically to confirm that there are no user facing changes when this bug is applied atop bug 30198
Thanks Martin. I'll leave for a bit and come back to this one later.
There is something wrong in the design here. I really don't think it's correct to pass the marc to Koha::Biblio->get_marc_notes I feel like we need a Koha::MARC class that would have the 'get_notes' method. Koha::Biblio->get_marc_notes fetches the marc and call it If you have a marc already you could call it directly.
(In reply to Jonathan Druart from comment #21) > There is something wrong in the design here. > I really don't think it's correct to pass the marc to > Koha::Biblio->get_marc_notes > > I feel like we need a Koha::MARC class that would have the 'get_notes' > method. > > Koha::Biblio->get_marc_notes fetches the marc and call it > If you have a marc already you could call it directly. We allow for optionally passing the marc (at Davids request)... but yes, I generally agree.. there's a bug 'move get_marc_* methods to Koha::Biblio::Metadata.. which Tomas raised and I've been trying to think of how to achieve.. this is one of the stepping stones towards that end.
Created attachment 136783 [details] [review] Bug 30744: Use RecordProcessor in get_marc_notes This patch utilises RecordProcessor to filter the MARC::Record for the right interface prior to constructing the marc notes array. We also remove the use of C4::XSLT for replacing AV values in the MARC fields in preference to using the RecordProcessor filter. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 136793 [details] [review] Bug 30744: Use RecordProcessor in get_marc_notes This patch utilises RecordProcessor to filter the MARC::Record for the right interface prior to constructing the marc notes array. We also remove the use of C4::XSLT for replacing AV values in the MARC fields in preference to using the RecordProcessor filter. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master for 22.11. Nice work everyone, thanks!
@RMAints please backport this bugs. If they don't apply we'll provide version-specific patches. But please try ASAP.
Backported to 22.05.x for 22.05.04
applied to 21.11.x for 21.11.11
Can't backport a dependency to 21.05.x so can't backport this. Send help to Bug 30918 if this one (30744) is really needed. (In reply to Martin Renvoize from comment #15) > I can create a backportable version of this easily enough too That's another solution.
Last month to get an opportunity to have this backported to 21.05.x if that is still needed and if someone can make a patch for 21.05.x