Created attachment 7198 [details] [review] $subfield_data{id} is used for AJAX value_builders and hidden fields In current version of code, it gets initialized too late, so it produces unitialized warnings for hidden fields (which is non-fatal) and breaks generated JavaScript for AJAX value_builders (which is fatal) This bug was introduced in ticket 6106 which is modification of 5955 which didn't have this particular problem. Created attachment 7199 [details] [review] Bug 7451 - PrepareItemrecordDisplay missing $subfield_data{id} which breaks AJAX value_builders Dobrica, it seems you've missed that our workflow has changed. You shouldn't use "Patch sent" anymore, but use bug status, where all workflow status for patches have been moved. Switching to status "needs signoff" & removing priority="Patch Sent", as I think you're requesting for a signoff on your patch ;-) (In reply to comment #3) > Dobrica, it seems you've missed that our workflow has changed. > You shouldn't use "Patch sent" anymore, but use bug status, where all workflow > status for patches have been moved. > Why not remove Patch Sent from the priority list? That should help remove some confusion (In reply to comment #4) > Why not remove Patch Sent from the priority list? That should help remove some > confusion Good question, I don't know. I've removed the field from the list. It still appears for existing bugs having this value, so i've updated the value to "PATCH-Sent (DO NOT USE)" Will send a mail to koha-devel immediatly about that I was testing in additem.pl and saw no bugs... because additem.pl doesn't use PrepareItemrecordDisplay but an internal subroutine generate_subfield_form which isn't affected by described problem. Why these two subs? It seems that they do quite the same job. Julian, great point. It seems that generate_subfield_form is mostly similar to inner loop of PrepareItemrecordDisplay. It seems that generate_subfield_form is cleaner code than PrepareItemrecordDisplay so I would propose to move generate_subfield_form into C4::Biblio and then rewrite PrepareItemrecordDisplay to use it. Does this make sense? Hmm whats the status of this patch still valid and needing testing? PrepareItemrecordDisplay is used by acquisition (that's how we found this problem) and serials, and this patch fixes problem with it, so it still needs sign-off.
I did took a long look at C4::Biblio::PrepareItemrecordDisplay and generate_subfield_form with goal of marging changes, but in the process I found that code diverged and I'm not brave anough just to merge them together.
I'm quite confused by some parts of generate_subfield_form in cataloguing/additem.pl:
if ($frameworkcode eq 'FA' && $subfieldlib->{kohafield} eq 'items.barcode'){
my $input = new CGI;
$value = $input->param('barcode');
}
This just doesn't seem right to me (hard-coded framework and direct access to CGI params), but hopefully somebody with more knowledge of this code could make educated guesses and merge those two implementations together in another bug?
Hi Dobrica, I can tell you what it does :) If you scan in a barcode in circulation that is not in the database yet, Koha offers to add temporary record for it (fast add = FA). The FA framework is one of the frameworks that can be installed with Koha and similar to the ACQ framework, so it's ok that it's hard coded. After you have added the fast add record and an item with the formerly uknown barcode, saving the item will take you back to circulation and immediately check out the new item to the borrower. It's a nice integrated workflow. I can't tell if there are better ways to code it, but it's working quite nicely. Hi Dobrica, I was able to reproduce the problem in serials: 1) I mapped the stocknumberAM123 plugin to my 952$i field and checked it worked in cataloguing. Entering AM and clickingon ... would correctly give me the next number. 2) I created a subscription in seials that would add items and tried the plugin there. There are warnings in the logs and the stocknumber is not generated. I am marking this failed QA for now, perhaps you have an idea? I had to fix a conflict when applying the patch. Perhaps I made a mistake there. Created attachment 8469 [details] [review] Bug 7451 - PrepareItemrecordDisplay missing $subfield_data{id} which breaks AJAX value_builders In current version of code, it gets initialized too late, so it produces unitialized warnings for hidden fields (which is non-fatal) and breaks generated JavaScript for AJAX value_builders (which is fatal) This bug was introduced in ticket 6106 which is modification of 5955 which didn't have this particular problem. Since then, this code moved to C4::Items because of de-nesting patches. Test scenario: 1. define 952$i plugin: stocknumberam123.pl (this is ajax value_builder) 2. go to one of following pages and verify that plugin works: acqui/addorderiso2709.pl acqui/neworderempty.pl acqui/orderreceive.pl serials/serials-edit.pl For stocknumberam123.pl plugin to work, you have to have at least one item with stocknumber in format which this plugin expects: capital letters, space, some number Created attachment 8612 [details] [review] [SIGNED-OFF] Bug 7451 - PrepareItemrecordDisplay missing $subfield_data{id} which breaks AJAX value_builders In current version of code, it gets initialized too late, so it produces unitialized warnings for hidden fields (which is non-fatal) and breaks generated JavaScript for AJAX value_builders (which is fatal) This bug was introduced in ticket 6106 which is modification of 5955 which didn't have this particular problem. Since then, this code moved to C4::Items because of de-nesting patches. Test scenario: 1. define 952$i plugin: stocknumberam123.pl (this is ajax value_builder) 2. go to one of following pages and verify that plugin works: acqui/addorderiso2709.pl acqui/neworderempty.pl acqui/orderreceive.pl serials/serials-edit.pl For stocknumberam123.pl plugin to work, you have to have at least one item with stocknumber in format which this plugin expects: capital letters, space, some number Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Works nicely, thanks for the great test plan. Moves the variable initialization in the hash out of an elsif clause into the scope higher, allowing this data to be accessible in more cases. From analysis of the code, this doesn't change what the field actually IS, just when it gets created. Marking as Passed QA. The patches for this bug do not apply to 3.6. If you want this included in 3.6.x, please rebase on 3.6.x and reset the version to rel_3_6. There have been no further reports of problems so I am marking this bug resolved. |
When using value_builders which have AJAX callback (like cataloguing/value_builder/stocknumberam123.pl) we get JavaScript error in browser because we are not initializing $subfield_data{id} (which is used as unique identifier for this particular field instance). This problem is also evident as warnings in intranet error log like this: Use of uninitialized value $subfield_data{"id"} in concatenation (.) or string at /srv/koha_ffzg//C4/Biblio.pm line 2664 but this affect just hidden fields and is not fatal, as opposed to AJAX example which breaks functionality.