Bug 19261 - Never hide biblionumber field in view policy
Summary: Never hide biblionumber field in view policy
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin Somers
QA Contact: Testopia
URL:
Keywords:
: 25686 (view as bug list)
Depends on: 15870
Blocks:
  Show dependency treegraph
 
Reported: 2017-09-06 10:19 UTC by Fridolin Somers
Modified: 2021-01-05 08:29 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 19261 - never hide biblionumber field in view policy (2.11 KB, patch)
2017-09-06 11:48 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 19261 - never hide biblionumber field in view policy (1.79 KB, patch)
2017-09-07 01:08 UTC, Dilan Johnpullé
Details | Diff | Splinter Review
Bug 19261 - never hide biblionumber field in view policy (1.85 KB, patch)
2017-09-29 12:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19261 - never hide biblionumber field in view policy (1.91 KB, patch)
2020-07-23 04:10 UTC, David Cook
Details | Diff | Splinter Review
Bug 19261: Never hide biblionumber field in view policy (2.71 KB, patch)
2020-07-26 16:13 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2017-09-06 10:19:11 UTC
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.
Comment 1 Fridolin Somers 2017-09-06 11:48:46 UTC
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
Comment 2 Dilan Johnpullé 2017-09-07 01:08:27 UTC
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>
Comment 3 Kyle M Hall 2017-09-29 12:19:22 UTC
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>
Comment 4 Jonathan Druart 2017-09-29 16:02:33 UTC
Please add a test to t/db_dependent/Filter_MARC_ViewPolicy.t
Comment 5 Mark Tompsett 2017-10-18 04:15:15 UTC
(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?
Comment 6 Mark Tompsett 2017-10-18 04:37:27 UTC
Also, I can't reproduce. Should it not have an obvious error after a restart_all on the kohadevbox?
Comment 7 Fridolin Somers 2018-11-07 09:53:40 UTC
Maybe we should better add a warn in about page ?
Comment 8 David Cook 2020-07-23 03:49:10 UTC
*** Bug 25686 has been marked as a duplicate of this bug. ***
Comment 9 David Cook 2020-07-23 03:50:03 UTC
(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
Comment 10 David Cook 2020-07-23 03:50:30 UTC
(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
Comment 11 David Cook 2020-07-23 03:51:34 UTC
(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.
Comment 12 David Cook 2020-07-23 03:59:38 UTC
*** Bug 25686 has been marked as a duplicate of this bug. ***
Comment 13 David Cook 2020-07-23 04:08:53 UTC
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.
Comment 14 David Cook 2020-07-23 04:10:47 UTC
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>
Comment 15 David Cook 2020-07-23 04:14:27 UTC
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!
Comment 16 Katrin Fischer 2020-07-26 16:13:13 UTC
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>
Comment 17 David Cook 2020-07-27 00:53:00 UTC
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".
Comment 18 Katrin Fischer 2020-07-27 05:43:49 UTC
(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.
Comment 19 Tomás Cohen Arazi 2020-07-27 12:24:09 UTC
Taking a look here. This is missing:
- Regression tests
- biblioitems.biblioitemnumber
Comment 20 Katrin Fischer 2020-07-27 12:28:47 UTC
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?
Comment 21 Tomás Cohen Arazi 2020-07-27 15:27:32 UTC
(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.
Comment 22 David Cook 2020-07-28 23:17:26 UTC
(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.
Comment 23 Tomás Cohen Arazi 2020-07-28 23:55:30 UTC
(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
Comment 24 David Cook 2020-07-29 02:51:11 UTC
(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.
Comment 25 Tomás Cohen Arazi 2020-07-29 03:02:33 UTC
(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.
Comment 26 David Cook 2020-07-29 03:09:52 UTC
(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.
Comment 27 Tomás Cohen Arazi 2020-07-29 03:26:10 UTC
(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.
Comment 28 Jonathan Druart 2020-07-29 07:25:00 UTC
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.
Comment 29 David Cook 2020-07-31 01:49:47 UTC
(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".
Comment 30 Jonathan Druart 2020-07-31 07:38:10 UTC
(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
Comment 31 Nick Clemens 2020-08-06 12:45:41 UTC
(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
Comment 32 Katrin Fischer 2020-10-25 02:30:57 UTC
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?
Comment 33 David Cook 2020-10-26 00:06:15 UTC
(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.
Comment 34 Fridolin Somers 2021-01-05 08:29:08 UTC
Isn't Bug 25826 enough ?