Bug 23247 - Use EmbedItems in opac-MARCdetail.pl
Summary: Use EmbedItems in opac-MARCdetail.pl
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 29697 22696 22721 25006
Blocks:
  Show dependency treegraph
 
Reported: 2019-07-01 17:28 UTC by Tomás Cohen Arazi
Modified: 2022-07-22 19:59 UTC (History)
5 users (show)

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


Attachments
Bug 23247: Simplify visibility logic used in opac-MARCdetail.pl (5.12 KB, patch)
2019-07-01 19:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23247: Simplify visibility logic used in opac-MARCdetail.pl (4.57 KB, patch)
2019-07-04 02:22 UTC, Mark Tompsett
Details | Diff | Splinter Review
Hi Tomas. Looks good. Signing this patch off. (4.63 KB, patch)
2019-10-03 08:36 UTC, Jesse Maseto
Details | Diff | Splinter Review
Bug 23247: Simplify visibility logic used in opac-MARCdetail.pl (4.30 KB, patch)
2020-03-30 22:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23247: (QA follow-up) Restore embedding items in record (1.85 KB, patch)
2020-03-30 22:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23247: Use EmbedItems in opac-MARCdetail.pl (2.36 KB, patch)
2022-07-22 19:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2019-07-01 17:28:42 UTC
The logic seems too complex for the covered use cases, and it queries the DB too many times without a real need for that.
Comment 1 Tomás Cohen Arazi 2019-07-01 19:12:19 UTC
Created attachment 91158 [details] [review]
Bug 23247: Simplify visibility logic used in opac-MARCdetail.pl

This patch uses the OO code, with prefetching and all, to make decisions
on visibility. It shouldn't change any behaviour, unless you are
counting DB queries, execution time, etc.

To test:
- Find a known record, go to the OPAC MARC page of it.
- Play with several options in OpacHiddenItems, refreshing it
- Play with OpacHiddenItemsExceptions as well
=> SUCCESS: Things work as expected
- Apply this patch
- Repeate the tests above
=> SUCCESS: Things work as expected!
- Sign off :-D
Comment 2 Mark Tompsett 2019-07-03 03:27:51 UTC
Comment on attachment 91158 [details] [review]
Bug 23247: Simplify visibility logic used in opac-MARCdetail.pl

Review of attachment 91158 [details] [review]:
-----------------------------------------------------------------

::: opac/opac-MARCdetail.pl
@@ +305,4 @@
>      push @item_loop, $item if $item;
>  }
>  my ( $holdingbrtagf, $holdingbrtagsubf ) =
> +  &GetMarcFromKohaField( "items.holdingbranch", $biblio->frameworkcode );

If you are refactoring, while I would prefer to keep framework code in, so as to allow weird and wonderful things in the future (UNIMARC and MARC21 co-existing), the function has already been neutered. So perhaps a refactor on this and other uses of this function here would be in order?
Comment 3 Mark Tompsett 2019-07-03 03:28:35 UTC
Setting to Failed QA, while discussion about cleaning up the other function call is had and/or done.
Comment 4 Katrin Fischer 2019-07-03 07:54:03 UTC
(In reply to M. Tompsett from comment #2)
> Comment on attachment 91158 [details] [review] [review]
> Bug 23247: Simplify visibility logic used in opac-MARCdetail.pl
> 
> Review of attachment 91158 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: opac/opac-MARCdetail.pl
> @@ +305,4 @@
> >      push @item_loop, $item if $item;
> >  }
> >  my ( $holdingbrtagf, $holdingbrtagsubf ) =
> > +  &GetMarcFromKohaField( "items.holdingbranch", $biblio->frameworkcode );
> 
> If you are refactoring, while I would prefer to keep framework code in, so
> as to allow weird and wonderful things in the future (UNIMARC and MARC21
> co-existing), the function has already been neutered. So perhaps a refactor
> on this and other uses of this function here would be in order?

Hi Marc, 

resetting status - this is only a clan-up, the parameter frameworkcode for GetMarcFromKohaField does no longer exist. See bug 22721 for some more info.
Comment 5 Mark Tompsett 2019-07-04 02:22:12 UTC
Created attachment 91253 [details] [review]
Bug 23247: Simplify visibility logic used in opac-MARCdetail.pl

This patch uses the OO code, with prefetching and all, to make decisions
on visibility. It shouldn't change any behaviour, unless you are
counting DB queries, execution time, etc.

To test:
- Find a known record, go to the OPAC MARC page of it.
- Play with several options in OpacHiddenItems, refreshing it
- Play with OpacHiddenItemsExceptions as well
=> SUCCESS: Things work as expected
- Apply this patch
- Repeate the tests above
=> SUCCESS: Things work as expected!
- Sign off :-D
Comment 6 Mark Tompsett 2019-07-04 02:27:38 UTC
I believe I have rebased it correctly to be off bug 22721 as mentioned by Katrin in comment #4. As such, I'm happy to leave it back at Needs Signoff.
Comment 7 Mark Tompsett 2019-07-04 02:29:45 UTC
Apologies, I lost the other Depends on.
Comment 8 Jonathan Druart 2019-09-01 19:57:54 UTC
(In reply to M. Tompsett from comment #6)
> I believe I have rebased it correctly to be off bug 22721 as mentioned by
> Katrin in comment #4. As such, I'm happy to leave it back at Needs Signoff.

Why did not you sign it off?
Comment 9 Jesse Maseto 2019-10-03 08:36:45 UTC
Created attachment 93528 [details] [review]
Hi Tomas. Looks good. Signing this patch off.

JesseM


Bug 23247: Simplify visibility logic used in opac-MARCdetail.pl

This patch uses the OO code, with prefetching and all, to make decisions
on visibility. It shouldn't change any behaviour, unless you are
counting DB queries, execution time, etc.

To test:
- Find a known record, go to the OPAC MARC page of it.
- Play with several options in OpacHiddenItems, refreshing it
- Play with OpacHiddenItemsExceptions as well
=> SUCCESS: Things work as expected
- Apply this patch
- Repeate the tests above
=> SUCCESS: Things work as expected!
- Sign off :-D

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Comment 10 Marcel de Rooy 2019-10-11 09:41:44 UTC
QAing
Comment 11 Marcel de Rooy 2019-10-11 09:52:48 UTC
From Patron/Category:

=head3 override_hidden_items

    if ( $patron->category->override_hidden_items ) {
        ...
    }

Returns a boolean that if patrons of this category are exempt from the OPACHiddenItems policies

TODO: Remove on bug 22547

=> Looks like we did not on 22547 ;)
Comment 12 Marcel de Rooy 2019-10-11 10:06:24 UTC
Does this report obsolete the borcat parameter of GetMarcBiblio btw?
If so, please provide a follow-up to get rid of it?
Comment 13 Marcel de Rooy 2019-10-11 10:09:54 UTC
If I have nothing in OPACHiddenItems or just itype: [CF],
I do not see my two items on a record with itype BK or BKS.
Just: No available items.

Without your patch, they come up again. Nice.

So there is still something wrong ;) Can you find it ?
Comment 14 Tomás Cohen Arazi 2019-11-14 19:33:32 UTC
(In reply to Marcel de Rooy from comment #12)
> Does this report obsolete the borcat parameter of GetMarcBiblio btw?
> If so, please provide a follow-up to get rid of it?

(In reply to Marcel de Rooy from comment #11)
> From Patron/Category:
> 
> =head3 override_hidden_items
> 
>     if ( $patron->category->override_hidden_items ) {
>         ...
>     }
> 
> Returns a boolean that if patrons of this category are exempt from the
> OPACHiddenItems policies
> 
> TODO: Remove on bug 22547
> 
> => Looks like we did not on 22547 ;)

That was a typo, it was meant to be bug 22457
Comment 15 Marcel de Rooy 2019-11-15 06:45:48 UTC
Testing again
Comment 16 Marcel de Rooy 2019-11-15 06:52:20 UTC
Tomas,
See my comment13.
I have a book with three items. One is CF.
Without your patch, normal view and marc view show 2 items. Which is ok.
With your patch, normal view shows still 2 items (of course), but marc view shows no available items. Should be 2.

Failed QA
Comment 17 Tomás Cohen Arazi 2020-03-30 22:21:50 UTC
Created attachment 102094 [details] [review]
Bug 23247: Simplify visibility logic used in opac-MARCdetail.pl

This patch uses the OO code, with prefetching and all, to make decisions
on visibility. It shouldn't change any behaviour, unless you are
counting DB queries, execution time, etc.

To test:
- Find a known record, go to the OPAC MARC page of it.
- Play with several options in OpacHiddenItems, refreshing it
- Play with OpacHiddenItemsExceptions as well
=> SUCCESS: Things work as expected
- Apply this patch
- Repeate the tests above
=> SUCCESS: Things work as expected!
- Sign off :-D

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Comment 18 Tomás Cohen Arazi 2020-03-30 22:21:56 UTC
Created attachment 102095 [details] [review]
Bug 23247: (QA follow-up) Restore embedding items in record

The original code called GetMarcBiblio with embeditems => 1, and I
overlooked it.

This patch makes it embed the items.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Tomás Cohen Arazi 2020-03-30 22:23:46 UTC
(In reply to Marcel de Rooy from comment #12)
> Does this report obsolete the borcat parameter of GetMarcBiblio btw?
> If so, please provide a follow-up to get rid of it?

The plan is to get rid of GetMarcBiblio altogether. Gimme some more time to align things :-D
Comment 20 Tomás Cohen Arazi 2020-03-30 22:24:46 UTC
(In reply to Marcel de Rooy from comment #16)
> Tomas,
> See my comment13.
> I have a book with three items. One is CF.
> Without your patch, normal view and marc view show 2 items. Which is ok.
> With your patch, normal view shows still 2 items (of course), but marc view
> shows no available items. Should be 2.
> 
> Failed QA

Phew, it took me a while to be back here. Please re-test with the follow-up. I rebased the first patch against master (IssuingRules vs CirculationRules)
Comment 21 Jonathan Druart 2020-03-31 09:56:00 UTC
The order of the columns is not identical, is that expected?

Without: https://snipboard.io/uT4lNK.jpg
With:    https://snipboard.io/iVGqpZ.jpg
Comment 22 Tomás Cohen Arazi 2020-03-31 11:55:24 UTC
(In reply to Jonathan Druart from comment #21)
> The order of the columns is not identical, is that expected?
> 
> Without: https://snipboard.io/uT4lNK.jpg
> With:    https://snipboard.io/iVGqpZ.jpg

That's not expected. I didn't notice!
Comment 23 Tomás Cohen Arazi 2022-07-22 19:50:43 UTC
Created attachment 138059 [details] [review]
Bug 23247: Use EmbedItems in opac-MARCdetail.pl

This patch uses the EmbedItems processor to add the items to the MARC
record.

To test:
- Find a known record, go to the OPAC MARC page of it.
- Play with several options in OpacHiddenItems, refreshing it
- Play with OpacHiddenItemsExceptions as well
=> SUCCESS: Things work as expected
- Apply this patch
- Repeate the tests above
=> SUCCESS: Things work as expected!
- Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi 2022-07-22 19:52:16 UTC
Ok, In light of bug 29697 and the design discussion on bug 31224, I decided to rework this bug.
Comment 25 Tomás Cohen Arazi 2022-07-22 19:59:56 UTC
(In reply to Marcel de Rooy from comment #13)
> If I have nothing in OPACHiddenItems or just itype: [CF],
> I do not see my two items on a record with itype BK or BKS.
> Just: No available items.
> 
> Without your patch, they come up again. Nice.
> 
> So there is still something wrong ;) Can you find it ?

I don't manage to reproduce this. It might have been the old YAML library we were using?

I tried

---
itype: ['CF']

and

---
itype: [CF]

Both produced the expected result.