Bug 8352 - Add automatic printing of 'hold to pull' notices
Summary: Add automatic printing of 'hold to pull' notices
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Kyle M Hall
QA Contact:
URL:
Whiteboard:
Keywords: dependency
: 8034 (view as bug list)
Depends on:
Blocks: 16764
  Show dependency treegraph
 
Reported: 2012-07-03 15:08 UTC by Kyle M Hall
Modified: 2018-10-01 08:19 UTC (History)
17 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
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
Bug 8352 - Add automatic printing of 'hold to pull' notices (10.78 KB, patch)
2012-07-03 15:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add automatic printing of 'hold to pull' notices (10.98 KB, patch)
2012-07-11 12:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add automatic printing of 'hold to pull' notices (11.50 KB, patch)
2012-09-26 11:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add automatic printing of 'hold to pull' notices (11.19 KB, patch)
2012-11-15 16:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add automatic printing of 'hold to pull' notices (11.19 KB, patch)
2013-01-03 14:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add automatic printing of 'hold to pull' notices (11.22 KB, patch)
2013-03-19 09:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
[Signed off] Bug 8352 - Add automatic printing of 'hold to pull' notices (11.29 KB, patch)
2013-03-20 16:15 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 8352 - Add automatic printing of 'hold to pull' notices (11.29 KB, patch)
2014-04-15 12:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add automatic printing of 'hold to pull' notices (9.41 KB, patch)
2014-05-19 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add printer administration, update script to use Koha defined printers (41.70 KB, patch)
2014-05-19 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add printer administration, update script to use Koha defined printers (42.53 KB, patch)
2014-05-19 17:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add printer administration, update script to use Koha defined printers (42.58 KB, patch)
2014-06-03 16:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add printer administration, update script to use Koha defined printers (42.60 KB, patch)
2014-06-03 16:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add printer administration, update script to use Koha defined printers (42.57 KB, patch)
2014-06-03 16:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add printer administration, update script to use Koha defined printers (42.48 KB, patch)
2014-06-03 16:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add automatic printing of 'hold to pull' notices (9.68 KB, patch)
2015-03-12 14:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add printer administration, update script to use Koha defined printers (42.68 KB, patch)
2015-03-12 14:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add automatic printing of 'hold to pull' notices (9.30 KB, patch)
2015-07-01 17:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add printer administration, update script to use Koha defined printers (42.50 KB, patch)
2015-07-01 17:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Fix column type in kohastructure.sql (1.59 KB, patch)
2015-07-01 17:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add automatic printing of 'hold to pull' notices (9.30 KB, patch)
2015-08-18 13:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add printer administration, update script to use Koha defined printers (42.61 KB, patch)
2015-08-18 13:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Fix column type in kohastructure.sql (1.59 KB, patch)
2015-08-18 13:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] (2.25 KB, patch)
2015-08-18 13:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Don't use SQL in pl files (17.42 KB, patch)
2015-08-18 13:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add automatic printing of 'hold to pull' notices (9.34 KB, patch)
2015-11-04 12:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8352 - Add printer administration, update script to use Koha defined printers (42.47 KB, patch)
2015-11-04 12:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8352 [QA Followup] - Fix column type in kohastructure.sql (1.66 KB, patch)
2015-11-04 12:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8352 [QA Followup] (2.31 KB, patch)
2015-11-04 12:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8352 [QA Followup] - Don't use SQL in pl files (17.48 KB, patch)
2015-11-04 12:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add automatic printing of 'hold to pull' notices (8.89 KB, patch)
2015-12-01 15:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add printer administration, update script to use Koha defined printers (42.53 KB, patch)
2015-12-01 15:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Fix column type in kohastructure.sql (1.65 KB, patch)
2015-12-01 15:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] (2.29 KB, patch)
2015-12-01 15:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Don't use SQL in pl files (16.46 KB, patch)
2015-12-01 15:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] (9.62 KB, patch)
2015-12-03 15:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Notices for translation (7.94 KB, patch)
2015-12-03 16:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add automatic printing of 'hold to pull' notices (8.89 KB, patch)
2015-12-14 14:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add printer administration, update script to use Koha defined printers (42.53 KB, patch)
2015-12-14 14:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Fix column type in kohastructure.sql (1.65 KB, patch)
2015-12-14 14:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Don't use SQL in pl files (16.33 KB, patch)
2015-12-14 14:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] (9.73 KB, patch)
2015-12-14 14:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Notices for translation (7.92 KB, patch)
2015-12-14 14:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Remove use of HTML::HTMLDoc (2.42 KB, patch)
2015-12-14 14:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Remove use of HTML::HTMLDoc (2.39 KB, patch)
2015-12-14 14:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Remove use of HTML::HTMLDoc (2.45 KB, patch)
2015-12-14 14:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add automatic printing of 'hold to pull' notices (8.88 KB, patch)
2016-01-19 14:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add printer administration, update script to use Koha defined printers (42.65 KB, patch)
2016-01-19 14:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Fix column type in kohastructure.sql (1.64 KB, patch)
2016-01-19 14:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Don't use SQL in pl files (16.32 KB, patch)
2016-01-19 14:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] (8.90 KB, patch)
2016-01-19 14:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Notices for translation (7.89 KB, patch)
2016-01-19 14:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Remove use of HTML::HTMLDoc (2.44 KB, patch)
2016-01-19 14:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8352 - Add automatic printing of 'hold to pull' notices (9.39 KB, patch)
2016-02-16 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8352 - Add printer administration, update script to use Koha defined printers (42.65 KB, patch)
2016-02-16 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Fix column type in kohastructure.sql (1.64 KB, patch)
2016-02-16 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Don't use SQL in pl files (16.32 KB, patch)
2016-02-16 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8352 [QA Followup] (9.30 KB, patch)
2016-02-16 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Notices for translation (11.90 KB, patch)
2016-02-16 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8352 [QA Followup] - Remove use of HTML::HTMLDoc (2.44 KB, patch)
2016-02-16 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8352: Fix perlcritic '$pdf_data' is declared but not used (730 bytes, patch)
2016-02-16 14:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8352: Fix type vs _type for bug 15446 (932 bytes, patch)
2016-02-16 14:18 UTC, Jonathan Druart
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-07-03 15:08:40 UTC
Add the ability to have 'hold to pull' notices printed out automatically for librarians. These notices will be based on the data in the tmp_holdsqueue table. As each new row is added to tmp_holdsqueue, a notice for that given row is printed.
Comment 1 Kyle M Hall 2012-07-03 15:12:10 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-07-11 12:19:33 UTC Comment hidden (obsolete)
Comment 3 Chris Cormack 2012-08-19 07:59:19 UTC
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql
Auto-merging C4/Installer/PerlDependencies.pm
CONFLICT (content): Merge conflict in C4/Installer/PerlDependencies.pm
Failed to merge in the changes.
Comment 4 Kyle M Hall 2012-09-26 11:48:35 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2012-11-15 16:19:37 UTC Comment hidden (obsolete)
Comment 6 Owen Leonard 2012-12-27 16:26:24 UTC
Please provide a test plan.
Comment 7 Kyle M Hall 2013-01-03 14:10:06 UTC
Test Plan:
1) Apply patch
2) Run updatedatabase.pl
3) Place some holds for two or more branches
4) Run misc/cronjobs/holds/print_holds_to_pull_on_demand.pl --test --printer MPL,lp,printserver,515 --printer CPL,lp2,printserver,516

Each --printer switch is the configuration for a given branch. Each branch can have a separate printer, or multiple branches can use the same printer. The switch value is the branchcode, printer name, server name and server port separated by commas. If a printer is not defined for a given branch, notices for
that branch will not be printed.
Comment 8 Kyle M Hall 2013-01-03 14:11:46 UTC Comment hidden (obsolete)
Comment 9 Alex Arnaud 2013-03-18 14:51:43 UTC
Patch does not apply on master:

error: patch failed: C4/Installer/PerlDependencies.pm:633
error: C4/Installer/PerlDependencies.pm: patch does not apply
error: patch failed: installer/data/mysql/en/mandatory/sample_notices.sql:10
error: installer/data/mysql/en/mandatory/sample_notices.sql: patch does not apply
error: patch failed: installer/data/mysql/updatedatabase.pl:6339
error: installer/data/mysql/updatedatabase.pl: patch does not apply
Comment 10 Kyle M Hall 2013-03-19 09:32:09 UTC Comment hidden (obsolete)
Comment 11 Alex Arnaud 2013-03-20 16:15:55 UTC Comment hidden (obsolete)
Comment 12 Chris Cormack 2013-06-05 00:25:51 UTC
I like the idea of use Net::Printer,  but did you know there is already a printer queue and a script to set the printer (network one).

This could be generalised to use this
Check admin/printers.pl

And
DROP TABLE IF EXISTS `printers`;
CREATE TABLE `printers` (
  `printername` varchar(40) NOT NULL default '',
  `printqueue` varchar(20) default NULL,
  `printtype` varchar(20) default NULL,
  PRIMARY KEY  (`printername`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

So i'd like to see it use this (extended if nessecary). 

Will put this to In discussion and will wait for an opinion from Galen, if he is happy with it as it is, I will continue with QA on it.
Comment 13 Katrin Fischer 2013-10-03 11:48:12 UTC
I think I like the idea to have a separate printing queue. Here in Germany we have big libraries with closed stacks with multiple areas and printers. It looks to me like using printers this feature would be easier extendable in the future?
Comment 14 Galen Charlton 2014-04-10 17:01:47 UTC
Now that somebody has actively called my attention to this:

+1 to using Net::Printer.
+1 to taking advantage of the existing structure for defining print queues.
Comment 15 Kyle M Hall 2014-04-15 12:38:49 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2014-04-15 12:50:54 UTC
(In reply to Galen Charlton from comment #14)
> +1 to taking advantage of the existing structure for defining print queues.

I'm afraid I don't believe this to be as good an idea as it may have seemed at first. It appears that admin/printers.pl has been hidden since 2008, 8 years ago. The schema for this table would need to be completely recreated as the existing one does not fit with the data needed for Net::Printer. Even worse, the code itself is extremely broken and would have to be rewritten from scratch.

So, in short, we wouldn't be taking advantage of the existing structure for defining print queues, because it would all need to be completely thrown away and totally rewritten, including the printers table itself.

I'm not really seeing what the advantage would be to defining the printers from the web interface either. In fact, it seems even a bit unwanted perhaps. Printer configuration is a system administration task. I don't see a great advantage to allowing librarians to alter the printer data from a web interface.

Any thoughts?
Comment 17 Katrin Fischer 2014-04-15 13:29:46 UTC
I took only a quick look at the patch and I think if we go the route of adding a new column to reserves, we also need to add the column old_reserves.

What happens if a print is not possible? (printer offline, no paper, etc.) is there a way to repeat the prints? Just a thought, but having the date/time of the print might be useful.

Failing QA on Kyle's request, because of old_reserves - still waiting for Galen's opionion of course.
Comment 18 Galen Charlton 2014-04-15 14:32:32 UTC
(In reply to Kyle M Hall from comment #16)
> (In reply to Galen Charlton from comment #14)
> > +1 to taking advantage of the existing structure for defining print queues.
> 
> I'm afraid I don't believe this to be as good an idea as it may have seemed
> at first. It appears that admin/printers.pl has been hidden since 2008, 8
> years ago. The schema for this table would need to be completely recreated
> as the existing one does not fit with the data needed for Net::Printer. Even
> worse, the code itself is extremely broken and would have to be rewritten
> from scratch.

I've sent Chris a query regarding whether he's got the current print queue stuff working for any of his libraries.

> So, in short, we wouldn't be taking advantage of the existing structure for
> defining print queues, because it would all need to be completely thrown
> away and totally rewritten, including the printers table itself.

Agreed there are some changes that would be required.

> I'm not really seeing what the advantage would be to defining the printers
> from the web interface either. In fact, it seems even a bit unwanted
> perhaps. Printer configuration is a system administration task. I don't see
> a great advantage to allowing librarians to alter the printer data from a
> web interface.

Disagree.  Particularly since Net::Printer talks to the printer directly, there isn't any server-side configuration of setting up lpr queues as seems to be required for the current printer queue approach.
Comment 19 Katrin Fischer 2014-04-25 13:19:32 UTC
I've got a question unrelated to the implementation :)

When an item is returned on the self check, it's not set to waiting, so it should show up on the holds to pull list. Would this also trigger a print?
Comment 20 Katrin Fischer 2014-04-25 13:59:26 UTC
Some notes after talking to Kyle about this feature:

- The prints will happen when the hold is places, the printer used is the pickup branch. This means, that will work for single brnach libraries, but might be a problem to multi branch libraries. Also if the item is 
- The printed flag being on reserves will make it a bit harder to extend this later, I think I would like a separate table with a time queued and a time printed better. We could also extend that later for other automated prints. 
- If the flag stays on reserves, we might want to make it a datetime instead for the time it was printed.
Comment 21 Kyle M Hall 2014-05-19 16:53:26 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2014-05-19 16:53:37 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2014-05-19 17:03:14 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2014-05-19 17:29:26 UTC
Test Plan addition:
There are several new module dependencies for this feature:
HTML::HTMLDoc: Required for both LPR and CUPS printing if printing HTML
Net::Printer: Required for LPR printing
Printer: Required for CUPS printing
Comment 25 Kyle M Hall 2014-06-03 16:09:01 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2014-06-03 16:14:20 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2014-06-03 16:24:26 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2014-06-03 16:31:46 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2015-02-26 15:28:23 UTC
I fixed the conflict on the first patch bug get
fatal: sha1 information is lacking or useless (installer/data/mysql/updatedatabase.pl).
on applying the second one.
Comment 30 Kyle M Hall 2015-03-12 14:23:57 UTC Comment hidden (obsolete)
Comment 31 Kyle M Hall 2015-03-12 14:24:05 UTC Comment hidden (obsolete)
Comment 32 Bernardo Gonzalez Kriegel 2015-05-13 16:05:00 UTC
Hi,
on your first patch you add on kohastructure.pl
 `printed` tinyint(1) DEFAULT NULL,
for reserves and old_reserves

But on final updatedatabase.pl (needs to be on atomicupdate!), you 
"only" alter reserves but using a different type

$dbh->do("ALTER TABLE reserves ADD printed DATETIME NULL AFTER suspend_until");

Both patches have conflicts
Comment 33 Kyle M Hall 2015-07-01 17:41:14 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #32)
> Hi,
> on your first patch you add on kohastructure.pl
>  `printed` tinyint(1) DEFAULT NULL,
> for reserves and old_reserves
> 
> But on final updatedatabase.pl (needs to be on atomicupdate!), you 
> "only" alter reserves but using a different type
> 
> $dbh->do("ALTER TABLE reserves ADD printed DATETIME NULL AFTER
> suspend_until");
> 
> Both patches have conflicts

Good catch!
Comment 34 Kyle M Hall 2015-07-01 17:42:07 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2015-07-01 17:42:14 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2015-07-01 17:42:17 UTC Comment hidden (obsolete)
Comment 37 Bernardo Gonzalez Kriegel 2015-07-01 18:05:18 UTC
All patches apply, but think you need to add this line to atomicupdate file:

ALTER TABLE old_reserves ADD printed DATETIME NULL AFTER suspend_until;

Both tables are modified on kohastructure

Is 'Signed Off' the right status?
Comment 38 Jonathan Druart 2015-07-09 14:21:00 UTC
Kyle,

1/ You propose a --test option to the script, but I think the test behavior should be the default and --confirm should sent the job to the printer, don't you think?

2/ Please avoid sql queries in pl scripts.

3/ You should also address Bernardo's concern.
Comment 39 Kyle M Hall 2015-08-18 13:47:49 UTC Comment hidden (obsolete)
Comment 40 Kyle M Hall 2015-08-18 13:47:58 UTC Comment hidden (obsolete)
Comment 41 Kyle M Hall 2015-08-18 13:48:01 UTC Comment hidden (obsolete)
Comment 42 Kyle M Hall 2015-08-18 13:48:04 UTC Comment hidden (obsolete)
Comment 43 Kyle M Hall 2015-08-18 13:48:08 UTC Comment hidden (obsolete)
Comment 44 Jonathan Druart 2015-08-18 14:20:48 UTC
Only the first patch is signed off.
Comment 45 Jonathan Druart 2015-08-18 14:21:49 UTC
svc/printer does not follow the coding guidelines.
Comment 46 Jonathan Druart 2015-08-18 15:01:48 UTC
(In reply to Jonathan Druart from comment #45)
> svc/printer does not follow the coding guidelines.

forget that, it's done in the last patch.
Comment 47 Kyle M Hall 2015-11-04 12:35:28 UTC Comment hidden (obsolete)
Comment 48 Kyle M Hall 2015-11-04 12:35:45 UTC Comment hidden (obsolete)
Comment 49 Kyle M Hall 2015-11-04 12:35:49 UTC Comment hidden (obsolete)
Comment 50 Kyle M Hall 2015-11-04 12:35:52 UTC Comment hidden (obsolete)
Comment 51 Kyle M Hall 2015-11-04 12:35:56 UTC Comment hidden (obsolete)
Comment 52 Katrin Fischer 2015-11-30 22:00:11 UTC
Please rebase - there are several conflicts.
Comment 53 Kyle M Hall 2015-12-01 15:33:27 UTC Comment hidden (obsolete)
Comment 54 Kyle M Hall 2015-12-01 15:33:40 UTC Comment hidden (obsolete)
Comment 55 Kyle M Hall 2015-12-01 15:33:44 UTC Comment hidden (obsolete)
Comment 56 Kyle M Hall 2015-12-01 15:33:48 UTC Comment hidden (obsolete)
Comment 57 Kyle M Hall 2015-12-01 15:33:53 UTC Comment hidden (obsolete)
Comment 58 Jonathan Druart 2015-12-02 11:01:34 UTC
(In reply to Kyle M Hall from comment #24)
> Test Plan addition:
> There are several new module dependencies for this feature:
> HTML::HTMLDoc: Required for both LPR and CUPS printing if printing HTML
> Net::Printer: Required for LPR printing
> Printer: Required for CUPS printing

Did you check how easy/difficult it is to package these 2 modules?

They don't seem maintained anymore
Printer-0.98 	     17 Aug 2004
HTML-HTMLDoc-0.11_01 03 Nov 2009
Comment 59 Katrin Fischer 2015-12-03 14:13:14 UTC
Is it possible both modules are not packaged for Ubuntu? I couldn't install/find either via apt-get? 

I think the dependency questions need to be resolved first, before we can move this further through QA!

Starting with a code review anyway:

1) QA script
 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/help/admin/printers.tt
   FAIL	  spelling
		 recieve  ==> receive
 FAIL	misc/cronjobs/holds/print_holds.pl
   FAIL	  pod
		*** ERROR: =over on line 135 without closing =back (at head1)  in file misc/cronjobs/holds/print_holds.pl

2) New notice
a) The insert statements are missing the message_transport_type:
+INSERT INTO `letter` (`module`,`code`,`branchcode`,`name`,`is_html`,`title`,`content`)

b) HOLD_PLACED_PRINT sounds like there is a connection to the existing HOLD_PLACED feature. If this is the case, should this be just the 'print' version of the HOLD_PLACED notice? And if not, should we rename to make the difference clear?

c) Notice is missing from the translated web installers. 

3) Templates
a) Icons need to be switched to Font Awesome. For example: koha-tmpl/intranet-tmpl/prog/en/includes/printers-toolbar.inc.

b) koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt changes the heading and description of one of the toolbar items back to the old one, should be: Z39.50/SRU searvers

c) I think page title/breadcrumbs and the new entry on Administration should match. Personally I'd prefer the simpler 'Printers' to 'Printer editor' as it's more in line with our other pages.

d) This would be nicer to translate using the newer syntax and a placeholder:
 else if (confirm(_("Are you sure you wish to delete printer(s) ") + idsToDelete + "?"))
See: Bug 12138 - Use placeholders in translatable javascript strings

4) Terminology 
Holds!! :)
koha-tmpl/intranet-tmpl/prog/en/modules/help/admin/printers.tt
print automatic reserve notices

5) Copyright
Is the copyright correct on the new file svc/printer?
+# Copyright 2014 ByWater Solutions
+# Copyright 2012 Foundations Bible College Inc.
Comment 60 Kyle M Hall 2015-12-03 15:32:37 UTC
(In reply to Jonathan Druart from comment #58)
> (In reply to Kyle M Hall from comment #24)
> > Test Plan addition:
> > There are several new module dependencies for this feature:
> > HTML::HTMLDoc: Required for both LPR and CUPS printing if printing HTML
> > Net::Printer: Required for LPR printing
> > Printer: Required for CUPS printing
> 
> Did you check how easy/difficult it is to package these 2 modules?
> 
> They don't seem maintained anymore
> Printer-0.98 	     17 Aug 2004
> HTML-HTMLDoc-0.11_01 03 Nov 2009

I have no experience packaging modules nor do I see any documentation on the Koha wiki.
Comment 61 Kyle M Hall 2015-12-03 15:33:02 UTC Comment hidden (obsolete)
Comment 62 Kyle M Hall 2015-12-03 16:05:46 UTC Comment hidden (obsolete)
Comment 63 Katrin Fischer 2015-12-03 16:08:27 UTC
Kyle, can you verify that the dependencies both need packaging?
Are there possible alternatives?
Comment 64 Katrin Fischer 2015-12-03 16:09:29 UTC
I am marking this blocked until we know more about the added dependencies.
Comment 65 Katrin Fischer 2015-12-03 16:16:29 UTC
(In reply to Kyle M Hall from comment #60)
> I have no experience packaging modules nor do I see any documentation on the
> Koha wiki.

http://wiki.koha-community.org/wiki/Building_Debian_Dependencies/Dependency_Guidelines
Comment 66 Kyle M Hall 2015-12-03 16:42:17 UTC
(In reply to Katrin Fischer from comment #64)
> I am marking this blocked until we know more about the added dependencies.

Based on the documentation here: http://wiki.koha-community.org/wiki/Building_Debian_Dependencies/Dependency_Guidelines

I ran "dh-make-perl --pkg-perl --build --cpan The::Module" for each of the new deps and it didn't generate any errors, so I think that means building the packages shouldn't be a problem!
Comment 67 Kyle M Hall 2015-12-03 16:55:18 UTC
Side note: this feature will also require the addition of the debian package htmldoc.
Comment 68 Galen Charlton 2015-12-07 17:54:30 UTC
Noting licenses of the proposed dependencies:

Net::Printer - BSD; GPL- and DFSG-compatible
Printer - Artistic License; not GPL-compatible, but DFSG-free. (Ancient module, though, and doesn't appear to be currently maintained).
HTML::HTMLDoc - Perl license. (Also ancient, doesn't appear to be currently maintained, and has 23 open issues on CPAN).

I wonder if Net::CUPS might be a better bet; it's currently packaged for Debian, but on the other hand, the Perl module doesn't appear to be maintained either. :/
Comment 69 Jonathan Druart 2015-12-08 09:36:44 UTC
Kyle,
For bug 8007, I have first used HTML::HTMLDoc too, but finally changed for PDF::FromHTML. Indeed HTML::HTMLDoc is buggy, see bug 8007 comment 64.
Have a loot at Koha::Borrower::Discharge::generate_as_pdf for an example on how to use it.
Comment 70 Katrin Fischer 2015-12-13 21:43:59 UTC
Moving this to FQA to get feedback from Kyle about the proposed alternatives.
Comment 71 Kyle M Hall 2015-12-14 14:46:57 UTC Comment hidden (obsolete)
Comment 72 Kyle M Hall 2015-12-14 14:47:06 UTC Comment hidden (obsolete)
Comment 73 Kyle M Hall 2015-12-14 14:47:09 UTC Comment hidden (obsolete)
Comment 74 Kyle M Hall 2015-12-14 14:47:13 UTC Comment hidden (obsolete)
Comment 75 Kyle M Hall 2015-12-14 14:47:16 UTC Comment hidden (obsolete)
Comment 76 Kyle M Hall 2015-12-14 14:47:20 UTC Comment hidden (obsolete)
Comment 77 Kyle M Hall 2015-12-14 14:47:23 UTC Comment hidden (obsolete)
Comment 78 Kyle M Hall 2015-12-14 14:48:56 UTC Comment hidden (obsolete)
Comment 79 Kyle M Hall 2015-12-14 14:51:14 UTC Comment hidden (obsolete)
Comment 80 Kyle M Hall 2015-12-14 14:56:55 UTC
(In reply to Katrin Fischer from comment #70)
> Moving this to FQA to get feedback from Kyle about the proposed alternatives.

I've switch to PDF::FromHTML due to the encoding and other issues related to HTML::HTMLDoc. I don't think there is a compelling reason to switch to Net::CUPS considering it is no more active than the currently used modules, and doesn't have very good documentation.
Comment 81 Kyle M Hall 2015-12-14 14:57:44 UTC
Note, the newest followup requires the package 'ghostscript' to be installed.
Comment 82 Kyle M Hall 2016-01-12 15:26:15 UTC
*** Bug 8034 has been marked as a duplicate of this bug. ***
Comment 83 Jonathan Druart 2016-01-19 14:31:42 UTC
fatal: sha1 information is lacking or useless (C4/Installer/PerlDependencies.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 8352 - Add printer administration, update script to use Koha defined printers
Comment 84 Kyle M Hall 2016-01-19 14:40:25 UTC Comment hidden (obsolete)
Comment 85 Kyle M Hall 2016-01-19 14:40:35 UTC Comment hidden (obsolete)
Comment 86 Kyle M Hall 2016-01-19 14:40:39 UTC Comment hidden (obsolete)
Comment 87 Kyle M Hall 2016-01-19 14:40:42 UTC Comment hidden (obsolete)
Comment 88 Kyle M Hall 2016-01-19 14:40:46 UTC Comment hidden (obsolete)
Comment 89 Kyle M Hall 2016-01-19 14:40:50 UTC Comment hidden (obsolete)
Comment 90 Kyle M Hall 2016-01-19 14:40:53 UTC Comment hidden (obsolete)
Comment 91 Jonathan Druart 2016-02-16 14:13:54 UTC
Created attachment 48127 [details] [review]
Bug 8352 - Add automatic printing of 'hold to pull' notices

Adds the ability to have a notice printed automatically
after a hold has been placed.

The system has the ability to define a separate printer
for each branchcode, so each library can receive a printed
notice for any holds placed, assuming they have set up the
ability to print over the LAN, WAN, or Internet. Notices are
printed to the pickup library.

Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>

bsoletes: 46882 - Bug 8352 [QA Followup]
Comment 92 Jonathan Druart 2016-02-16 14:14:00 UTC
Created attachment 48128 [details] [review]
Bug 8352 - Add printer administration, update script to use Koha defined printers

This patch adds the ability to define printers within Koha. The printer
CRUD is based on the code for the quote of the day system.

This patch extends the existing ability to print via LPR to any network
printer with the added ability to print to a CUPS printer configured on
the Koha server.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Browse to admin/printers.pl
4) Configure a network printer via LPR ( explained in the printers help )
5) Create a hold
6) Execute the command "misc/cronjobs/holds/print_holds.pl -v --printer $BRANCHCODE=$PRINTER_ID
   where branchcode is the pickup branch for the hold, and printer_id is
   the id number for the printer.
7) See your reserve slip is printer!

Bonus points: Repeat the test plan, but for a printer you set up via CUPS on the Koha server

Signed-off-by: Jenice Tate <jtate@mdah.state.ms.us>
Comment 93 Jonathan Druart 2016-02-16 14:14:05 UTC
Created attachment 48129 [details] [review]
Bug 8352 [QA Followup] - Fix column type in kohastructure.sql

Signed-off-by: Jenice Tate <jtate@mdah.state.ms.us>
Comment 94 Jonathan Druart 2016-02-16 14:14:10 UTC
Created attachment 48130 [details] [review]
Bug 8352 [QA Followup] - Don't use SQL in pl files

Signed-off-by: Jenice Tate <jtate@mdah.state.ms.us>
Comment 95 Jonathan Druart 2016-02-16 14:14:15 UTC
Created attachment 48131 [details] [review]
Bug 8352 [QA Followup]
Comment 96 Jonathan Druart 2016-02-16 14:14:21 UTC
Created attachment 48132 [details] [review]
Bug 8352 [QA Followup] - Notices for translation
Comment 97 Jonathan Druart 2016-02-16 14:14:25 UTC
Created attachment 48133 [details] [review]
Bug 8352 [QA Followup] - Remove use of HTML::HTMLDoc
Comment 98 Jonathan Druart 2016-02-16 14:15:57 UTC
Patches rebased, conflicts in notices sql files fixed!

QA comments:
1/ 
 FAIL   Koha/Printer.pm
   FAIL   critic
                "$pdf_data" is declared but not used at line 61, column 9. Unused variables clutter code and make it harder to read.

2/ What's the status of the NET::Printer lib? It's not packaged already.
Kyle, did you try to package it?

Failing QA for 1.
Comment 99 Jonathan Druart 2016-02-16 14:18:49 UTC
Created attachment 48134 [details] [review]
Bug 8352: Fix perlcritic '$pdf_data' is declared but not used
Comment 100 Jonathan Druart 2016-02-16 14:18:55 UTC
Created attachment 48135 [details] [review]
Bug 8352: Fix type vs _type for bug 15446
Comment 101 Jonathan Druart 2016-02-16 14:19:18 UTC
(In reply to Jonathan Druart from comment #98)
> Patches rebased, conflicts in notices sql files fixed!
> 
> QA comments:
> 1/ 
>  FAIL   Koha/Printer.pm
>    FAIL   critic
>                 "$pdf_data" is declared but not used at line 61, column 9.
> Unused variables clutter code and make it harder to read.

Ok fixed.
Comment 102 Jonathan Druart 2016-03-11 11:12:37 UTC
(In reply to Jonathan Druart from comment #98)
> 2/ What's the status of the NET::Printer lib? It's not packaged already.
> Kyle, did you try to package it?

Still waiting for an answer.
Comment 103 Kyle M Hall 2016-04-19 12:46:11 UTC
(In reply to Jonathan Druart from comment #102)
> (In reply to Jonathan Druart from comment #98)
> > 2/ What's the status of the NET::Printer lib? It's not packaged already.
> > Kyle, did you try to package it?
> 
> Still waiting for an answer.

I can tell you that dh-make-perl --pkg-perl --build --cpan Net::Printer produced a deb file with no apparent errors. However, without any good documentation on the package submission process, I really don't know where to go from there.
Comment 104 Chris Cormack 2016-04-19 19:57:25 UTC
You've read https://wiki.koha-community.org/wiki/Building_Debian_Dependencies  right?
Comment 105 Kyle M Hall 2016-04-20 10:24:16 UTC
(In reply to Chris Cormack from comment #104)
> You've read
> https://wiki.koha-community.org/wiki/Building_Debian_Dependencies  right?

Yep. I'm a bit lost on http://pkg-perl.alioth.debian.org/git.html . There is a bit of hand waving there, the section titled "building the package" has the most helpful guidance: Use your favourite way of building the Debian package.

This documentation is simply not clear not understandable. I don't claim to be the brightest bulb there is, but I'd like to think I can follow instructions when they are presented in a reasonable way ; )
Comment 106 Kyle M Hall 2016-04-20 10:25:23 UTC
Also, when I try to confirm my account on Alioth, all I get is a "user not found" error. So I can't even push the code to Alioth.
Comment 107 Chris Cormack 2016-04-20 10:51:17 UTC
Weird, working fine for me.

You've asked to be added to the group eh? 
http://pkg-perl.alioth.debian.org/
(Under A note on joining)

All the docs are on that page. You could try asking on the #debian-perl channel listed there. They are pretty receptive to helping

Or you could try to catch jajm on IRC he's usually closer to your timezone than I am. And has packaged https://tracker.debian.org/pkg/libtemplate-stash-autoescaping-perl
Comment 108 Kyle M Hall 2016-04-20 10:59:10 UTC
(In reply to Chris Cormack from comment #107)
> Weird, working fine for me.
> 
> You've asked to be added to the group eh? 
> http://pkg-perl.alioth.debian.org/
> (Under A note on joining)
> 
> All the docs are on that page. You could try asking on the #debian-perl
> channel listed there. They are pretty receptive to helping
> 
> Or you could try to catch jajm on IRC he's usually closer to your timezone
> than I am. And has packaged
> https://tracker.debian.org/pkg/libtemplate-stash-autoescaping-perl

Thanks! I figured out why I couldn't confirm my account ( all new accounts end with -guest and I didn't notice that ). I've requested to be added to the pkg-perl group. I'll check out the docs. Thanks again!
Comment 109 Katrin Fischer 2018-09-16 16:20:36 UTC
Are there any plans to revive/continue this?
Comment 110 Kyle M Hall 2018-09-21 11:17:07 UTC
(In reply to Katrin Fischer from comment #109)
> Are there any plans to revive/continue this?

I'm open to continuing work on this, but I don't know when I'll be able to pick back up on it.