Bug 8034

Summary: Enable server print queue selection
Product: Koha Reporter: Srdjan Jankovic <srdjan>
Component: CirculationAssignee: Srdjan Jankovic <srdjan>
Status: RESOLVED DUPLICATE QA Contact: Paul Poulain <paul.poulain>
Severity: enhancement    
Priority: P3 CC: gitbot, gmcharlt, jonathan.druart, josef.moravec, katrin.fischer, kyle.m.hall, kyle, mathsabypro, paul.poulain, veron
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8352
Change sponsored?: Sponsored Patch complexity: Medium patch
Who signed the patch off: Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 8038    
Attachments: patch
patch
bug_8034: Restored network printer maintenance and selection
bug_8034: Restored network printer maintenance and selection
Bug 8034: Restored network printer maintenance and selection
bug_8034: Restored network printer maintenance and selection
bug_8034: Restored network printer maintenance and selection
bug_8034: Restored network printer maintenance and selection
bug_8034: Restored network printer maintenance and selection
bug_5786, bug_5787, bug_5788: Holds rewrite
bug_8034: Restored network printer maintenance and selection
bug_8034: Restored network printer maintenance and selection
bug_8034: Disable delete printer for printers asigned to branches
bug_8034: Restored network printer maintenance and selection
bug_8034: Restored network printer maintenance and selection
bug_8034: Restored network printer maintenance and selection

Description Srdjan Jankovic 2012-05-01 05:20:14 UTC
Re-enable server print queue selection that was disabled due to being disfunctional.

Work to be done:
* Add system preference UsePrintQueues. Print queue selection will be enabled only if UsePrintQueues is true.
* Place admin/printers.pl link on the admin page. Should be visible UsePrintQueues dependent.
* Add branches.default_printqueue that will be used if not set in session
* Uncomment printer selection in "Set Library". Feature will be visible UsePrintQueues dependent.

A follow-up bug will be created for printing using the selection.

How to test:
* Check that UsePrintQueues syspref is available
* When UsePrintQueues is "No", check:
- "Printers" link on the "Admin" page is not present
- "Set Library" does not have printer selection
- branch maintenance does not have default printer queue selection
* When UsePrintQueues is "Yes", check:
- "Printers" link on the "Admin" page is present
- follow the link and add some printer queues
- branch maintenance does have default printer queue selection
- "Set Library" does have printer selection; select one
- selected printer sticks for the session

*
Comment 1 Srdjan Jankovic 2012-05-02 06:02:30 UTC Comment hidden (obsolete)
Comment 2 Jared Camins-Esakov 2012-05-13 13:13:34 UTC
The printer selection box still comes up, even when UsePrintQueues is set to "Don't use." Other than that, the patch seems to work fine.
Comment 3 Srdjan Jankovic 2012-05-14 01:03:02 UTC Comment hidden (obsolete)
Comment 4 Jared Camins-Esakov 2012-05-26 10:35:01 UTC Comment hidden (obsolete)
Comment 5 Paul Poulain 2012-06-09 11:06:09 UTC
QA comments:

 * I don't understand the need/use of 
+    my $userenv_branch = $userenv ? $userenv->{"branch"} : undef;
 why don't you use $userenv->{"branch"} ?
 * the sub 
+ =head2 get_user_printer
 must not be in C4/Auth.pm, but in C4/Print.pm (not a perfect option) or in Koha:: namespace (in the hackfest currently running, I've proposed a time to discuss of naming convention/organisation for Koha:: namespace)
 * same comment for sub GetPrinterDetails {, should not be in C4/Koha.pm

Other than this, the code looks OK (haven't tested it yet)
Comment 6 Srdjan Jankovic 2012-06-12 00:13:24 UTC
(In reply to comment #5)
> QA comments:
> 
>  * I don't understand the need/use of 
> +    my $userenv_branch = $userenv ? $userenv->{"branch"} : undef;
>  why don't you use $userenv->{"branch"} ?

I was not sure that $userenv is guaranteed to be there.

>  * the sub 
> + =head2 get_user_printer
>  must not be in C4/Auth.pm, but in C4/Print.pm (not a perfect option)

/() should not be in C4/Print.pm, because Print.pm is doing printing, not printer queue selection. get_user_printer() is a matter of session, that's why I placed it there (same as library selection). 

> or in
> Koha:: namespace (in the hackfest currently running, I've proposed a time to
> discuss of naming convention/organisation for Koha:: namespace)
>  * same comment for sub GetPrinterDetails {, should not be in C4/Koha.pm
> 
> Other than this, the code looks OK (haven't tested it yet)

I can move GetPrinter* functions to C4/Print.pm. I'd probably prefer to have C4/Printer.pm, but C4/Print.pm would do.
Comment 7 Paul Poulain 2012-06-13 11:03:28 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > QA comments:
> > 
> >  * I don't understand the need/use of 
> > +    my $userenv_branch = $userenv ? $userenv->{"branch"} : undef;
> >  why don't you use $userenv->{"branch"} ?
> 
> I was not sure that $userenv is guaranteed to be there.
as it's related to staff interface, you must have a $userenv available, otherwise there's a problem somewhere, and it must be fixed where it is.
> >  * the sub 
> > + =head2 get_user_printer
> >  must not be in C4/Auth.pm, but in C4/Print.pm (not a perfect option)
> 
> /() should not be in C4/Print.pm, because Print.pm is doing printing, not
> printer queue selection. get_user_printer() is a matter of session, that's
> why I placed it there (same as library selection). 
OK, got it.

> > or in
> > Koha:: namespace (in the hackfest currently running, I've proposed a time to
> > discuss of naming convention/organisation for Koha:: namespace)
> >  * same comment for sub GetPrinterDetails {, should not be in C4/Koha.pm
> > 
> > Other than this, the code looks OK (haven't tested it yet)
> 
> I can move GetPrinter* functions to C4/Print.pm. I'd probably prefer to have
> C4/Printer.pm, but C4/Print.pm would do.
mmm... I hesitate... I think I agree with you and the best would be to have a C4/Printer.pm.
Comment 8 Srdjan Jankovic 2012-07-11 02:21:07 UTC Comment hidden (obsolete)
Comment 9 Jared Camins-Esakov 2012-08-11 18:57:05 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2012-09-05 13:53:20 UTC
QA Comments:
It sounds good to me.

But the test you added (t/db_dependent/lib/KohaTest/Printer.pm) does not passed the perl -wc :
Invalid CODE attribute: Test( 1 ) at t/db_dependent/lib/KohaTest/Printer.pm line 24
BEGIN failed--compilation aborted at t/db_dependent/lib/KohaTest/Printer.pm line 24.

Are these tests still used ?
Comment 11 Srdjan Jankovic 2012-09-05 23:54:24 UTC Comment hidden (obsolete)
Comment 12 Srdjan Jankovic 2012-09-06 00:00:06 UTC
Patch did not apply, so I rebased.

I think I fixed that test. If not, please let me know.
Actually, I'm ashamed to admit it, but I don't know how to run those tests. I tried it but getting too many errors that I don't know what to do with.
I created that particular test file because I moved two functions from C4::Koha to C4::Printer. I wasn't worried about those test because they do not represent a huge value - just check ic certain subs are present, and UI would have failed anyway.
Can you please shed some light on how to run those tests.
Comment 13 Jonathan Druart 2012-09-06 09:56:22 UTC
(In reply to comment #12)

> Can you please shed some light on how to run those tests.

Hum... no :) I don't know either !
It seems these tests are not still used currently.
from irc :
 Joubu: Does someone know if unit tests in t/db_dependent/lib/KohaTest are still used ?
 rangi: no they aren't
 rangi: but if they could be made to work again, they would be
 rangi: you can never have too many tests

Some feedback about your patch:
1/ The syspref UsePrintQueues already exists and the updatedatabase will raise an error. I think the insert is useless.

2/ A link is missing in the left menu in the administration module (under Additional parameters)

3/ The edit action does not work (in my logs: use of uninitialized value $data[0] in concatenation (.) or string at /home/koha/C4/Printer.pm line 95)

4/ It would be possible to remote one click. Indeed after adding a printer we have to click on a useless 'ok' button. Can we redirect directly on the printer list page ?

5/ When I edit a library, the printer is not selected by default

6/ In header.inc it would be better to hide the '-' caracter if there is no printer defined for the current library.

7/ It would be great to automatically select (on the fly) the printer attached to the library we select (on the selectbranchprinter page)
Comment 14 Srdjan Jankovic 2012-09-07 02:00:08 UTC
(In reply to comment #13)

> 1/ The syspref UsePrintQueues already exists and the updatedatabase will
> raise an error. I think the insert is useless.

I could not see that. Can you please point me to the place where that was introduced earlier.

> 
> 2/ A link is missing in the left menu in the administration module (under
> Additional parameters)

I'm not sure what you mean by that. There's "Network Printers" link in "Additional parameters"

> 
> 3/ The edit action does not work (in my logs: use of uninitialized value
> $data[0] in concatenation (.) or string at /home/koha/C4/Printer.pm line 95)

That is just a warning. I can get rid of it if you wish. Edit worked nicely for me. What exactly happened?

> 
> 4/ It would be possible to remote one click. Indeed after adding a printer
> we have to click on a useless 'ok' button. Can we redirect directly on the
> printer list page ?

Sounds reasonable. Will do.

> 
> 5/ When I edit a library, the printer is not selected by default

I think it was a design decision, and makes sense. There could be a library without a printer. And even if there's a printer, I think it is better to have it blank initially rather than default to the first one. But if community has a different opinion, I can change that.

> 
> 6/ In header.inc it would be better to hide the '-' caracter if there is no
> printer defined for the current library.

Will do.

> 
> 7/ It would be great to automatically select (on the fly) the printer
> attached to the library we select (on the selectbranchprinter page)

Makes sense, will do that one too.
Comment 15 Jonathan Druart 2012-09-07 08:09:34 UTC
(In reply to comment #14)
> I could not see that. Can you please point me to the place where that was
> introduced earlier.

Last time I tested I had the syspref in my database, but now I don't find any occurrence on master.
So maybe I already tested this patch :)

> I'm not sure what you mean by that. There's "Network Printers" link in
> "Additional parameters"

In admin module (eg. admin/printers.pl) on the left-side.

> That is just a warning. I can get rid of it if you wish. Edit worked nicely
> for me. What exactly happened?

Just a warning but see C4/Printer l.95: $sth->execute("$data[0]%");
It could caused a problem if it is uninitialized :)
To reproduce:
- click on the "new printer" button
- fill with: "foo name", "foo queue", "foo type"
- submit, ok
- edit and change "foo queue" with "bar queue"
In fact, the issue only exists if I change the queue value.

> I think it was a design decision, and makes sense. There could be a library
> without a printer. And even if there's a printer, I think it is better to
> have it blank initially rather than default to the first one. But if
> community has a different opinion, I can change that.

Ok but if an user edit a library to change anything else, he erases the previous choice.
Maybe someone else could give his/her opinion on this?

> Sounds reasonable. Will do.
> Will do.
> Makes sense, will do that one too.
Thanks for your work!
Comment 16 Srdjan Jankovic 2012-09-10 01:56:00 UTC Comment hidden (obsolete)
Comment 17 Srdjan Jankovic 2012-09-10 01:58:09 UTC
I think I've sorted it out now. Changes are reasonably minor, so I'll leave it in Signed Off, but you can flip it to Needs Signoff if you wish.
Comment 18 Jonathan Druart 2012-09-10 12:56:49 UTC
Hi Srdjan,
With your last patch I still can't update the queue value for an existing printer. (following my previous test plan)
Comment 19 Srdjan Jankovic 2012-09-10 23:24:04 UTC
Printer queue value is the primary key for the table, and cannot be updated if the printer is assigned to a branch. mysql does not support ON UPDATE CASCADE I believe. It should leave a trail in the log files. It should eirher be documented, or printers should be given synthetic ids if that is a problem.
Comment 20 Jonathan Druart 2012-09-11 07:44:30 UTC
(In reply to comment #19)
> Printer queue value is the primary key for the table, and cannot be updated
> if the printer is assigned to a branch. mysql does not support ON UPDATE
> CASCADE I believe.

It seems it does: http://dev.mysql.com/doc/refman/5.5/en/innodb-foreign-key-constraints.html
"CASCADE: Delete or update the row from the parent table, and automatically delete or update the matching rows in the child table. Both ON DELETE CASCADE and ON UPDATE CASCADE are supported"
Comment 21 Srdjan Jankovic 2012-09-12 01:20:39 UTC Comment hidden (obsolete)
Comment 22 Srdjan Jankovic 2012-09-12 01:22:19 UTC
Ok, I stand corrected there. Queue value can be updated now.
Comment 23 Jonathan Druart 2012-09-12 08:30:35 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2012-09-12 08:32:20 UTC
I prefer someone else makes QA on this patch so I don't passed QA.
Comment 25 Srdjan Jankovic 2012-09-19 00:18:40 UTC Comment hidden (obsolete)
Comment 26 Jared Camins-Esakov 2012-10-06 04:19:04 UTC
Changing target version so this bug report will show up in the list of 3.12-targeted features and doesn't get lost in the shuffle during feature freeze.
Comment 27 Paul Poulain 2012-11-28 16:08:42 UTC
QA comment:
 * the patch does not apply anymore, there's a conflict on C4/Koha.pm. It seems to be easy to fix, but I let you do, to avoid a mistake
 * The DB update, you write "ALTER TABLE branches MODIFY branchprinter varchar(20) NULL, ADD FOREIGN KEY (branchprinter) REFERENCES printers (printqueue) ON UPDATE CASCADE" => why not also add ON DELETE SET NULL (on delete cascade would be a strong mistake, you don't want to delete branches just because you deleted a printer !!!) ?

No other comments yet
Comment 28 Srdjan Jankovic 2012-11-30 05:22:22 UTC Comment hidden (obsolete)
Comment 29 Chris Cormack 2013-01-02 01:50:05 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2013-02-07 13:21:49 UTC
(In reply to comment #27)
> QA comment:
>  * The DB update, you write "ALTER TABLE branches MODIFY branchprinter
> varchar(20) NULL, ADD FOREIGN KEY (branchprinter) REFERENCES printers
> (printqueue) ON UPDATE CASCADE" => why not also add ON DELETE SET NULL (on
> delete cascade would be a strong mistake, you don't want to delete branches
> just because you deleted a printer !!!) ?

This is relevant and there is no answer.

Marked as Failed QA.
Comment 31 Srdjan Jankovic 2013-02-08 02:52:19 UTC
(In reply to comment #27)
> QA comment:

>  * The DB update, you write "ALTER TABLE branches MODIFY branchprinter
> varchar(20) NULL, ADD FOREIGN KEY (branchprinter) REFERENCES printers
> (printqueue) ON UPDATE CASCADE" => why not also add ON DELETE SET NULL (on
> delete cascade would be a strong mistake, you don't want to delete branches
> just because you deleted a printer !!!) ?

Because we don't want to leave branches without their default printer, so only unused printers can be removed.
Comment 32 Jonathan Druart 2013-02-08 08:43:40 UTC
(In reply to comment #31)
> (In reply to comment #27)
> > QA comment:
> 
> >  * The DB update, you write "ALTER TABLE branches MODIFY branchprinter
> > varchar(20) NULL, ADD FOREIGN KEY (branchprinter) REFERENCES printers
> > (printqueue) ON UPDATE CASCADE" => why not also add ON DELETE SET NULL (on
> > delete cascade would be a strong mistake, you don't want to delete branches
> > just because you deleted a printer !!!) ?
> 
> Because we don't want to leave branches without their default printer, so
> only unused printers can be removed.

So you must catch the error raised by mysql if a printer is deleted and some branches use it.
Comment 33 Srdjan Jankovic 2013-02-08 08:57:37 UTC
(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #27)
> > > QA comment:
> > 
> > >  * The DB update, you write "ALTER TABLE branches MODIFY branchprinter
> > > varchar(20) NULL, ADD FOREIGN KEY (branchprinter) REFERENCES printers
> > > (printqueue) ON UPDATE CASCADE" => why not also add ON DELETE SET NULL (on
> > > delete cascade would be a strong mistake, you don't want to delete branches
> > > just because you deleted a printer !!!) ?
> > 
> > Because we don't want to leave branches without their default printer, so
> > only unused printers can be removed.
> 
> So you must catch the error raised by mysql if a printer is deleted and some
> branches use it.

I can do that if you insist, however current version does not do that, ie does not check anything at all. This is an improvement as is.
Comment 34 Jonathan Druart 2013-02-08 09:40:14 UTC
(In reply to comment #33)

> > So you must catch the error raised by mysql if a printer is deleted and some
> > branches use it.
> 
> I can do that if you insist, however current version does not do that, ie
> does not check anything at all. This is an improvement as is.

It is essential. The current version of this script is not reachable from any page.
Without this catch, I get an error:
 DBD::mysql::db do failed: Cannot delete or update a parent row: a foreign key constraint fails (`koha`.`branches`, CONSTRAINT `branches_ibfk_1` FOREIGN KEY (`branchprinter`) REFERENCES `printers` (`printqueue`) ON UPDATE CASCADE) at /home/koha/src/C4/Printer.pm line 144.
Comment 35 Srdjan Jankovic 2013-02-13 05:39:34 UTC Comment hidden (obsolete)
Comment 36 Srdjan Jankovic 2013-04-22 02:26:57 UTC Comment hidden (obsolete)
Comment 37 I'm just a bot 2013-09-18 09:53:13 UTC
Applying: bug_8034: Restored network printer maintenance and selection
Using index info to reconstruct a base tree...
M	C4/Auth.pm
M	C4/Koha.pm
M	admin/branches.pl
M	circ/circulation.pl
M	circ/returns.pl
M	installer/data/mysql/kohastructure.sql
M	installer/data/mysql/sysprefs.sql
M	installer/data/mysql/updatedatabase.pl
M	koha-tmpl/intranet-tmpl/prog/en/includes/header.inc
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/branches.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
M	koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/circ/selectbranchprinter.tt
A	t/db_dependent/lib/KohaTest/Koha.pm
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): t/db_dependent/lib/KohaTest/Koha.pm deleted in HEAD and modified in bug_8034: Restored network printer maintenance and selection. Version bug_8034: Restored network printer maintenance and selection of t/db_dependent/lib/KohaTest/Koha.pm left in tree.
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/selectbranchprinter.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/branches.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/branches.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/header.inc
Auto-merging installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging circ/returns.pl
Auto-merging circ/circulation.pl
Auto-merging admin/branches.pl
CONFLICT (content): Merge conflict in admin/branches.pl
Auto-merging C4/Koha.pm
Auto-merging C4/Auth.pm
CONFLICT (content): Merge conflict in C4/Auth.pm
Patch failed at 0001 bug_8034: Restored network printer maintenance and selection
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 38 Srdjan Jankovic 2016-01-08 06:13:46 UTC Comment hidden (obsolete)
Comment 39 Srdjan Jankovic 2016-01-08 06:14:35 UTC
Created attachment 46415 [details] [review]
bug_8034: Restored network printer maintenance and selection

This patch is just for restoring printer maintenance and selection, not
for priting itself. It is just a preparation step.

* Added UsePrintQueues syspref. If set to No, no printer info will be
  displayed/used
* Database changes:
- printers table PRIMARY KEY is now printqueue. It is more natural. We
  should really have a synthetic id, but printqueue is good enough
- branches.branchprinter is a FOREIGN KEY to printers.printqueue
* Created C4::Auth::get_user_printer() function that will return
  appropriate printer. In order of preference:
- session selected
- logged in branch branchprinter
* Moved printer functions to C4::Printer
* C4::Printer - added branch count to printer queries
* admin/printers.tt - show or not delete link
* Restored printer maint/selection in admin zone UsePrintQueues
  permitting
* Restored printer selection in circ/selectbranchprinter.pl
  UsePrintQueues permitting
Comment 40 Kyle M Hall 2016-01-08 19:22:23 UTC
This work will certainly conflict with Bug 8352.
Comment 41 Srdjan Jankovic 2016-01-11 00:25:08 UTC
In that case let's pick one. I have no problems with this being obsoleted in favour of 8352.
Comment 42 Kyle M Hall 2016-01-12 15:26:15 UTC
(In reply to Srdjan Jankovic from comment #41)
> In that case let's pick one. I have no problems with this being obsoleted in
> favour of 8352.

Excellent! Thanks!

*** This bug has been marked as a duplicate of bug 8352 ***