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.

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)

(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.

(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.

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 ?

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.

(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)

(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.

(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!

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.

Hi Srdjan,
With your last patch I still can't update the queue value for an existing printer. (following my previous test plan)

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.

(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"

Ok, I stand corrected there. Queue value can be updated now.

I prefer someone else makes QA on this patch so I don't passed QA.

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.

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

(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.

(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.

(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.

(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.

(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.

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".

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

This work will certainly conflict with Bug 8352.

In that case let's pick one. I have no problems with this being obsoleted in favour of 8352.

(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 ***