Bug 5877 - Offline circulation improvements : upload all files, apply at once
Summary: Offline circulation improvements : upload all files, apply at once
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 3.8
Hardware: All All
: P1 - high enhancement (vote)
Target Milestone: ---
Assignee: Paul Poulain
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 5874
Blocks: 5575
  Show dependency treegraph
 
Reported: 2011-03-16 02:36 UTC by Chris Cormack
Modified: 2013-12-05 20:04 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed Patch (11.52 KB, patch)
2011-03-16 02:40 UTC, Chris Cormack
Details | Diff | Splinter Review
FF4 released today (43.38 KB, image/png)
2011-03-22 15:50 UTC, Nicole C. Engard
Details
proposed patch (updated) (18.19 KB, patch)
2011-07-18 14:29 UTC, Paul Poulain
Details | Diff | Splinter Review
Rebased patch with version set correctly (18.19 KB, patch)
2011-08-05 10:33 UTC, Julian Maurice
Details | Diff | Splinter Review
Rebased patch with version set correctly (18.19 KB, patch)
2011-08-05 10:36 UTC, Julian Maurice
Details | Diff | Splinter Review
rebased patch vith version set correctly (17.72 KB, patch)
2011-10-03 19:33 UTC, Paul Poulain
Details | Diff | Splinter Review
proposed patch (17.80 KB, patch)
2011-10-03 20:24 UTC, Paul Poulain
Details | Diff | Splinter Review
[PATCH] [SIGNED-OFF] Bug 5877 : Offline circulation improvements : upload all files, apply at once (17.83 KB, patch)
2011-12-02 16:14 UTC, Sophie MEYNIEUX
Details | Diff | Splinter Review
log (40.66 KB, image/png)
2012-01-18 15:31 UTC, Nicole C. Engard
Details
Follow-up fix (4.77 KB, patch)
2012-01-19 17:57 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5877 : Offline circulation improvements : upload all files, apply at once (17.85 KB, patch)
2012-02-13 13:57 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 5877 Follow-up for Offline circulation improvements (4.79 KB, patch)
2012-02-17 14:54 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 5877 : Offline circulation improvements : upload all files, apply at once (17.83 KB, patch)
2012-02-17 14:54 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 5877 : Offline circulation improvements : upload all files, apply at once (17.83 KB, patch)
2012-02-17 14:55 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED_QA]Bug 5877 Follow-up for Offline circulation improvements (4.79 KB, patch)
2012-02-17 14:56 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 5877 QA follow-up (11.30 KB, patch)
2012-02-22 17:12 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 5877 : Offline circulation improvements : upload all files, apply at once (17.87 KB, patch)
2012-03-01 10:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2011-03-16 02:36:14 UTC

    
Comment 1 Chris Cormack 2011-03-16 02:37:30 UTC
Offline circ : You now can upload all offline files from the Firefox extension.
Once all circ desks have uploaded the file, the librarian can apply all of them, sorted by date.
This avoid the problem of someone issuing an item on desk A, returning it on desk B.
Before this improvement, if desk B uploaded the file before A, the return was applied before the issue,
resulting in the items reamining issued.
Comment 2 Chris Cormack 2011-03-16 02:40:47 UTC Comment hidden (obsolete)
Comment 3 Nicole C. Engard 2011-03-16 19:04:32 UTC
I'm confused about this one - the patch applies fine. I installed the Firefox KOCT https://addons.mozilla.org/en-US/firefox/addon/koct/ I set it up and my log shows that checkouts have been recorded, but when I try to send them to Koha nothing happens - nothing I can tell at least. I need a test case and/or a tutorial on how this works and how to test this patch.

Nicole
Comment 4 Paul Poulain 2011-03-18 14:03:38 UTC
Nicole, do you have configured your extension correctly (last tab of the extension, where you define you koha url & librarian login & password) ?

when you upload the datas, the last line is updated from "local" to "uploaded" (or something like that)
Comment 5 Nicole C. Engard 2011-03-19 15:39:35 UTC
I do have it set up with my super librarian login and the URL of my Koha staff client - but I do not see any note about it being uploaded.
Comment 6 Paul Poulain 2011-03-22 14:44:19 UTC
Nicole, the table pending_offline_operations that must be here seems not to be on the database !
Does it work better after a:
CREATE TABLE `pending_offline_operations` (
  `operationid` int(11) NOT NULL AUTO_INCREMENT,
  `userid` varchar(30) NOT NULL,
  `branchcode` varchar(10) NOT NULL,
  `timestamp` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP,
  `action` varchar(10) NOT NULL,
  `barcode` varchar(20) NOT NULL,
  `cardnumber` varchar(16) DEFAULT NULL,
  PRIMARY KEY (`operationid`)
) ENGINE=MyISAM AUTO_INCREMENT=2 DEFAULT CHARSET=latin1
Comment 7 Nicole C. Engard 2011-03-22 15:50:15 UTC
Created attachment 3400 [details]
FF4 released today

The plugin isn't compatible with FF 4 - so I haven't tried adding the database info because FF has disabled the plugin until it's updated to work with the newest version of the browser.

Nicole
Comment 8 Katrin Fischer 2011-04-24 15:10:28 UTC
The table pending_offline_operations is not part of the patch and not part of my new 3.4 database. Marking as Failed QA.
Comment 9 Paul Poulain 2011-05-09 15:16:35 UTC
(In reply to comment #7)
> Created attachment 3400 [details]
> FF4 released today
> 
> The plugin isn't compatible with FF 4 - so I haven't tried adding the database
> info because FF has disabled the plugin until it's updated to work with the
> newest version of the browser.

Plugin updated, now compatible with FF4
Comment 10 Nicole C. Engard 2011-05-09 15:21:35 UTC
It still says last updated in February ... was that when it was fixed?

https://addons.mozilla.org/en-US/firefox/addon/koct/
Comment 11 Nicole C. Engard 2011-05-09 15:23:55 UTC
Also, that page links to the old koha sites - can it be updated?
Comment 12 Paul Poulain 2011-05-09 15:32:16 UTC
(In reply to comment #10)
> It still says last updated in February ... was that when it was fixed?
> 
> https://addons.mozilla.org/en-US/firefox/addon/koct/

in fact, the fix was just to say to mozilla.org that it was OK with FF4. No fix in KOCT itself
Comment 13 Paul Poulain 2011-05-09 15:32:34 UTC
(In reply to comment #11)
> Also, that page links to the old koha sites - can it be updated?

OK, will ask Claire immediatly for that
Comment 14 Paul Poulain 2011-05-23 11:34:33 UTC
some of our customers are now live with FF4 and this plugin, switching back to "needs signoff"

Notice: 3 customers reported "the plugin has disappeared". It just because, by default, on FF4, the plugin bar at the bottom is hidden. ctrl-/ to have it displayed !
Comment 15 Liz Rea 2011-06-15 18:43:46 UTC
The necessary database structure is still not part of this patch, and needs to be added, as Katrin mentioned earlier. Marking as Failed QA on those grounds.
Comment 16 Paul Poulain 2011-07-18 14:29:37 UTC Comment hidden (obsolete)
Comment 17 Julian Maurice 2011-08-05 10:33:27 UTC Comment hidden (obsolete)
Comment 18 Julian Maurice 2011-08-05 10:36:56 UTC Comment hidden (obsolete)
Comment 19 Owen Leonard 2011-09-30 16:37:15 UTC
I'm trying to test this and failing because I don't understand how the offline circ addon communicates with Koha. The wiki page (http://wiki.koha-community.org/wiki/Offline_circulation_firefox_plugin) shows screenshots of the KOCT which don't match the current plugin: The current plugin offers two buttons under the log tab, "Commit to Koha" and "Apply directly." Clicking either one of these doesn't appear to do anything.

The wiki page also shows a screenshot of a page in Koha which doesn't appear to exist in master. The "Process Pending Offline Operations" page doesn't exist as far as I can tell. I'm guessing this is in a Biblibre branch?

Is this why the KOCT doesn't do anything?
Comment 20 Paul Poulain 2011-10-03 19:33:01 UTC Comment hidden (obsolete)
Comment 21 Paul Poulain 2011-10-03 20:24:25 UTC Comment hidden (obsolete)
Comment 22 Paul Poulain 2011-10-24 11:38:13 UTC
Updating Version : This ENH will be for Koha 3.8
Comment 23 Paul Poulain 2011-10-25 15:05:48 UTC
Bug versionned for master. entries will be made against rel_3_8 once the patch has been applied (see thread about that on koha-devel yesterday)
Comment 24 Sophie MEYNIEUX 2011-12-02 16:14:16 UTC Comment hidden (obsolete)
Comment 25 Marcel de Rooy 2012-01-10 14:52:29 UTC
Before starting QA, I would say that a patch of this size needs external signoff. From the comments I understand that several tested, but nobody signed off until now..
Comment 26 Paul Poulain 2012-01-10 20:46:54 UTC
Marcel, it seems Sophie made a mistake when signing off the patch: i'm the author of the patch (see obsoletes), Sophie just signed it off. I don't know why she appears as the author !

going back to signed off for QA, thx
Comment 27 Marcel de Rooy 2012-01-18 12:57:20 UTC
(In reply to comment #26)
> Marcel, it seems Sophie made a mistake when signing off the patch: i'm the
> author of the patch (see obsoletes), Sophie just signed it off. I don't know
> why she appears as the author !
> 
> going back to signed off for QA, thx

Well, Sophie could correct that easily, I suppose.

My earlier question was actually:  a patch of this size would preferably not have patch writer and patch signer from the same company..
I already asked the list and QA team to think about a status for that situation like Needs additional signoff or so, but no response so far.. Don't think that In Discussion is a good alternative now.
Comment 28 Paul Poulain 2012-01-18 13:10:03 UTC
Nicole, Liz & owen, one of you already has tried to test this patch, if you want to give it another try...
Comment 29 Owen Leonard 2012-01-18 13:30:05 UTC
Paul, I'd like to hear my questions in comment 19 addressed before I test again.
Comment 30 Paul Poulain 2012-01-18 15:09:19 UTC
(In reply to comment #29)
> Paul, I'd like to hear my questions in comment 19 addressed before I test
> again.

wiki page updated, with new screenshots & updated documentation. Feel free to fix typos ;-)

Also please check that you have a pending_offline_operations table (should be added by the updatedatase), it "uploading" circ information can also fail because of that !
Comment 31 Nicole C. Engard 2012-01-18 15:30:04 UTC
I have installed the plugin and I can't get it to log my actions at all - I haven't tested this patch yet because I can't get to that step.  Also, my screens don't look like the ones on the Wiki - will attach my screens here.

Nicole
Comment 32 Nicole C. Engard 2012-01-18 15:31:36 UTC
Created attachment 7218 [details]
log

My log stays empty no matter what I scan and save on the previous tabs.
Comment 33 Paul Poulain 2012-01-18 15:39:20 UTC
(In reply to comment #32)
> Created attachment 7218 [details]
> log
> 
> My log stays empty no matter what I scan and save on the previous tabs.

Are you using MacOS ?
this plugin works for all our customers that are under windows. Works also under linux, I think it's untested on MacOS-X
Comment 34 Nicole C. Engard 2012-01-18 15:40:28 UTC
Bingo! I'm on Mac and it does not work.

Nicole
Comment 35 Paul Poulain 2012-01-18 16:24:52 UTC
(In reply to comment #34)
> Bingo! I'm on Mac and it does not work.

And, for the record: bingo bingo, now it works, and Nicole made nothing specific :(
Comment 36 Owen Leonard 2012-01-18 20:35:50 UTC
Paul, the screenshot on the wiki page shows a calendar for choosing "expected return" date in the KOCT addon. Is that something you've developed for the addon which isn't in the official version?
Comment 37 Owen Leonard 2012-01-19 17:57:03 UTC Comment hidden (obsolete)
Comment 38 Paul Poulain 2012-01-27 10:14:17 UTC
(In reply to comment #36)
> Paul, the screenshot on the wiki page shows a calendar for choosing "expected
> return" date in the KOCT addon. Is that something you've developed for the
> addon which isn't in the official version?

nope, I don't know where this come from. The KOCT we deploy for our customers is the one that is at  https://addons.mozilla.org/en-US/firefox/addon/koct/ 
(I can't ask Jean André anymore about early code, as he's not more working for us. Maybe it was an early-code screenshot, I really don't know)
Comment 39 Marcel de Rooy 2012-02-06 07:58:03 UTC
I would like to see an external signoff on a patch of this size. Sophie signed it off, while Paul apparently should be the author.
Could someone outside Biblibre signoff please? I see that several people tested this one already, but did not..
Changing status to reflect this request.
Comment 40 Owen Leonard 2012-02-13 13:57:56 UTC Comment hidden (obsolete)
Comment 41 Paul Poulain 2012-02-17 14:39:08 UTC
The script devel/misc/testbugzillapatches.pl has detected that this bug does not apply anymore, so i'm marking this bug as "does not apply".

The script is supposed to handle conflicts related to updatedatabase, but no other cases of merge conflict. So the conflict may be trivial to fix.

If it happens that the patch attached does apply cleanly, please send me an email, there's something wrong in the script. It's possible, as it's a recent script.
If the patch applies does not apply as I think, please rebase and resubmit, thanks
Comment 42 Paul Poulain 2012-02-17 14:54:11 UTC Comment hidden (obsolete)
Comment 43 Paul Poulain 2012-02-17 14:54:26 UTC Comment hidden (obsolete)
Comment 44 Paul Poulain 2012-02-17 14:55:39 UTC Comment hidden (obsolete)
Comment 45 Paul Poulain 2012-02-17 14:56:35 UTC
Created attachment 7718 [details] [review]
[PASSED_QA]Bug 5877 Follow-up for Offline circulation improvements

Internationalization fix: Offline circulation operations in
Circulation.pm return English strings for display in the
template. These strings can't be translated, so we must check
their values in the JavaScript and return a translatable
string based on their values.

Interface change: Switching to text links for checking/unchecking
checkboxes since that is more consistent with other areas in
Koha.

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Comment 46 Paul Poulain 2012-02-17 14:59:04 UTC
OK, after checking why the script devel/misc/testbugzillapatches.pl has detected this patch does not apply, I've discovered that it was just because the 2 follow-up was appearing before the main patch.

So:
* i've rebased on master, everything is OK, i just managed the updatedatabase
* signed-off owen follow-up
* re-uploaded the 2 patches, in the correct order
* we're back to signed-off / waiting for QA.

Thanks ian/marcel for your QA !
Comment 47 Marcel de Rooy 2012-02-20 12:34:47 UTC
QA Comment (first patch): Looks good in general, just a few points need attention.

C4/Circulation.pm:
Please adjust or send a follow up for AddOfflineOperation:
The insert statement does not mention the field names. If the table structure is changed some day, this kind of statement will fail.

offline_circ/list.pl, offline_circ/process.pl, offline_circ/service.pl: 
Address of Free Software Foundation is not correct. 
Please add use strict and use warnings.
Please confirm too if tests do not show any warnings in your logfile.

Second patch looks good. Marked as Passed QA in attachment name.
Comment 48 Paul Poulain 2012-02-22 17:12:41 UTC
Created attachment 7812 [details] [review]
Bug 5877 QA follow-up

* removing tabs as indenter
* adding fieldnames in INSERT clause
* updating FSF address
* removing ` in SQL table creation (mysql-ism)
* use strict & use warning added & no error in logs checked

Note that process_koc.pl and updatedatabase are not related to this bug, but the FSF address was wrong, I fixed it as well
Comment 49 Paul Poulain 2012-02-22 17:14:56 UTC
(In reply to comment #47)
> QA Comment (first patch): Looks good in general, just a few points need
> attention.
> 
> C4/Circulation.pm:
> Please adjust or send a follow up for AddOfflineOperation:
> The insert statement does not mention the field names. If the table structure
> is changed some day, this kind of statement will fail.
good catch ! follow-up coming.

> offline_circ/list.pl, offline_circ/process.pl, offline_circ/service.pl: 
> Address of Free Software Foundation is not correct. 
good catch too. I also have fixed the FSF address ... in updatedatabase, as it was wrong here too !

I also have fixed some tabs (replaced by 4 spaces) and removed the ` in the table creation.

> Please add use strict and use warnings.
same thing : in the follow-up i'll attach now. they don't issue any warning.

switching to "signed off", then changes are QA changes that should have no effect on the feature itself
Comment 50 Marcel de Rooy 2012-02-29 14:36:46 UTC
When I apply 7716 followed by 7718 and then the latest one (7812), the third patch fails. What am I doing wrong here? Please check.
Comment 51 Owen Leonard 2012-02-29 15:01:13 UTC
After dealing with the updatedatabase.pl conflict in attachment 7716 [details] [review] the other two patches apply fine for me. I used "git am -i3u" on the first patch and "git bz apply" for the others.
Comment 52 Marcel de Rooy 2012-02-29 15:06:24 UTC
After fixing the first, the third still gives:

Applying: Bug 5877 QA follow-up
error: patch failed: C4/Circulation.pm:3037
error: C4/Circulation.pm: patch does not apply
error: patch failed: offline_circ/list.pl:41
error: offline_circ/list.pl: patch does not apply
error: patch failed: offline_circ/service.pl:30
error: offline_circ/service.pl: patch does not apply
Using index info to reconstruct a base tree...
error: patch failed: C4/Circulation.pm:3037
error: C4/Circulation.pm: patch does not apply
error: patch failed: offline_circ/list.pl:41
error: offline_circ/list.pl: patch does not apply
error: patch failed: offline_circ/service.pl:30
error: offline_circ/service.pl: patch does not apply
Did you hand edit your patch?
It does not apply to blobs recorded in its index.
Cannot fall back to three-way merge.
Comment 53 Marcel de Rooy 2012-02-29 15:07:27 UTC
(In reply to comment #51)
> After dealing with the updatedatabase.pl conflict in attachment 7716 [details] [review] the other
> two patches apply fine for me. I used "git am -i3u" on the first patch and "git
> bz apply" for the others.

Are we on the same commit? 1a5afaa26d374144d1ebde6bcf9c69b97c048a5d
Comment 54 Owen Leonard 2012-02-29 15:09:26 UTC
(In reply to comment #53)
> Are we on the same commit? 1a5afaa26d374144d1ebde6bcf9c69b97c048a5d

Yes
Comment 55 Marcel de Rooy 2012-02-29 15:40:35 UTC
Forget my latest comments: problems with vim..
Comment 56 Marcel de Rooy 2012-03-01 10:43:38 UTC
Created attachment 7960 [details] [review]
Bug 5877 : Offline circulation improvements : upload all files, apply at once

Simple rebase.
Comment 57 Marcel de Rooy 2012-03-01 10:47:28 UTC
So, everything applied fine now; vim was causing problems yesterday.
The last followup patch resolves the earlier QA comments (and even more!)
So the three patches together satisy QA now. Changing status.
Comment 58 Paul Poulain 2012-03-02 17:05:16 UTC
patches where not in the right order (follow-up before main patch), but I deal with that. patch pushed.
Comment 59 Jared Camins-Esakov 2012-12-31 00:52:38 UTC
There have been no further reports of problems so I am marking this bug resolved.