Bug 7515 - authorized value code showing in opac for public note
Summary: authorized value code showing in opac for public note
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Owen Leonard
QA Contact: Testopia
URL:
Keywords:
Depends on: 7525 7379
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-09 13:24 UTC by Nicole C. Engard
Modified: 2019-02-20 19:15 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7515 - authorized value code showing in opac for public note (1.73 KB, patch)
2012-02-14 18:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7515 - authorized value code showing in opac for public note (1.72 KB, patch)
2012-02-14 19:11 UTC, Nicole C. Engard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7515 - authorized value code showing in opac for public note (1.80 KB, patch)
2012-02-14 19:12 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 7515 - authorized value code showing in opac for public note (5.41 KB, patch)
2012-02-16 17:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7515 - authorized value code showing in opac for public note (4.76 KB, patch)
2012-02-23 14:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7515 - authorized value code showing in opac for public note (4.83 KB, patch)
2012-02-26 21:05 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 7515 - authorized value code showing in opac for public note (4.75 KB, patch)
2012-02-28 14:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7515 - authorized value code showing in opac for public note (4.96 KB, patch)
2012-02-28 15:08 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 7515 - Followup - Only set itemnotes to authorised value if said value exists. (1.05 KB, patch)
2012-03-12 13:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7515 - Followup - Only set itemnotes to authorised value if said value exists. (1.06 KB, patch)
2012-03-15 15:13 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2012-02-09 13:24:18 UTC
See: http://screencast.com/t/PoGPO0PB

If you have an authorized value for the public notes field on the item record it shows the description when editing items, but when you view the normal view in the opac and the staff client it shows the authorized value code and that means nothing to the public - it should show the description for authorized values.
Comment 1 Kyle M Hall 2012-02-14 18:12:09 UTC Comment hidden (obsolete)
Comment 2 Nicole C. Engard 2012-02-14 19:11:34 UTC Comment hidden (obsolete)
Comment 3 Nicole C. Engard 2012-02-14 19:12:00 UTC Comment hidden (obsolete)
Comment 4 Ian Walls 2012-02-16 16:15:58 UTC
This patch doesn't differentiate between the OPAC description (opaclib) and the staff client description (lib, which is used here).  This means that the public will see the staffside description, which is counter to expectations.  Marking as Failed QA.
Comment 5 Kyle M Hall 2012-02-16 17:53:39 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2012-02-16 17:54:59 UTC
Updated the patch to take into account whether the data is to be used for the intranet or the opac.
Comment 7 Julian Maurice 2012-02-23 10:03:19 UTC
QA comment:

In GetItemsInfo I think that all added code can be replaced by two already existing subs:

    my $sql_itemnotes = "
        SELECT authorised_value
        FROM   marc_subfield_structure
        WHERE  kohafield = 'items.itemnotes'
        AND    frameworkcode = ?
    ";
    my $sth_itemnotes = $dbh->prepare( $sql_itemnotes );
    $sth_itemnotes->execute( $data->{'frameworkcode'} );
    my ($authorised_valuecode) = $sth_itemnotes->fetchrow;

can be replaced by:

    my ($authorised_valuecode) = C4::Koha::GetAuthValCode('items.itemnotes', $data->{frameworkcode});

and

    my $lib = $opac ? 'lib_opac' : 'lib';
    if ($authorised_valuecode) {
        $sth_itemnotes = $dbh->prepare(
            "SELECT $lib FROM authorised_values
             WHERE  category=?
             AND authorised_value=?"
        );
        $sth_itemnotes->execute( $authorised_valuecode, $data->{'itemnotes'} );
        my ($lib) = $sth_itemnotes->fetchrow;
        $data->{'itemnotes'} = $lib;
    }

can be replaced by:

    $data->{itemnotes} = C4::Koha::GetKohaAuthorisedValueLib($authorised_valuecode, $data->{itemnotes}, $opac);

Does anybody agree?
Comment 8 Kyle M Hall 2012-02-23 14:47:37 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2012-02-23 14:48:45 UTC
Updated patch to take into account the suggestions by Julian Maurice.
Comment 10 Nicole C. Engard 2012-02-26 21:05:15 UTC Comment hidden (obsolete)
Comment 11 Paul Poulain 2012-02-27 17:13:38 UTC
QA comment:
* my pre-apply hook complains:
"my" variable $authorised_valuecode masks earlier declaration in same scope at /home/paul/koha.dev/koha-community//C4/Items.pm line 1264.


Looking at the code, you're adding
+        my ($authorised_valuecode) = C4::Koha::GetAuthValCode('items.itemnotes', $data->{frameworkcode});

and $authorised_valuecode is declared a few line before.

Removing the my is probably enough, please fix, test & resubmit.

(the code is fine otherwise)
Comment 12 Kyle M Hall 2012-02-28 14:52:05 UTC Comment hidden (obsolete)
Comment 13 Nicole C. Engard 2012-02-28 15:08:43 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2012-03-09 15:35:14 UTC
QA Comment: 
Filling itemnotes in Items.pm: Shouldn't you test if GetAuthValCode returned something? If it is not an authorized value, you are overwriting it, isn't it?
Comment 15 Kyle M Hall 2012-03-12 13:57:31 UTC Comment hidden (obsolete)
Comment 16 Nicole C. Engard 2012-03-14 16:54:34 UTC
After applying the two patches (was I only supposed to do the followup?) above and doing a search I get :


Software error:

Global symbol "$itemnotes" requires explicit package name at /home/nengard/kohaclone/C4/Items.pm line 1267.
Compilation failed in require at /home/nengard/kohaclone/C4/Reserves.pm line 29.
BEGIN failed--compilation aborted at /home/nengard/kohaclone/C4/Reserves.pm line 29.
Compilation failed in require at /home/nengard/kohaclone/C4/Circulation.pm line 26.
BEGIN failed--compilation aborted at /home/nengard/kohaclone/C4/Circulation.pm line 26.
Compilation failed in require at /home/nengard/kohaclone/C4/Overdues.pm line 26.
BEGIN failed--compilation aborted at /home/nengard/kohaclone/C4/Overdues.pm line 26.
Compilation failed in require at /home/nengard/kohaclone/C4/Members.pm line 30.
BEGIN failed--compilation aborted at /home/nengard/kohaclone/C4/Members.pm line 30.
Compilation failed in require at /home/nengard/kohaclone/C4/Search.pm line 28.
BEGIN failed--compilation aborted at /home/nengard/kohaclone/C4/Search.pm line 28.
Compilation failed in require at /home/nengard/kohaclone/catalogue/search.pl line 146.
BEGIN failed--compilation aborted at /home/nengard/kohaclone/catalogue/search.pl line 146.

For help, please send mail to the webmaster (webmaster@debian), giving this error message and the time and date of the error.
Comment 17 Kyle M Hall 2012-03-15 15:13:55 UTC Comment hidden (obsolete)
Comment 18 delaye 2012-03-30 15:05:41 UTC
i have crete une link between items.itemnotes(995$u : UNIMARc) and authorised_values
I create an item
In the opac the description of the authorised value display

it's work!
Comment 19 Paul Poulain 2012-04-04 15:38:28 UTC
This patch looks to be a duplicate of bug 7379 if i'm not mistaking. I think we should find a generic solution and not add specific code.

So switching to in discussion and also linking to bug 7525
Comment 20 Kyle M Hall 2012-10-03 13:00:10 UTC
(In reply to comment #19)
> This patch looks to be a duplicate of bug 7379 if i'm not mistaking. I think
> we should find a generic solution and not add specific code.
> 
> So switching to in discussion and also linking to bug 7525

This is not a duplicate of bug 7379, but it is a similar situation. That one was pushed, so I don't see a reason why we shouldn't push this one too.

Could we solve this at the template level using Koha::Template::Plugin::KohaAuthorisedValues? I think that would be an elegant solution, but we may need to add a more subroutine to it for various situations.
Comment 21 Kyle M Hall 2013-03-11 18:27:04 UTC
(In reply to comment #19)
> This patch looks to be a duplicate of bug 7379 if i'm not mistaking. I think
> we should find a generic solution and not add specific code.
> 
> So switching to in discussion and also linking to bug 7525

Paul, can you give me some more input on this?

I think there are a few solutions:
1) Solve at the .pl level: i.e. patch as we go along in the perl code ( i.e. this bug's patches )

2) Solve at the .tt level: i.e. use Koha::Template::Plugin::KohaAuthorisedValues and patch as we go along at the template level. We can patch this sub to check for an authorised value mapping and return either the correct mapping or the original passed in data. The will greatly simplify the amount of code necessary to support the use of authorised values mapping.

3) Solve at the .pm level: i.e. all subs that send back data from biblio, biblioitems or items will first pass that data through a new sub ReplaceWithAuthorisedValues that will check each field to see if it is mapped to an authorised value list and replace the code with the data if necessary.

I personally think choice 2 is will be the quickest, lightest weight solution.
Comment 22 Galen Charlton 2013-08-12 17:24:31 UTC
(In reply to Kyle M Hall from comment #21)
> 2) Solve at the .tt level: i.e. use
> Koha::Template::Plugin::KohaAuthorisedValues and patch as we go along at the
> template level. We can patch this sub to check for an authorised value
> mapping and return either the correct mapping or the original passed in
> data. The will greatly simplify the amount of code necessary to support the
> use of authorised values mapping.

I prefer this option.
Comment 23 Owen Leonard 2019-02-20 19:15:53 UTC
(In reply to Galen Charlton from comment #22)
> > 2) Solve at the .tt level: i.e. use
> > Koha::Template::Plugin::KohaAuthorisedValues and patch as we go along at the
> > template level.

> I prefer this option.

This is very easily done, but we don't have a default authorized value category for public note and nonpublic note. Are these defaults we want to add?