Bug 10676 - OpacHiddenItems not working for restricted on OPAC detail
Summary: OpacHiddenItems not working for restricted on OPAC detail
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Target Milestone: ---
Assignee: Katrin Fischer
QA Contact: Martin Renvoize
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-02 08:14 UTC by Katrin Fischer
Modified: 2019-05-27 12:25 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10676: Fix behaviour of OpacHiddenItems for items.restricted (5.27 KB, patch)
2019-01-04 10:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10676: Add unit tests - GetItemsInfo (2.67 KB, patch)
2019-01-04 11:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10676: Fix behaviour of OpacHiddenItems for items.restricted (5.37 KB, patch)
2019-01-04 21:59 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 10676: Add unit tests - GetItemsInfo (2.79 KB, patch)
2019-01-04 21:59 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 10676: Fix behaviour of OpacHiddenItems for items.restricted (5.32 KB, patch)
2019-01-04 22:04 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 10676: Add unit tests - GetItemsInfo (2.73 KB, patch)
2019-01-04 22:04 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 10676: Fix behaviour of OpacHiddenItems for items.restricted (5.38 KB, patch)
2019-02-14 15:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 10676: Add unit tests - GetItemsInfo (2.81 KB, patch)
2019-02-14 15:16 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2013-08-02 08:14:30 UTC

    
Comment 1 Katrin Fischer 2013-08-02 08:23:11 UTC
For this configuration in OpacHiddenItems wthdrawn and notforloan work, but restricted does not:

notforloan: [2]
wthdrawn: [2]
restricted: [2]

Looking at the code I didn't see a reason for this. If someone could test and confirm/invalidate this bug it would be appreciated.
Comment 2 Katrin Fischer 2015-06-07 09:51:47 UTC
Still puzzling, still valid. Nothing helpful in the logs.
Comment 3 Joy Nelson 2016-06-15 17:44:42 UTC
I can verify this.  on 3.22.07 opachiddenitems set with restricted: [1] does not hide items 


(In reply to Katrin Fischer from comment #2)
> Still puzzling, still valid. Nothing helpful in the logs.
Comment 4 Katrin Fischer 2019-01-03 08:28:47 UTC
Tested on 17.11 again - still not working. Looking at the code I don't see a reason for it not working. lost and damaged are also numerical and work without issues, but restricted does not.

It doesn't appear to be a reserved word in SQL... out of ideas.
Comment 5 Joy Nelson 2019-01-03 17:31:49 UTC
Katrin,
I've just tested this on 18.06.00.028 and it is working.  Restricted items are being hidden in the OPAC when a value of restricted: [1] is placed in the OPACHiddenItems syspref.




(In reply to Katrin Fischer from comment #4)
> Tested on 17.11 again - still not working. Looking at the code I don't see a
> reason for it not working. lost and damaged are also numerical and work
> without issues, but restricted does not.
> 
> It doesn't appear to be a reserved word in SQL... out of ideas.
Comment 6 Katrin Fischer 2019-01-03 18:10:43 UTC
Thank you, Joy, that's good news!
Comment 7 Katrin Fischer 2019-01-04 09:01:26 UTC
Hi Joy,

I tested again today on master - it still doesn't work for me.

Here is what I did:

OpacHiddenItems:
restricted: [1]

Changed 1 item out of 4 to be restricted.

Opac now shows it as: 
Available (Restricted Access)

Not hidden...
Comment 8 Katrin Fischer 2019-01-04 09:50:43 UTC
After some debugging I think I have found the reason. It won't work with the numeric value of the authorised value used, it needs the description:

restricted: ['Restricted Access']

For other values like Damaged it will works with numerics. 

The reason is some code in GetItemsInfo:

        # get notforloan complete status if applicable
        $descriptions = Koha::AuthorisedValues->get_description_by_koha_field({frameworkcode => $data->{frameworkcode}, kohafield => 'items.notforloan', authorised_value => $data->{itemnotforloan} });
        $data->{notforloanvalue}     = $descriptions->{lib} // '';
        $data->{notforloanvalueopac} = $descriptions->{opac_description} // '';

        # get restricted status and description if applicable
        $descriptions = Koha::AuthorisedValues->get_description_by_koha_field({frameworkcode => $data->{frameworkcode}, kohafield => 'items.restricted', authorised_value => $data->{restricted} });
        $data->{restricted}     = $descriptions->{lib} // '';
        $data->{restrictedopac} = $descriptions->{opac_description} // '';

While it uses a new variable "*value*" for notforloan, it overwrites the restricted numerical value with the description.

I think the clean solution is to change it so that OpacHiddenItems works like for the other item fields with authorised values, using the codes.
Comment 9 Katrin Fischer 2019-01-04 10:15:54 UTC
Created attachment 83630 [details] [review]
Bug 10676: Fix behaviour of OpacHiddenItems for items.restricted

The numerical restricted value was overwritten in GetItemsInfo
with the description. So OpacHiddenItems would only work for
restricted using the description, not the numerical value
of the authorised value like for other similar status fields.

This changes GetItemsInfo to use a separate variable for
the description, as it's already done for notforloan and
changes the template files where the variable is used
accordingly.

To test
- Before applying patch
- Configure OpacHiddenItems with:
  restricted: [1]
- Find a record with more than one item and mark one of the items
  as "Restricted Access"
  (assume standard sample data - check authorised value if status doesn't exist)
- Verify the item is still shown in OPAC.
- Apply patch
- Verify that the item is now hidden
- Verify that the status still displays nicely in the staff detail page
- Delete OpacHiddenItems
- Verify that the status is showing nicely in the OPAC detail page
Comment 10 Katrin Fischer 2019-01-04 11:43:09 UTC
Created attachment 83631 [details] [review]
Bug 10676: Add unit tests - GetItemsInfo

To test:
  prove t/db_dependent/Items.t
Comment 11 Pierre-Marc Thibault 2019-01-04 20:10:16 UTC
All seems allright, except the last step where the item doesn't show up when OpacHiddenItems is empty in OPAC. Can you please verify if it's the same for you?
Comment 12 Joy Nelson 2019-01-04 20:18:08 UTC
My testing of the original issue (on 18.06) could not replicate it.  I was using the code for the Restricted value, not the description.  The items were hidden on the opac when Restricted: [1] was in the opachiddenitems.
Comment 13 Katrin Fischer 2019-01-04 20:25:40 UTC
Hi Joy, is it possible you used a framework that was not linked to the authorised value correctly? Looking at the code it can't work with the number, unless the number is in the description or the authorised value description cannot be found.
Comment 14 Katrin Fischer 2019-01-04 20:28:24 UTC
(In reply to Pierre-Marc Thibault from comment #11)
> All seems allright, except the last step where the item doesn't show up when
> OpacHiddenItems is empty in OPAC. Can you please verify if it's the same for
> you?

Yes it did work - when the pref is empty and read correctly, there is no more reason to hide it for Koha.

Can you please check that the item didn't fit another criteria for being hidden? 
Is your system preference OpacHiddenItems really empty?
Is your system set up with memcached and Plack?
Comment 15 Joy Nelson 2019-01-04 20:37:21 UTC
My framework is the Default framework.  952$5 is tied to RESTRICTED authorized value list.  The RESTRICTED auth value list has code of 1 and description of "Restricted Access".

OpacHiddenItems contains (with a blank line at end)
itype: [GAMES,EBOOK]
ccode: [REFERENCE,MUSIC]
location: [HOMESCHOOL,PARENTING]
restricted: [1]

My item was none of the values listed below and appeared in OPAC searches.  When i edit the item to RESTRICTED (items.restricted=1), it dissappears from searches.  Although number of bibs still show the count of 5, where search results only show 4.  

(In reply to Katrin Fischer from comment #13)
> Hi Joy, is it possible you used a framework that was not linked to the
> authorised value correctly? Looking at the code it can't work with the
> number, unless the number is in the description or the authorised value
> description cannot be found.
Comment 16 Joy Nelson 2019-01-04 20:43:04 UTC
Testing with Nick shows that this is dependent on the number of items.  My test were for a bib with 1 item and that item was restricted.  It does hide the title from the OPAC because the one item is 'hidden'.

However when another item is added, the hidden item does not appear in search results (as available), but does appear in the detail page when the title is clicked on.

(In reply to Joy Nelson from comment #15)
> My framework is the Default framework.  952$5 is tied to RESTRICTED
> authorized value list.  The RESTRICTED auth value list has code of 1 and
> description of "Restricted Access".
> 
> OpacHiddenItems contains (with a blank line at end)
> itype: [GAMES,EBOOK]
> ccode: [REFERENCE,MUSIC]
> location: [HOMESCHOOL,PARENTING]
> restricted: [1]
> 
> My item was none of the values listed below and appeared in OPAC searches. 
> When i edit the item to RESTRICTED (items.restricted=1), it dissappears from
> searches.  Although number of bibs still show the count of 5, where search
> results only show 4.  
> 
> (In reply to Katrin Fischer from comment #13)
> > Hi Joy, is it possible you used a framework that was not linked to the
> > authorised value correctly? Looking at the code it can't work with the
> > number, unless the number is in the description or the authorised value
> > description cannot be found.
Comment 17 Pierre-Marc Thibault 2019-01-04 20:47:04 UTC
(In reply to Katrin Fischer from comment #14)
> (In reply to Pierre-Marc Thibault from comment #11)
> > All seems allright, except the last step where the item doesn't show up when
> > OpacHiddenItems is empty in OPAC. Can you please verify if it's the same for
> > you?
> 
> Yes it did work - when the pref is empty and read correctly, there is no
> more reason to hide it for Koha.
> 
> Can you please check that the item didn't fit another criteria for being
> hidden? 
> Is your system preference OpacHiddenItems really empty?
> Is your system set up with memcached and Plack?

Sorry, it was likely a mistake on the refresh of the content.
Comment 18 Katrin Fischer 2019-01-04 21:30:20 UTC
Hi Joy, thx for checking again! I'd need to investigate a bit more, but it's possible the code for hiding the record is using a slightly different logic. Koha is always fun ;)

Pierre, is it working ok now for you?
Comment 19 Pierre-Marc Thibault 2019-01-04 21:34:18 UTC
I didn't complete the test since there is a current conversation involving Joy which let me think there is a problem that is not in the test plan and that would make my sign off meaningless. I am wrong?
Comment 20 Katrin Fischer 2019-01-04 21:45:45 UTC
If I understood correctly, you can still sign off. This deals with the case, that not all items on a record are hidden and the OPAC detail pages.

You could extend the test plan a bit:
- Verify that if the restricted item is the only item, the record cannot be found in the OPAC
- Verfy you can't access the record directly by URL
- Verify the item information in the results is correct

From what Joy said those already work, but should not be broken by the patch.
Comment 21 Pierre-Marc Thibault 2019-01-04 21:59:16 UTC Comment hidden (obsolete)
Comment 22 Pierre-Marc Thibault 2019-01-04 21:59:19 UTC Comment hidden (obsolete)
Comment 23 Pierre-Marc Thibault 2019-01-04 22:04:54 UTC
Created attachment 83670 [details] [review]
Bug 10676: Fix behaviour of OpacHiddenItems for items.restricted

The numerical restricted value was overwritten in GetItemsInfo
with the description. So OpacHiddenItems would only work for
restricted using the description, not the numerical value
of the authorised value like for other similar status fields.

This changes GetItemsInfo to use a separate variable for
the description, as it's already done for notforloan and
changes the template files where the variable is used
accordingly.

To test
- Before applying patch
- Configure OpacHiddenItems with:
  restricted: [1]
- Find a record with more than one item and mark one of the items
  as "Restricted Access"
  (assume standard sample data - check authorised value if status doesn't exist)
- Verify the item is still shown in OPAC.
- Apply patch
- Verify that the item is now hidden
- Verify that the status still displays nicely in the staff detail page
- Delete OpacHiddenItems
- Verify that the status is showing nicely in the OPAC detail page

Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com>
Comment 24 Pierre-Marc Thibault 2019-01-04 22:04:57 UTC
Created attachment 83671 [details] [review]
Bug 10676: Add unit tests - GetItemsInfo

To test:
  prove t/db_dependent/Items.t

Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com>
Comment 25 Pierre-Marc Thibault 2019-01-04 22:05:48 UTC Comment hidden (obsolete)
Comment 26 Pierre-Marc Thibault 2019-01-04 22:11:06 UTC
Your test plan works and it doesn't seem there's a problem with the extended steps either. Sorry, for the mistakes I made while editing the page, I will try to be more focus next time. :)
Comment 27 Martin Renvoize 2019-02-14 15:16:15 UTC
Created attachment 85122 [details] [review]
Bug 10676: Fix behaviour of OpacHiddenItems for items.restricted

The numerical restricted value was overwritten in GetItemsInfo
with the description. So OpacHiddenItems would only work for
restricted using the description, not the numerical value
of the authorised value like for other similar status fields.

This changes GetItemsInfo to use a separate variable for
the description, as it's already done for notforloan and
changes the template files where the variable is used
accordingly.

To test
- Before applying patch
- Configure OpacHiddenItems with:
  restricted: [1]
- Find a record with more than one item and mark one of the items
  as "Restricted Access"
  (assume standard sample data - check authorised value if status doesn't exist)
- Verify the item is still shown in OPAC.
- Apply patch
- Verify that the item is now hidden
- Verify that the status still displays nicely in the staff detail page
- Delete OpacHiddenItems
- Verify that the status is showing nicely in the OPAC detail page

Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Martin Renvoize 2019-02-14 15:16:18 UTC
Created attachment 85123 [details] [review]
Bug 10676: Add unit tests - GetItemsInfo

To test:
  prove t/db_dependent/Items.t

Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize 2019-02-14 15:16:55 UTC
Well done for getting to the bottom of this challenging one Katrin.

All works for me, no regressions found and the QA scripts all pass.. Passing QA
Comment 30 Nick Clemens 2019-02-15 18:45:37 UTC
Awesome work all!

Pushed to master for 19.05
Comment 31 Martin Renvoize 2019-02-25 13:39:57 UTC
Pushed to 18.11.x for 18.11.04
Comment 32 Lucas Gass 2019-03-05 15:59:57 UTC
backported to 18.05.x for 18.05.11