| Summary: | Create Batch Label from Staged MARC Records fails after upgrade to 3.4.x | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Larry Baerveldt <larry> |
| Component: | Label/patron card printing | Assignee: | Ian Walls <koha.sekjal> |
| Status: | CLOSED FIXED | QA Contact: | Bugs List <koha-bugs> |
| Severity: | normal | ||
| Priority: | PATCH-Sent (DO NOT USE) | CC: | brendan, chris, koha.sekjal |
| Version: | 3.4 | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | --- |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: | ||
| Circulation function: | |||
| Attachments: |
Proposed fix
Bug 6673: Create Batch Label from Staged MARC Records fails Expanded patch Signed-off patch |
||
|
Description
Larry Baerveldt
2011-08-05 13:16:22 UTC
Created attachment 4869 [details] [review] Proposed fix Created attachment 4874 [details] [review] Bug 6673: Create Batch Label from Staged MARC Records fails For some reason, the code attempts to get the current users branch in the following manner: $branch_code = get_branch_code_from_name($template->param('LoginBranchname')) This seems particularly cumbersome and prone to failure if there is not template param LoginBranchname. This patch changes the given line to use the more traditional method for finding current branchcode: $branch_code = C4::Context->userenv->{'branch'}; Initial testing shows this to help alleviate the problem Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Works well, is more robust. Signed Off I just tried testing this by doing the following: - Apply the patch (attachment 4874 [details] [review]) to my test system - Export a batch of MARC records from our live system - Import the batch into my test system - Find the batch listed on the Manage Staged MARC Records page - Click 'Create Label Batch' link for that set I still got "Error attempting to create label batch" message. In my logs I find this: manage-marc-import.pl: create_labelbatch_from_importbatch() : Call to C4::ImportBatch::GetItemNumbersFromImportBatch returned no item number(s) from import batch #13. at /home/oleonard/kohaclone/tools/manage-marc-import.pl line 196. Owen - Can you try it with a new batch of items - instead of ones that are already in your system. I just want to compare the differences. Looking at the database - I see the batches that are not adding items (are going into the database with a NULL itemnumber in the labels_database tables - since they are already in there). It will just give me another spot to test on. Sorry, looks like I neglected to post the results of my last test: Adding a batch of items which I've gotten from an external source still result in the same error in my test system running current master. Owen, I've discovered that it's not possible to create a label batch if do not have a branch set, as is the default with the Koha database user. The branchcode added to the system must be a valid entry. Are you testing with that user, or with a specific patron account? I've poked through this pretty thoroughly... not sure what else it could be Further evidence: all other invocations of the variable LoginBranchname work like this:
get_branch_code_from_name($template->{VARS}->{'LoginBranchname'})
Whereas (at least before the attached patch), manage-marc-import.pl does this:
get_branch_code_from_name($template->param('LoginBranchname'))
By default, LoginBranchname is defined as:
C4::Context->userenv?C4::Context->userenv->{"branchname"}:"insecure"
or
C4::Context->userenv?C4::Context->userenv->{"branchname"}:""
depending on what part of C4/Auth.pm you look at.
Since 'insecure' and '' are both invalid entries, it makes more sense to bypass the code=>name=>code jump, and just invoke the code in the standard fashion, as submitted in this patch.
Furthermore
In 3.4.x and onwards,
$template->param('LoginBranchname') always returns nothing
$template->param('anything') will always return nothing. With the change to template::toolkit the param call is just a wrapper to set a variable for use by the template:
$template->param('fish' => 'food') == $template->{VARS}->{'fish'} = 'food';
$template->param('fish') == $template->{VARS}-{'fish'} = '';
The way to get a variable is $template->{VARS}->{'LoginBranchname'}
I agree, we may as well just fetch the branch for the userenv. The reason its failing now, is branch is never going to be set with the existing code
Created attachment 5495 [details] [review] Expanded patch This expanded patch removes all usage of get_branchcode_by_name($template->param('LoginBranchname')) and get_branchcode_by_name($template->{VARS}->('LoginBranchname')) in favour of the more standard C4::Context->userenv->{'branch'} call. This occurs 5 places in the code, all related to Labels Created attachment 5620 [details] [review] Signed-off patch I tested this batch on a fresh data set and found it to work. Before applying the patch, attempting to create a label batch from an imported MARC record batch (/cgi-bin/koha/tools/manage-marc-import.pl?import_batch_id=xx&op=create_labels) would result in an error. After applying the patch the label batch is created without errors. I'm very confident of my marking this patch Passed QA, but if anyone would like another set of eyes on this before it gets committed, please speak up. Pushed, please test There have been no further reports of problems so I am marking this bug resolved. |