Bug 7451 - PrepareItemrecordDisplay missing $subfield_data{id} which breaks AJAX value_builders
Summary: PrepareItemrecordDisplay missing $subfield_data{id} which breaks AJAX value_b...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: 3.8
Hardware: All All
: P1 - high critical (vote)
Assignee: Dobrica Pavlinusic
QA Contact: Ian Walls
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-17 12:52 UTC by Dobrica Pavlinusic
Modified: 2019-06-27 09:24 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
$subfield_data{id} is used for AJAX value_builders and hidden fields (2.66 KB, patch)
2012-01-17 14:25 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 7451 - PrepareItemrecordDisplay missing $subfield_data{id} which breaks AJAX value_builders (2.68 KB, patch)
2012-01-17 14:27 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 7451 - PrepareItemrecordDisplay missing $subfield_data{id} which breaks AJAX value_builders (2.76 KB, patch)
2012-03-22 09:20 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7451 - PrepareItemrecordDisplay missing $subfield_data{id} which breaks AJAX value_builders (2.88 KB, patch)
2012-03-23 16:03 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Dobrica Pavlinusic 2012-01-17 12:52:57 UTC
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.
Comment 1 Dobrica Pavlinusic 2012-01-17 14:25:19 UTC Comment hidden (obsolete)
Comment 2 Dobrica Pavlinusic 2012-01-17 14:27:15 UTC Comment hidden (obsolete)
Comment 3 Paul Poulain 2012-01-17 17:37:31 UTC
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 ;-)
Comment 4 Chris Cormack 2012-01-17 19:04:08 UTC
(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
Comment 5 Paul Poulain 2012-01-18 08:20:49 UTC
(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
Comment 6 Julian Maurice 2012-01-19 09:58:53 UTC
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.
Comment 7 Dobrica Pavlinusic 2012-01-19 12:19:29 UTC
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?
Comment 8 Chris Cormack 2012-02-18 07:14:33 UTC
Hmm whats the status of this patch still valid and needing testing?
Comment 9 Dobrica Pavlinusic 2012-02-20 15:50:13 UTC
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?
Comment 10 Katrin Fischer 2012-02-20 15:56:38 UTC
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.
Comment 11 Katrin Fischer 2012-02-27 06:27:48 UTC
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.
Comment 12 Dobrica Pavlinusic 2012-03-22 09:20:14 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2012-03-23 16:03:29 UTC
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.
Comment 14 Ian Walls 2012-03-25 21:06:53 UTC
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.
Comment 15 Jared Camins-Esakov 2012-05-03 23:35:46 UTC
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.
Comment 16 Jared Camins-Esakov 2012-12-31 00:55:12 UTC
There have been no further reports of problems so I am marking this bug resolved.