Bug 8220 - Allow koc uploads to go to process queue instead of being applied directly.
Summary: Allow koc uploads to go to process queue instead of being applied directly.
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low new feature (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-10 09:51 UTC by Kyle M Hall
Modified: 2014-02-19 18:41 UTC (History)
8 users (show)

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


Attachments
Bug 8220 - Allow koc uploads to go to process queue instead of being applied directly. (19.54 KB, patch)
2012-06-10 09:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8220 - Allow koc uploads to go to process queue instead of being applied directly. (20.03 KB, patch)
2012-06-11 12:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8220 - Allow koc uploads to go to process queue instead of being applied directly. (20.01 KB, patch)
2012-06-11 15:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8220 - Allow koc uploads to go to process queue instead of being applied directly. (19.54 KB, patch)
2012-07-01 02:44 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8220 - Allow koc uploads to go to process queue instead of being applied directly. (19.62 KB, patch)
2012-08-04 05:53 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 8220 - Allow koc uploads to go to process queue instead of being applied directly. (18.43 KB, patch)
2012-09-18 13:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8220 - QA Followup - Unit Test (2.17 KB, patch)
2012-12-11 19:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Test KOC File (447 bytes, application/octet-stream)
2013-01-07 16:01 UTC, Kyle M Hall
Details
Bug 8220 - Allow koc uploads to go to process queue instead of being applied directly. (18.67 KB, patch)
2013-03-14 22:33 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8220 - QA Followup - Unit Test (2.17 KB, patch)
2013-03-14 22:33 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 8220 - Allow koc uploads to go to process queue instead of being applied directly. (18.82 KB, patch)
2013-03-18 11:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 8220 - QA Followup - Unit Test (2.49 KB, patch)
2013-03-18 11:11 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 Kyle M Hall 2012-06-10 09:51:11 UTC

    
Comment 1 Kyle M Hall 2012-06-10 09:52:18 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-06-11 12:00:20 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2012-06-11 15:01:42 UTC Comment hidden (obsolete)
Comment 4 Bob Birchall 2012-06-11 16:24:24 UTC
I ordered a Biblibre sandbox to test this patch.  It appears that the patch did not apply.  The extra links do not appear on the Circulation page.  The email from the sandbox creator included the following:

The patches attached to bugzilla 8220 have been applied, and the result is
<h1>Something went wrong !</h1>Applying: Bug 8220 - Allow koc uploads to go to process queue instead of being applied directly.<br/>
Using index info to reconstruct a base tree...<br/>
Falling back to patching base and 3-way merge...<br/>
Auto-merging C4/Circulation.pm<br/>
Auto-merging installer/data/mysql/updatedatabase.pl<br/>
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl<br/>
Failed to merge in the changes.<br/>
Patch failed at 0001 Bug 8220 - Allow koc uploads to go to process queue instead of being applied directly.<br/>
When you have resolved this problem run git am --resolved.<br/>
If you would prefer to skip this patch, instead run git am --skip.<br/>
To restore the original branch and stop patching run git am --abort.<br/>
Bug 8220 - Allow koc uploads to go to process queue instead of being applied directly.<br/>
<br/>
Bug 8220 - Allow koc uploads to go to process queue instead of being applied directly.<br/>
Apply? [yn] <br/>
Patch left in /tmp/Bug-8220---Allow-koc-uploads-to-go-to-process-queu-W6oPhU.patch<br/>

Unable to sign off.
Comment 5 Chris Cormack 2012-07-01 02:44:06 UTC Comment hidden (obsolete)
Comment 6 Chris Cormack 2012-07-01 02:44:34 UTC
Patch rebased and merge conflicts fixed
Comment 7 Paul Poulain 2012-08-04 05:53:12 UTC
Patch tested with a sandbox, by Bob Birchall <bob@calyx.net.au>
Comment 8 Paul Poulain 2012-08-04 05:53:33 UTC Comment hidden (obsolete)
Comment 9 Bob Birchall 2012-08-04 05:57:29 UTC
Test 1:  Created two Koc files.  Loaded both to Koha.  Was able to manage the processing of the files OK.

Test 2:  Used KOC utility to record an issue of an item to a patron.  Saved the KOC file as file A.  Closed KOC.  Opened KOC again, recorded the return of the same item.  Saved the KOC file as file B.  Returned to Koha, uploaded file B first, then file A.  Processed the queue.  Transactions processed in correct sequence (by timestamp).  All good.

Signed off.
Comment 10 Marcel de Rooy 2012-08-30 09:43:35 UTC
Patch contains merge markers.
Comment 11 Kyle M Hall 2012-09-18 13:38:51 UTC Comment hidden (obsolete)
Comment 12 Jared Camins-Esakov 2012-10-06 04:17:40 UTC
Are you sure this is an enhancement? It sounds to me like it fixes a bug. Without the patch, anyone using both the standalone and FF offline circ modules will end up putting their system in an indeterminate state.
Comment 13 Paul Poulain 2012-10-08 08:44:03 UTC
(In reply to comment #12)
> Are you sure this is an enhancement? It sounds to me like it fixes a bug.
> Without the patch, anyone using both the standalone and FF offline circ
> modules will end up putting their system in an indeterminate state.

I think that's a non-sense to use both systems. Either you want the advanced features of the standalone, at the price of local install on your system, or you want the trivial to install & maintain at the price of no local copy of patrons & other adv features.

So, yes I think it's an ENH
Comment 14 Katrin Fischer 2012-10-08 09:29:50 UTC
Hm, I think the problem is different. As you said, the client version offers some advanced features, but you run into problems when you use it at more than one computer. So the patch makes this work (as it does work for the plugin). I think it fixes a problem people might not be aware of using the client version.
Comment 15 Kyle M Hall 2012-11-21 16:20:35 UTC
I see no reason to limit libraries to one version or the other. Why put up an artificial limit?
Comment 16 Paul Poulain 2012-12-11 19:03:41 UTC
QA comment:
 * passes koha-qa.pl, except for a strange 
	valid                       FAIL
		Subroutine ProcessOfflinePayment redefined 
 * this patch introduces a new sub in C4/Circulation.pm, that has no unit test. This is a requirement for 3.12

failing QA (I haven't tested the patch yet, the code looks correct)
Comment 17 Kyle M Hall 2012-12-11 19:48:09 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2012-12-12 20:37:51 UTC
Thank you for the unit tests, Kyle! :)
Comment 19 Paul Poulain 2013-01-03 17:37:39 UTC
Kyle, I would be happy to QA this patch, but I need a .koc file to test it, could you please attach one ?
Comment 20 Kyle M Hall 2013-01-07 16:01:57 UTC
Created attachment 14452 [details]
Test KOC File
Comment 21 Kyle M Hall 2013-01-07 16:03:33 UTC
That would be great! I've attached a KOC file that checks out barcodes 1, 2, 3, 4 and 5 to borrower cardnumber 123456789, and returns barcodes 6, 7, 8 and 9. If you want to change the borrower cardnumber and/or the barcodes, you should be able to just open the file in any text editor and alter it to your liking.

(In reply to comment #19)
> Kyle, I would be happy to QA this patch, but I need a .koc file to test it,
> could you please attach one ?
Comment 22 Katrin Fischer 2013-03-14 22:26:34 UTC
Comment on attachment 12310 [details] [review]
Bug 8220 - Allow koc uploads to go to process queue instead of being applied directly.

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

Hi Kyle,

I started testing with some code review to see what you have done here - I found some smaller issues. A follow-up resolving them would be good.

::: C4/Circulation.pm
@@ +3334,5 @@
> +    my $amount = $operation->{amount};
> +
> +    recordpayment( $borrower->{borrowernumber}, $amount );
> +
> +    return "Success."

Untranslatable string.

::: installer/data/mysql/updatedatabase.pl
@@ +5830,5 @@
> +$DBversion = "3.09.00.XXX";
> +if (C4::Context->preference("Version") < TransformToNum($DBversion)) {
> +    $dbh->do("ALTER TABLE `pending_offline_operations` CHANGE `barcode` `barcode` VARCHAR( 20 ) CHARACTER SET utf8 COLLATE utf8_general_ci NULL DEFAULT NULL");
> +    $dbh->do("ALTER TABLE `pending_offline_operations` ADD `amount` DECIMAL( 28, 6 ) NULL DEFAULT NULL");
> +    print "Upgrade to $DBversion done (redefine the field branchcode as PRIMARY KEY of branches)\n";

Wrong text in update message.

::: koha-tmpl/intranet-tmpl/prog/en/modules/offline_circ/enqueue_koc.tt
@@ +17,5 @@
> +
> +[% FOREACH message IN messages %]
> +  [% IF ( message.message ) %]
> +      [% IF ( message.ERROR_file_version ) %]
> +          <div class="dialog alert"><p>Warning: This file is version [% message.upload_version %], but I only know how to import version [% message.current_version %]. I'll try my best.</p>

Missing </div>

::: offline_circ/enqueue_koc.pl
@@ +18,5 @@
> +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> +#
> +
> +use strict;
> +use warnings;

Maybe better use Modern::Perl?
Comment 23 Katrin Fischer 2013-03-14 22:33:02 UTC Comment hidden (obsolete)
Comment 24 Katrin Fischer 2013-03-14 22:33:13 UTC Comment hidden (obsolete)
Comment 25 Katrin Fischer 2013-03-14 22:33:42 UTC
Attaching rebased patches.
Comment 26 Katrin Fischer 2013-03-18 11:11:08 UTC
Created attachment 16230 [details] [review]
[PASSED QA] Bug 8220 - Allow koc uploads to go to process queue instead of being applied directly.

The primary advantage to the Firefox offline cirulation plugin when compared
to the offline circulation desktop application, is the ability to add offline
circulation actions to a queue so that multiple machines running offline
circ can have their circ actions combined and ordered chronologically before
being executed. This commit adds the ability to put actions from uploaded
KOC files into this queue. In this way, both the FF plugina and the desktop
application can be run side by side with no ill effects.

Signed-off-by: Bob Birchall <bob@calyx.net.au>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 27 Katrin Fischer 2013-03-18 11:11:29 UTC
Created attachment 16232 [details] [review]
[PASSED QA] Bug 8220 - QA Followup - Unit Test

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

All tests and qa script pass.

Tests done:
- created a .koc file with return, issue and fine payments.
- queued that file into Koha
- created some transactions using the Firefox plugin
- queued that into Koha
- processed files and checked outcome was ok
Comment 28 Jared Camins-Esakov 2013-03-22 00:42:54 UTC
This patch has been pushed to master.

I did not test this since I have a Mac and no VMs suitable for testing the KOC application at the moment, but as it is non-core and has been well-tested by Katrin and Bob I am pushing it as-is.

A nice follow-up bug might remove the extra variable(s) in the pl file (our $dbh, possibly others).