Bug 9902 - item subfields not populating
Summary: item subfields not populating
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Katrin Fischer
URL:
Keywords:
: 9901 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-03-22 13:45 UTC by Nicole C. Engard
Modified: 2014-12-07 20:02 UTC (History)
7 users (show)

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


Attachments
Bug 9902 - item subfields not populating in 3.10 (2.92 KB, patch)
2013-03-27 13:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9902 - item subfields not populating in 3.10 (3.00 KB, patch)
2013-03-27 15:21 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 9902 - item subfields not populating in 3.10 (2.86 KB, patch)
2013-04-01 12:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9902 - item subfields not populating (3.07 KB, patch)
2013-04-01 12:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9902 - item subfields not populating (3.16 KB, patch)
2013-04-02 08:51 UTC, Chris Cormack
Details | Diff | Splinter Review
[PUSHED 3.12&master][PASSED QA] Bug 9902 - item subfields not populating (3.26 KB, patch)
2013-04-07 14:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
[3.10.X] Bug 9902 - item subfields not populating (3.13 KB, patch)
2013-05-29 09:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9902 - item subfields not populating (3.22 KB, patch)
2013-07-04 03:17 UTC, Liz Rea
Details | Diff | Splinter Review
[3.10.x][PASSED QA] Bug 9902 - item subfields not populating (3.45 KB, patch)
2013-08-03 18:16 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 Nicole C. Engard 2013-03-22 13:45:21 UTC
When receiving issues in serials subfield 2, a, and b are blank on the item form. This wasn't the case in 3.8 they used to fill in with the defaults like when in cataloging.
Comment 1 Kyle M Hall 2013-03-27 13:09:41 UTC
Also subfield o
Comment 2 Kyle M Hall 2013-03-27 13:14:34 UTC
*** Bug 9901 has been marked as a duplicate of this bug. ***
Comment 3 Kyle M Hall 2013-03-27 13:21:31 UTC Comment hidden (obsolete)
Comment 4 Nicole C. Engard 2013-03-27 15:21:58 UTC Comment hidden (obsolete)
Comment 5 Chris Cormack 2013-03-29 09:19:17 UTC
Comment on attachment 16938 [details] [review]
Bug 9902 - item subfields not populating in 3.10

Review of attachment 16938 [details] [review]:
-----------------------------------------------------------------

::: C4/Items.pm
@@ +2663,1 @@
>                          $defaultvalue = $defaultvalues->{callnumber};

Hmm this doesn't make sense to me

We have if do something
else
do the exact same thing 

couldnt that just be 
do the thing

IE couldnt that just be $defaultvalue = $defaultvalues->{callnumber};

The comments do not match the code, that else will be triggered even if the item record does exist, if the has a callnumber, it will match the else. IE the whole IF and ELSE is a no op
Comment 6 Kyle M Hall 2013-04-01 12:06:59 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2013-04-01 12:14:59 UTC Comment hidden (obsolete)
Comment 8 Chris Cormack 2013-04-02 07:48:01 UTC
(In reply to comment #7)
> Created attachment 17093 [details] [review] [review]
> Bug 9902 - item subfields not populating


Thanks for addressing the review, ill retest now
Comment 9 Chris Cormack 2013-04-02 08:35:17 UTC
Interestingly in 3.8.x we did

 if (   ( $tagslib->{$tag}->{$subfield}->{kohafield} eq 'items.holdingbranch' || $tagslib->{$tag}->{$subfield}->{kohafield} eq 'items.homebranch' )
                    && $defaultvalues
                    && $defaultvalues->{'branchcode'} ) {
                    my $temp;
                    if ($itemrecord) {
                        $temp = $itemrecord->field($subfield);
                    }
                    unless ($temp) {
                        $defaultvalue = $defaultvalues->{branchcode} if $defaultvalues;
                    }
                }  

Bug 9116 removed these, and inadvertently caused this regression

                 if (   ( $tagslib->{$tag}->{$subfield}->{kohafield} eq 'items.holdingbranch' || $tagslib->{$tag
                     && $defaultvalues
                     && $defaultvalues->{'branchcode'} ) {
-                    my $temp;
-                    if ($itemrecord) {
-                        $temp = $itemrecord->field($subfield);
-                    }
-                    unless ($temp) {
-                        $defaultvalue = $defaultvalues->{branchcode} if $defaultvalues;
+                    if ( $itemrecord and $defaultvalues and not $itemrecord->field($subfield) ) {
+                        $defaultvalue = $defaultvalues->{branchcode};
                     }
                 }


You'll notice the new behaviour is not the same as the old one, basically the change in bug 9116 is fine for editing any item, but means that default values are never used on new items, causing the problem that Nicole discovered and Kyle fixed.

We have a couple of choices, revert bug 9116 (probably hard to do now) as it was just a code cleaning fix that broke functionality, or use Kyle's patch.

I think probably best to use Kyles patch. This does prove a good illustration of why code cleaning is good in theory, hard to get right in practice. 
http://blog.bigballofwax.co.nz/2012/07/31/yes-you-should-rewrite-it-no-not-like-that/
Comment 10 Chris Cormack 2013-04-02 08:51:02 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2013-04-02 20:12:24 UTC
I agreee that this is an improvement, but I think we are still missing something - shelving location. I can specify that in the subscription, but it does not carry over.
Comment 12 Katrin Fischer 2013-04-07 14:24:27 UTC
Hmm, not sure what happened, but on retesting this it works for all listed fields.
Comment 13 Katrin Fischer 2013-04-07 14:25:20 UTC Comment hidden (obsolete)
Comment 14 Jared Camins-Esakov 2013-04-07 17:20:50 UTC
This patch has been pushed to master and 3.12.x.
Comment 15 Mathieu Saby 2013-04-21 18:11:53 UTC
I am not sure, but I fear this patch is responsible for a nasty software error when creating a basket with staged records.
See BZ 10088

M. Saby
Comment 16 Nicole C. Engard 2013-05-29 01:29:36 UTC
Was this pushed to 3.10.x? I don't think so and that's where this bug was found. Reopening.  Close if I'm wrong.
Comment 17 Katrin Fischer 2013-05-29 09:24:56 UTC
This couldn't be pushed, because it caused another bug on 10088. I would like to see this solved with a patch for 3.10 specifically and the patch being signed off also making sure the problem from 10088 doesn't occur.
Comment 18 Jonathan Druart 2013-05-29 09:48:27 UTC Comment hidden (obsolete)
Comment 19 Katrin Fischer 2013-05-29 09:52:15 UTC
Jonathan, did you see my comment 17? Bit worried about the problems with bug 10088.
Comment 20 Jonathan Druart 2013-05-29 09:55:27 UTC
Nicole, I submit a patch for the 3.10 branch. Could you test it please?
Patch for 10088 has to be applied too!

@Katrin, No I did not see your comment before submitting the patch, sorry :-/ But if 2 patches are pushed together, I expect not to have problem. Don't you think?
Comment 21 Liz Rea 2013-07-04 03:17:52 UTC Comment hidden (obsolete)
Comment 22 Katrin Fischer 2013-08-03 18:16:38 UTC
Created attachment 20089 [details] [review]
[3.10.x][PASSED QA] Bug 9902 - item subfields not populating

Test Plan:
1) Create a new serial with a default location, call number, and library
   ( the library will need to be any but the one you are logged in as )
2) Click the "Recieve" button for this serial
3) Click "Click to add item"
4) Note those values are not populated
5) Apply the patch
6) Reload the page
7) Click "Click to add item"
8) Note those values are now populated

Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>
All tests pass!

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

This seems to restore the former behaviour.

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
signing for 3.10.x - restores former behaviour.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Works nicely. Before applying the patch callnumber,
location and branches are not carried over correctly.
After applying the patch, this works as expected.
Comment 23 Bernardo Gonzalez Kriegel 2013-08-03 20:10:52 UTC
Pushed to 3.10.x, will be in 3.10.10