Bug 26323

Summary: Not_for_loan, damaged, location and ccode values must be retrieved from the correct AV category
Product: Koha Reporter: Didier Gautheron <didier.gautheron>
Component: CirculationAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart, kyle.m.hall, lucas, m.de.rooy, marjorie.barry-vila, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26424
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21883
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18170
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23912
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00
Bug Depends on:    
Bug Blocks: 21883, 22352, 26819, 27038    
Attachments: Bug 26323: Retrieve the correct NOT_LOAN value
Bug 26323: Retrieve the correct values for LOC, DAMAGED, LOC and CCODE
Bug 26323: Retrieve the correct values for LOST, DAMAGED, LOC and CCODE
Bug 26323: Retrieve the correct NOT_LOAN value
Bug 26323: Retrieve the correct values for LOST, DAMAGED, LOC and CCODE
Bug 26323: (follow-up) Add new cases introduced
Bug 26323: Retrieve the correct NOT_LOAN value
Bug 26323: Retrieve the correct values for LOST, DAMAGED, LOC and CCODE
Bug 26323: (follow-up) Add new cases introduced
Bug 26323: (QA follow-up) Fix syntax errors

Description Didier Gautheron 2020-08-31 10:40:17 UTC
Templates:

intranet-tmpl/prog/en/modules/circ/circulation_batch_checkouts.tt
intranet-tmpl/prog/en/modules/circ/returns.tt
intranet-tmpl/prog/en/modules/reserve/request.tt

are assuming items.notforloan authorized values category is NOT_LOAN.
It's not always true for UNIMARC.
cf.:
koha-tmpl/intranet-tmpl/prog/en/modules/admin/authorised_values.tt

they should use items.notforloan mapping
Comment 1 Jonathan Druart 2020-09-03 14:00:34 UTC
Looks like this is also needed for LOC, DAMAGED, LOC and CCODE.
Comment 2 Jonathan Druart 2020-09-03 14:09:18 UTC
Created attachment 109627 [details] [review]
Bug 26323: Retrieve the correct NOT_LOAN value

From the template we are assuming that items.notforloan is mapped with
the NOT_LOAN authorised value category, but that is not necessarily the
case.

We must retrieve the correct AV category
Comment 3 Jonathan Druart 2020-09-03 14:09:24 UTC
Created attachment 109628 [details] [review]
Bug 26323: Retrieve the correct values for LOC, DAMAGED, LOC and CCODE

Same as previously
Comment 4 Jonathan Druart 2020-09-03 14:11:00 UTC
This will be tricky to test, that's why I hadn't provided a test plan.

You should test some of the modified occurrences and confirmed it displays the correct value.

Also read the diff and search for typos!
Comment 5 Katrin Fischer 2020-09-03 17:07:24 UTC
All our documentation lists the authorised values, they are used in the default frameworks you are not supposed to change etc. Do we really need to support that they can be anything? I think we would make developer's and support staff's life easier if we kept expecting specific codes. Even with this patch, we will still do that in a lot of other places where there is no mapping table for lookup (TERM, SUGG, etc.)

My first thought here would actually be to fix the ETAT ones to be NOT_LOAN instead.
Comment 6 Jonathan Druart 2020-09-04 09:07:09 UTC
(In reply to Katrin Fischer from comment #5)
> All our documentation lists the authorised values, they are used in the
> default frameworks you are not supposed to change etc. Do we really need to
> support that they can be anything? I think we would make developer's and
> support staff's life easier if we kept expecting specific codes. Even with
> this patch, we will still do that in a lot of other places where there is no
> mapping table for lookup (TERM, SUGG, etc.)

Yes but TERM, SUGG, etc. are not linked with a DB fields. That's not the same problematic.
The UI let you modify it, so I think we should go toward supporting it. And this second patch goes to this direction.
Also it seems that we should remove the ability to edit the AV mapping for non default frameworks (but that's definitely out of the scope here).

> My first thought here would actually be to fix the ETAT ones to be NOT_LOAN
> instead.

Ok but the patch is written already :)
Comment 7 Didier Gautheron 2020-09-04 11:03:56 UTC
(In reply to Jonathan Druart from comment #3)
> Created attachment 109628 [details] [review] [review]
> Bug 26323: Retrieve the correct values for LOC, DAMAGED, LOC and CCODE
> 
> Same as previously

Hi
I'm not sure about the one LOC, CCODE and co aren't hard code in template only they are in perl code too.
Comment 8 Fridolin Somers 2020-09-04 11:45:33 UTC
(In reply to Jonathan Druart from comment #3)
> Created attachment 109628 [details] [review] [review]
> Bug 26323: Retrieve the correct values for LOC, DAMAGED, LOC and CCODE
> 
> Same as previously

You mean "for LOST, DAMAGED, LOC and CCODE" ;)
Comment 9 Jonathan Druart 2020-09-07 09:53:33 UTC
(In reply to didier from comment #7)
> (In reply to Jonathan Druart from comment #3)
> > Created attachment 109628 [details] [review] [review] [review]
> > Bug 26323: Retrieve the correct values for LOC, DAMAGED, LOC and CCODE
> > 
> > Same as previously
> 
> Hi
> I'm not sure about the one LOC, CCODE and co aren't hard code in template
> only they are in perl code too.

As I said previously, it's a move toward the right direction IMO. It does not mean to fix all the occurrences (but does fix all the occurrences in the templates).
Comment 10 Jonathan Druart 2020-09-07 09:54:41 UTC
Created attachment 109693 [details] [review]
Bug 26323: Retrieve the correct values for LOST, DAMAGED, LOC and CCODE

Same as previously
Comment 11 Marcel de Rooy 2020-09-11 06:23:32 UTC
(In reply to Katrin Fischer from comment #5)
> All our documentation lists the authorised values, they are used in the
> default frameworks you are not supposed to change etc. Do we really need to
> support that they can be anything? I think we would make developer's and
> support staff's life easier if we kept expecting specific codes. Even with
> this patch, we will still do that in a lot of other places where there is no
> mapping table for lookup (TERM, SUGG, etc.)

Replacing hardcoded values is normally the right thing to do.
But I may understand your point. Think that we can still refer to specific codes as they are the values loaded by the default setup. Someone who changes the authorised values and kohafield mappings, should know what they do.

That said, I dont see why we should not move on this path. (Knowing that we will be on the road for some time ;)
Comment 12 Katrin Fischer 2020-09-25 15:32:07 UTC
I am not going to block this - I am still going to discourage libraries to use their own whenever asked tho :)
Comment 13 Martin Renvoize 2020-09-25 16:00:22 UTC
Created attachment 110767 [details] [review]
Bug 26323: Retrieve the correct NOT_LOAN value

From the template we are assuming that items.notforloan is mapped with
the NOT_LOAN authorised value category, but that is not necessarily the
case.

We must retrieve the correct AV category

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2020-09-25 16:00:28 UTC
Created attachment 110768 [details] [review]
Bug 26323: Retrieve the correct values for LOST, DAMAGED, LOC and CCODE

Same as previously

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2020-09-25 16:00:33 UTC
Created attachment 110769 [details] [review]
Bug 26323: (follow-up) Add new cases introduced

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2020-09-25 16:01:34 UTC
All works as expected to me.. Signing off.

Query: Do the hard coded CASE values in intranet-tmpl/prog/en/modules/admin/authorised_values.tt need any handling.. I couldn't make up my mind.
Comment 17 Katrin Fischer 2020-09-26 18:27:46 UTC
Created attachment 110789 [details] [review]
Bug 26323: Retrieve the correct NOT_LOAN value

From the template we are assuming that items.notforloan is mapped with
the NOT_LOAN authorised value category, but that is not necessarily the
case.

We must retrieve the correct AV category

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Katrin Fischer 2020-09-26 18:27:52 UTC
Created attachment 110790 [details] [review]
Bug 26323: Retrieve the correct values for LOST, DAMAGED, LOC and CCODE

Same as previously

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Katrin Fischer 2020-09-26 18:27:56 UTC
Created attachment 110791 [details] [review]
Bug 26323: (follow-up) Add new cases introduced

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 20 Katrin Fischer 2020-09-26 18:28:03 UTC
Created attachment 110792 [details] [review]
Bug 26323: (QA follow-up) Fix syntax errors

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Jonathan Druart 2020-09-28 14:11:52 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 22 Lucas Gass 2020-10-20 14:03:54 UTC
doesnt apply clean to 20.05.x, no backport