Bug 7379 - copynumber is displayed 'as is' when linked to autorised values
Summary: copynumber is displayed 'as is' when linked to autorised values
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: 3.8
Hardware: All All
: P3 normal (vote)
Assignee: Adrien SAURAT
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 7515
  Show dependency treegraph
 
Reported: 2011-12-27 11:00 UTC by Gaetan Boisson
Modified: 2014-05-26 21:04 UTC (History)
6 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
proposed patch (23.46 KB, patch)
2012-01-03 10:31 UTC, Adrien SAURAT
Details | Diff | Splinter Review
proposed patch #2 (24.13 KB, patch)
2012-01-03 13:07 UTC, Adrien SAURAT
Details | Diff | Splinter Review
proposed patch #3 (27.56 KB, patch)
2012-01-03 15:41 UTC, Adrien SAURAT
Details | Diff | Splinter Review
[SIGNED-OFF] (27.70 KB, patch)
2012-01-03 16:07 UTC, Gaetan Boisson
Details | Diff | Splinter Review
proposed patch #2 (5.92 KB, patch)
2012-03-02 10:13 UTC, Adrien SAURAT
Details | Diff | Splinter Review
proposed patch #3 (5.90 KB, patch)
2012-06-09 14:50 UTC, Adrien SAURAT
Details | Diff | Splinter Review
Bug 7379- Allow for new type of LDAP authentication (3.86 KB, patch)
2012-10-19 15:14 UTC, Rolando Isidoro
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Gaetan Boisson 2011-12-27 11:00:33 UTC
When items.copynumber is linked to an autorized value, the value displayed both on OPAC and on the staff interface is the one found in the database and not the description linked to the value.
This should be fixed in order for this field to function like the other ones, where the authorized value's description is displayed when the field is linked to an authorized value.
Comment 1 Adrien SAURAT 2012-01-03 10:31:09 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2012-01-03 10:36:42 UTC
I am not sure about the indentation changes - it makes it hard to read your patch and spot the real changes. 

I think in the past it was said to do things like this in a separate patch (http://wiki.koha-community.org/wiki/Coding_Guidelines#Refactoring_Code)

Not sure how we want to handle this now - perhaps a question for the QA team?
Comment 3 Adrien SAURAT 2012-01-03 10:51:25 UTC
Well, refactoring could take some time and is not necessarily useful. Indenting is quick to do and makes the file more readable (but the patch far less readable indeed).

I'll follow any line, but it needs to be clear, Chris told me a few weeks ago that it was good to enhance the code whenever possible without creating new bugs for this.

======================================

I'll help you read this patch.

I used the same code already used for "ccode", adding this in opac-detail.pl :

my $copynumbers =  GetKohaAuthorisedValues('items.copynumber',$dat->{'frameworkcode'}, 'opac');

...

if ( my $copynumber = $itm->{'copynumber'} ) {
    $itm->{'copynumber'} = $copynumbers->{$copynumber} if ( defined($copynumbers) && exists( $copynumbers->{$copynumber} ) );
}


and this in detail.pl :

my $copynumbers     = GetKohaAuthorisedValues('items.copynumber', $fw);

...

my $copynumber = $item->{'copynumber'};
    $item->{'copynumber'} = $copynumbers->{$copynumber} if ( defined( $copynumber ) && defined($copynumber) && exists( $copynumbers->{$copynumber} ) );

==========================

Next time I'll try to modify only the code surrounding the bug fix.
Comment 4 Magnus Enger 2012-01-03 10:53:56 UTC
I think there is some relevant advice here: http://wiki.koha-community.org/wiki/Perltidy We just need to figure out what a "code block" is... ;-)
Comment 5 Katrin Fischer 2012-01-03 11:00:35 UTC
Magnus: agreed :)

For the record, meeting minutes and decided style:
http://librarypolice.com/koha-meetings/2011/koha.2011-10-05-10.00.html
AGREED: perl-style (Brooke, 12:37:05)

I also discovered the bugzilla 'Diff' view is quite helpful to spot the real changes.
Comment 6 Gaetan Boisson 2012-01-03 11:25:01 UTC
This works fine on the admin side. On the OPAC side however, it doesn't work when the authorized value is 0.
Comment 7 Adrien SAURAT 2012-01-03 13:07:38 UTC Comment hidden (obsolete)
Comment 8 Gaetan Boisson 2012-01-03 13:15:21 UTC
The 0 problem is solved!
I discovered a new one though : on the items tab on the admin interface the informations are displayed in a different layout, this still displays the authorised value and not it's description.
Comment 9 Adrien SAURAT 2012-01-03 15:41:26 UTC Comment hidden (obsolete)
Comment 10 Gaetan Boisson 2012-01-03 16:07:46 UTC Comment hidden (obsolete)
Comment 11 Ian Walls 2012-01-11 02:39:29 UTC
Perhaps I'm missing something, but it does seem to me that copynumber is a field that makes sense to link to an authorised values field.  Unlike location and ccode, which are a discrete set of values, copynumbers are sequential, with a potentially infinite number of values.  Preconfiguring a set of values seems like a very uncommon practice.

If we're going to add support for copynumber authorised values, I think we should add support for ANY item field being linked to authorised values (since that can be changed by any library in the Frameworks)

This is where I'd change the status to "In Discussion"...
Comment 12 Adrien SAURAT 2012-01-11 08:42:49 UTC
Gaetan would know better about the functionnal part. I only know that at least one of our libraries uses copynumber with a set of values.


For information (I guess it was understood this way but prefer to be sure), if there are no authorised values associated to the copynumber, the copynumber is simply displayed. This patch wouldn't force any library to add a new authorised values.

And doing this for copynumber maybe (Gaetan?) makes sense because not so many fields are displayed on the detail page. Which other field would need to get such a link possibility?
Comment 13 Gaetan Boisson 2012-01-11 10:09:55 UTC
Ian is right that it is uncommon practice to use an authorised value for copynumber. The problem has it roots in the fact that we don't have something like 'extended item attributes' like we have for patrons, so we find ourselves doing with the available fields to take care of all the small idiosyncrasies of our libraries.

For now my opinion is that whenever a field is linked to an authorised value, then the label for the value should be displayed and not the value itself.
Comment 14 Katrin Fischer 2012-01-11 10:21:57 UTC
I think a lot of people reuse fields and reusing copynumber should be relatively safe. As we have the option to link an authorised value category to each field in configuration, it should work for all of them or only offer the option for those where it works. A general solution would be nice - but not sure how hard it is.
Comment 15 Adrien SAURAT 2012-01-11 14:46:29 UTC
There is no way to dynamically implement a general solution which would display labels wherever we expect them.
I had to specifically add this for copynumber in the detail pages.

Doing this for every field in every potential page would require a exhaustive listing of all cases.
Which fields would need this? ("all of them" seems not so sure to me, see "Last seen" and "Barcode")
Comment 16 Marcel de Rooy 2012-01-18 13:07:28 UTC
It seems to me (from the above comments) that this report is a typical candidate for being parked at In Discussion for this time. Perhaps the author could mail the list and ask for more input?
Comment 17 Gaetan Boisson 2012-02-10 15:12:23 UTC
In order to make the discussion move forward, i am renaming this bug to something more specific (the submitted patch has already been tested and signed off), and creating another bug ticket where we should see whether we want to generalize this behaviour to other fields and how.

See :

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7525
Comment 18 Paul Poulain 2012-02-20 21:32:35 UTC
I tend to agree with Gaetan's opinion : let's push this patch, and discuss a generic option.
Won't force the decision though. Marcel, what's your ?
Comment 19 Marcel de Rooy 2012-03-01 11:25:50 UTC
Adrien:
Patch has merge conflicts now in opac-detail and detail.pl
The conflict in opac-detail is the most interesting, mainly because of this (new) call:
     my ($reserve_status) = C4::Reserves::CheckReserves($itm->{itemnumber});

I started resolving, but I think it is better to leave that up to you in this case. When you have tested it again, I can continue QA "unbiased"..
Comment 20 Adrien SAURAT 2012-03-02 10:13:14 UTC Comment hidden (obsolete)
Comment 21 Marcel de Rooy 2012-03-02 10:27:33 UTC
I am setting this status back to Signed off (which it was before starting QA).
Will pick this up soon if no other QA team member beats me to it.
Comment 22 Marcel de Rooy 2012-03-05 09:52:06 UTC
QA Comment:
A general discussion about this kind of change can be held elsewhere indeed. I do not know if many will put auth values on copynumber, but I do not object to this specific change.

catalogue/detail and opac-detail look good.
moredetail: there is no check here like in detail; if the value has no entry in the auth value hash, you display nothing while detail and opac-detail show the original value.
Please add the check there too for getting this patch in the next stage.
Comment 23 Adrien SAURAT 2012-06-09 14:50:26 UTC
Created attachment 10036 [details] [review]
proposed patch #3

New patch, with default value appearing in "moredetail" even if the AV does not exist.

(to test this specific behaviour : select a value then delete the AV associated to it)
Comment 24 Marcel de Rooy 2012-06-21 11:14:23 UTC
The adjusted patch lost the signoff in the commit message, but did not really change. A previous QA comment has been applied.
Looks good to me.
Passed QA
Comment 25 Paul Poulain 2012-06-25 14:40:05 UTC
Patch pushed, signoff from Gaetan reintroduced
Comment 26 Rolando Isidoro 2012-10-19 15:14:54 UTC Comment hidden (obsolete)
Comment 27 Katrin Fischer 2012-10-19 15:18:45 UTC
Hi Rolando,
thank you for your patch, but I think you might have had a typo in your bug number - could you check?
Comment 28 Rolando Isidoro 2012-10-19 15:19:52 UTC
Comment on attachment 12954 [details] [review]
Bug 7379- Allow for new type of LDAP authentication

Wrong bug number, sorry for the mix up.
Comment 29 Chris Cormack 2013-04-21 00:49:10 UTC
Pushed to 3.8.x will be in 3.8.12