|
Description
Didier Gautheron
2020-08-31 10:40:17 UTC
Looks like this is also needed for LOC, DAMAGED, LOC and CCODE. 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 Created attachment 109628 [details] [review] Bug 26323: Retrieve the correct values for LOC, DAMAGED, LOC and CCODE Same as previously 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! 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. (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 :) (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. (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" ;) (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). Created attachment 109693 [details] [review] Bug 26323: Retrieve the correct values for LOST, DAMAGED, LOC and CCODE Same as previously (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 ;) I am not going to block this - I am still going to discourage libraries to use their own whenever asked tho :) 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> 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> Created attachment 110769 [details] [review] Bug 26323: (follow-up) Add new cases introduced Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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. 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> 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> 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> Created attachment 110792 [details] [review] Bug 26323: (QA follow-up) Fix syntax errors Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Pushed to master for 20.11, thanks to everybody involved! doesnt apply clean to 20.05.x, no backport |