Since Bug 15870 fields defined as hidden in frameworks are not present in MARC. Some libraries may have/want field containing biblionumber hidden. This generates an error in OPAC detail view. I propose that field linked to biblionumber is not removed from MARC even if hidden.
Created attachment 66870 [details] [review] Bug 19261 - never hide biblionumber field in view policy Since Bug 15870 fields defined as hidden in frameworks are not present in MARC. Some libraries may have/want field containing biblionumber hidden. This generates an error in OPAC detail view. I propose that field linked to biblionumber is not removed from MARC even if hidden. Test plan : - Use XSLT for OPAC and intranet - Create a biblio record using default framework - Go to delail page in OPAC and intranet - Edit in default framework - In (sub)field linked to biblionumber, go to Advanced constraints - Uncheck OPAC and intranet in visibility and save => Check delail page in OPAC and intranet
Created attachment 66907 [details] [review] Bug 19261 - never hide biblionumber field in view policy Followed test plan and biblionumber subfield 999c is visible in OPAC irrespective of visibility now. Patch applies and works as expected. Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>
Created attachment 67444 [details] [review] Bug 19261 - never hide biblionumber field in view policy Followed test plan and biblionumber subfield 999c is visible in OPAC irrespective of visibility now. Patch applies and works as expected. Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Please add a test to t/db_dependent/Filter_MARC_ViewPolicy.t
(In reply to Fridolin SOMERS from comment #0) > Some libraries may have/want field containing biblionumber hidden. > This generates an error in OPAC detail view. > I propose that field linked to biblionumber is not removed from MARC even if > hidden. Then shouldn't we fix the OPAC detail view rather than tweak the filter?
Also, I can't reproduce. Should it not have an obvious error after a restart_all on the kohadevbox?
Maybe we should better add a warn in about page ?
*** Bug 25686 has been marked as a duplicate of this bug. ***
(In reply to Fridolin SOMERS from comment #7) > Maybe we should better add a warn in about page ? Jonathan has a patch for that at Bug 25826
(In reply to M. Tompsett from comment #6) > Also, I can't reproduce. Should it not have an obvious error after a > restart_all on the kohadevbox? This is reproducible by Bug 25826 and Bug 25686
(In reply to M. Tompsett from comment #5) > Then shouldn't we fix the OPAC detail view rather than tweak the filter? No, because it's not just the OPAC detail view. It's anywhere in Koha that relies on biblionumber in the MARC record, which was previously taken for granted.
Ok I just reproduced this in master on koha-testing-docker after hiding 999$c in Default and Books* frameworks, and then doing this staff interface search: local-number:29 or local-number:30 It created search results with the following links: http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=&searchid=scs_1595477248050 http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=&searchid=scs_1595477248050 As you can see, the biblionumber is missing.
Created attachment 107212 [details] [review] Bug 19261 - never hide biblionumber field in view policy Followed test plan and biblionumber subfield 999c is visible in OPAC irrespective of visibility now. Patch applies and works as expected. Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: David Cook <dcook@prosentient.com.au>
I can confirm this patch works using the following test plan: Test plan: 0a) Use koha-testing-docker 0b) Do not apply patch 1) Hide 999$c in "Books, Booklets, Workbooks" framework 2) Search for "local-number:29 or local-number:30) 3) Note the search results include links like the following: http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=&searchid=scs_1595477248050 http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=&searchid=scs_1595477248050 4) Apply the patch 5) restart_all 6) Search for "local-number:29 or local-number:30) 7) Note the search results include links like the following: http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=29&searchid=scs_1595477625746 http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=30&searchid=scs_1595477625746 Cheers, Frido for writing the original patch almost 3 years ago! It's still good!
Created attachment 107375 [details] [review] Bug 19261: Never hide biblionumber field in view policy Followed test plan and biblionumber subfield 999c is visible in OPAC irrespective of visibility now. Patch applies and works as expected. Test plan: 0a) Use koha-testing-docker 0b) Do not apply patch 1) Hide 999$c in "Books, Booklets, Workbooks" framework 2) Search for "local-number:29 or local-number:30) 3) Note the search results include links like the following: http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=&searchid=scs_1595477248050 http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=&searchid=scs_1595477248050 4) Apply the patch 5) restart_all 6) Search for "local-number:29 or local-number:30) 7) Note the search results include links like the following: http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=29&searchid=scs_1595477625746 http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=30&searchid=scs_1595477625746 Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
I think Martin said that Jonathan might hold off on pushing this one, as Tomas is working on a patch to expand the list of fields to not hide beyond just "biblio.biblionumber".
(In reply to David Cook from comment #17) > I think Martin said that Jonathan might hold off on pushing this one, as > Tomas is working on a patch to expand the list of fields to not hide beyond > just "biblio.biblionumber". What other fields would there be? biblionumber is the only thing I can come up with that would have a negative effect on functionality.
Taking a look here. This is missing: - Regression tests - biblioitems.biblioitemnumber
I don't think we use the biblioitemnumber anywhere for links or similar, but it might be a little safer. Should we mark this bug duplicate with bug 25826?
(In reply to Katrin Fischer from comment #20) > I don't think we use the biblioitemnumber anywhere for links or similar, but > it might be a little safer. > > Should we mark this bug duplicate with bug 25826? I'm not sure. It feels like bug 25826 is the way to go.
(In reply to Tomás Cohen Arazi from comment #21) > I'm not sure. It feels like bug 25826 is the way to go. I don't think Bug 25826 will work, as the patch would only affect manual edits via the web UI. It doesn't take into account existing installations, imported frameworks, hacky SQL updates, etc.
(In reply to David Cook from comment #22) > (In reply to Tomás Cohen Arazi from comment #21) > > I'm not sure. It feels like bug 25826 is the way to go. > > I don't think Bug 25826 will work, as the patch would only affect manual > edits via the web UI. It doesn't take into account existing installations, > imported frameworks, hacky SQL updates, etc. What Jonathan and I discussed was to give proper feedback on about.pl about this problem, and have the frameworks form forbid changing this values, unless we are fixing them. I understand your concern, but conclusion was we would keep carrying this error
(In reply to Tomás Cohen Arazi from comment #23) > What Jonathan and I discussed was to give proper feedback on about.pl about > this problem, and have the frameworks form forbid changing this values, > unless we are fixing them. > > I understand your concern, but conclusion was we would keep carrying this > error Ahh, I think that I understand what you're saying. That's an interesting perspective. That idea does have some merit. Instead of creating an exception in ViewPolicy, you'd be relying on an audit to raise a warning, and then prevent edits based on that warning. In that case, I think we'd need to fail a framework import if it hides the biblionumber too.
(In reply to David Cook from comment #24) > (In reply to Tomás Cohen Arazi from comment #23) > > What Jonathan and I discussed was to give proper feedback on about.pl about > > this problem, and have the frameworks form forbid changing this values, > > unless we are fixing them. > > > > I understand your concern, but conclusion was we would keep carrying this > > error > > Ahh, I think that I understand what you're saying. That's an interesting > perspective. That idea does have some merit. > > Instead of creating an exception in ViewPolicy, you'd be relying on an audit > to raise a warning, and then prevent edits based on that warning. > > In that case, I think we'd need to fail a framework import if it hides the > biblionumber too. We should at least warn. You might want to import, run the frameworks tests and fix stuff, but find some value on that de you got an export of. That can be done in a follow-up bug, though. I feel like this could even be fixed with a button in the frameworks tests report (i.e. if there is a making, but hidden, have a way to fix it). But the other bug actually provides a path for stable so people don't chase ghosts, while we improve error reporting.
(In reply to Tomás Cohen Arazi from comment #25) > We should at least warn. Agreed > That can be done in a follow-up bug, though. I feel like this could even be > fixed with a button in the frameworks tests report (i.e. if there is a > making, but hidden, have a way to fix it). > > But the other bug actually provides a path for stable so people don't chase > ghosts, while we improve error reporting. Yeah I don't think it has to be an either/or thing. I think the other bug adding the warning to about.pl and preventing edits is useful. Just not complete. Personally, I like a solution that doesn't require people to think, which is why I like the patch here on Bug 19261.
(In reply to David Cook from comment #26) > (In reply to Tomás Cohen Arazi from comment #25) > > We should at least warn. > > Agreed > > > That can be done in a follow-up bug, though. I feel like this could even be > > fixed with a button in the frameworks tests report (i.e. if there is a > > making, but hidden, have a way to fix it). > > > > But the other bug actually provides a path for stable so people don't chase > > ghosts, while we improve error reporting. > > Yeah I don't think it has to be an either/or thing. I think the other bug > adding the warning to about.pl and preventing edits is useful. Just not > complete. > > Personally, I like a solution that doesn't require people to think, which is > why I like the patch here on Bug 19261. I would fix the problematic (existing) frameworks here. And pair this with bug 25686, for now. Maybe provide an easy fix on the frameworks tests page.
I am against the idea of displaying "something" that has been configured to be hidden. ViewPolicy must do what we ask it to do, I don't think it's a good idea to start adding exceptions. Preventing to hide the biblionumber from the UI and adding the warning will certainly answer the issue.
(In reply to Jonathan Druart from comment #28) > I am against the idea of displaying "something" that has been configured to > be hidden. ViewPolicy must do what we ask it to do, I don't think it's a > good idea to start adding exceptions. Oh... that's a good point. > Preventing to hide the biblionumber from the UI and adding the warning will > certainly answer the issue. I'm not sure I understand what you mean by "Preventing to hide the biblionumber from the UI".
(In reply to David Cook from comment #29) > I'm not sure I understand what you mean by "Preventing to hide the > biblionumber from the UI". What Tomas implemented on https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=107438
(In reply to Jonathan Druart from comment #28) > I am against the idea of displaying "something" that has been configured to > be hidden. ViewPolicy must do what we ask it to do, I don't think it's a > good idea to start adding exceptions. While I agree, we don't really hide the biblionumber if this is set - the links for 'Place hold | Save to lists' include the biblionumber even if hidden from the record link. The checkbox on the result also has the biblionumber
We still get reports of this issue in IRC and on the mailing list (both this week). Should we revive this patch and add it? I think we had applied on doing both - fixing the GUI and fix the behaviour?
(In reply to Katrin Fischer from comment #32) > We still get reports of this issue in IRC and on the mailing list (both this > week). Should we revive this patch and add it? I think we had applied on > doing both - fixing the GUI and fix the behaviour? Yeah I saw that. I think something needs to be done.
Isn't Bug 25826 enough ?
(In reply to Fridolin Somers from comment #34) > Isn't Bug 25826 enough ? We haven't seen this issue in a while. I guess it was enough.