Description
Tomás Cohen Arazi (tcohen)
2019-07-01 17:28:42 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 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? Setting to Failed QA, while discussion about cleaning up the other function call is had and/or done. (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. 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 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. Apologies, I lost the other Depends on. (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? 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> QAing 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 ;) Does this report obsolete the borcat parameter of GetMarcBiblio btw? If so, please provide a follow-up to get rid of it? 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 ? (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 Testing again 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 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> 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> (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 (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) The order of the columns is not identical, is that expected? Without: https://snipboard.io/uT4lNK.jpg With: https://snipboard.io/iVGqpZ.jpg (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! 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> (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. Created attachment 144689 [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 Seems to work as advertised. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: LMSCloudPaulD <paul.derscheid@lmscloud.de> Created attachment 146813 [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 Seems to work as advertised. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: LMSCloudPaulD <paul.derscheid@lmscloud.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Pushed to master for 23.05. Nice work everyone, thanks! Many hands makes light work, thankyou everyone! Pushed to 22.11.x for the next release Missing dependencies for 22.05.x, no backport. |