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.
Created attachment 7025 [details] [review] proposed patch Displays description for copynumber (staff and OPAC) + corrects a small bug which was generating error messages in logs + corrects indentations
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?
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.
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... ;-)
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.
This works fine on the admin side. On the OPAC side however, it doesn't work when the authorized value is 0.
Created attachment 7026 [details] [review] proposed patch #2 Corrected. Works on Staff/OPAC even if value is 0.
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.
Created attachment 7029 [details] [review] proposed patch #3 Handles now the page "moredetail" (items).
Created attachment 7030 [details] [review] [SIGNED-OFF] Works on the OPAC and in the admin, both in normal and item view. Bug 7379: display of copynumber description The biblio/items detail pages display the copynumber description if an authorized value is configured accordingly. Signed-off-by: Gaetan Boisson <gaetan.boisson@biblibre.com>
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"...
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?
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.
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.
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")
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?
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
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 ?
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"..
Created attachment 7977 [details] [review] proposed patch #2 It should apply again. This time the patch is easier to read (no indentation change).
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.
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.
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)
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
Patch pushed, signoff from Gaetan reintroduced
Created attachment 12954 [details] [review] Bug 7379- Allow for new type of LDAP authentication This patch aims to solve the LDAP bind authentication method. Here are some considerations: - This is a standalone patch, so all the previous submitted ones are rendered obsolete; - LDAP bind authentication is now done in 3 steps: 1 - LDAP anonymous bind; 2 - LDAP search entry for the given username; 3 - LDAP bind with the DN of the found entry + the given password. - The process fails if none or more than 1 entries are found for the given username; - The <principal_name> setting in koha-conf.xml isn't used anymore; - The patch is backwards compatible, so users already using the previously implemented LDAP bind authentication should be able to use it the same.
Hi Rolando, thank you for your patch, but I think you might have had a typo in your bug number - could you check?
Comment on attachment 12954 [details] [review] Bug 7379- Allow for new type of LDAP authentication Wrong bug number, sorry for the mix up.
Pushed to 3.8.x will be in 3.8.12