Bug 7001 - User Configurable Slips
Summary: User Configurable Slips
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 3.8
Hardware: All All
: P3 enhancement (vote)
Assignee: Srdjan Jankovic
QA Contact: Bugs List
URL:
Keywords:
: 2349 3246 (view as bug list)
Depends on: 3150 6485 7467 8259
Blocks: 9081
  Show dependency treegraph
 
Reported: 2011-10-11 03:51 UTC by Srdjan Jankovic
Modified: 2015-04-01 10:59 UTC (History)
12 users (show)

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


Attachments
patch (141.45 KB, patch)
2011-10-24 06:25 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (143.66 KB, patch)
2011-10-25 00:47 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (143.89 KB, patch)
2011-11-21 01:01 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (144.21 KB, patch)
2011-12-19 01:57 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (144.37 KB, patch)
2011-12-28 03:15 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (144.37 KB, patch)
2012-01-05 01:17 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (145.98 KB, patch)
2012-01-05 01:51 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug 7001 - fix errant ) that broke updatedatabase (1.31 KB, patch)
2012-01-09 15:40 UTC, Liz Rea
Details | Diff | Splinter Review
Sample Custom CSS file (998 bytes, text/plain)
2012-01-09 22:54 UTC, Liz Rea
Details
patch (150.09 KB, patch)
2012-01-12 08:28 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (151.76 KB, patch)
2012-01-13 04:01 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (152.15 KB, patch)
2012-01-13 07:44 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (152.23 KB, patch)
2012-01-16 07:01 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (150.67 KB, patch)
2012-01-23 03:41 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (147.86 KB, patch)
2012-01-27 01:50 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[Signed Off] bug_7001: Issue and Reserve slips are notices. (148.87 KB, patch)
2012-02-01 19:21 UTC, Liz Rea
Details | Diff | Splinter Review
no list of branches (125.33 KB, image/png)
2012-02-10 19:04 UTC, Paul Poulain
Details
follow-up patch (1.20 KB, patch)
2012-02-16 02:56 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (148.04 KB, patch)
2012-02-21 04:56 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (148.04 KB, patch)
2012-02-22 05:52 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_7001: Issue and Reserve slips are notices. (148.87 KB, patch)
2012-02-22 15:29 UTC, Liz Rea
Details | Diff | Splinter Review
patch (6.32 KB, patch)
2012-02-29 01:09 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (151.82 KB, patch)
2012-03-09 00:28 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
follow-up patch fix protected letters (1.46 KB, patch)
2012-03-12 00:06 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 7167 Followup FIX conflicts with Bug 5347 (5.81 KB, patch)
2012-03-13 09:04 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 Srdjan Jankovic 2011-10-11 03:51:14 UTC
Work To Be Done:
* Hold notification slips and Checkout receipts brought under the Notices
editor
* Associate Notices/Slips with branches, and include a fallback to 'default'
notice type, so there can be both a standard notice layout, and branch-
specific ones
  -  Provide pull down to choose branch and then have
notices under each branch like with circ/fine rules
  -  Use IndependentBranches to prevent edits to notices of other
branches
* Parsing of patron extended attributes into notices
* Allow for notices/slips to be created in and printed with HTML
  -  Right now emails are sent only in plain text
  -  This can either be an automatic process, the script identifies when
the notice is in HTML, or it can be a choice on notice creation (HTML
or Plain Text)
* Ability to include/link/otherwise associate a CSS file with HTML
notices/slips
  -  New system preference NoticeCSS
  -  New system preference SlipCSS

To Test:
* Test on a system with at least two branches
* Create a CSS file for the NoticeCSS
* Create a CSS file for the SlipCSS
* Create a patron at Branch A (Patron A)
  -  Choose to receive at least two enhanced messages (one should be a
hold filled)
  -  Enter at least one extended patron attribute
* Create patron at Branch B (Patron B)
  -  Choose to receive at least two enhanced messages (one should be a
hold filled)
  -  Enter at least one extended patron attribute
* Create at least two notices for each branch (one should be hold filled)
  -  Include branch specific fields as well as information on items and
the borrower
  -  Make sure there are some fields that are different in each notice
  -  Make sure one from each branch has HTML code in it
* Create at least two slips for each branch
  -  One hold slip
  -  Include branch specific fields as well as information on items and
the borrower (include at least one extended patron attribute)
  -  One branch should be in HTML and the other plain text
  -  One checkout receipt
  -  Include branch specific fields as well as information on items and
the borrower (include at least one extended patron attribute)
  -  One branch should be in HTML and the other plain text
* Check out one item to Patron A at Branch A
  -  Print a slip for that check out
  -  Confirm that data is parsed correctly
  -  Print a quick slip for that check out
  -  Confirm that data is parsed correctly
  -  Print a page for that check out
  -  Confirm that data is parsed correctly
* Check out another item to Patron A at Branch A
  -  Print a slip for that check out
  -  Confirm that data is parsed correctly
  -  Print a quick slip for that check out
  -  Confirm that data is parsed correctly
  -  Print a page for that check out
  -  Confirm that data is parsed correctly
* Check out one item to Patron B at Branch B
  -  Print a slip for that check out
  -  Confirm that data is parsed correctly
  -  Print a quick slip for that check out
  -  Confirm that data is parsed correctly
  -  Print a page for that check out
  -  Confirm that data is parsed correctly
* Check out another item to Patron B at Branch B
  -  Print a slip for that check out
  -  Confirm that data is parsed correctly
  -  Print a quick slip for that check out
  -  Confirm that data is parsed correctly
  -  Print a page for that check out
  -  Confirm that data is parsed correctly
* Place a hold for Patron A for pick up at Branch A
* Place a hold for Patron B for pick up at Branch B
* Check in an item on hold from Branch A
  -  Print the hold slip
  -  Confirm that the data is parsed correctly
* Check in an item on hold from Branch B
  -  Print the hold slip
  -  Confirm that the data is parsed correctly
* Check both patrons’ email to be sure that the hold filled email parsed
properly
* Trigger the sending of at least one other notice and confirm that that
email is parsed properly as well
Comment 1 Srdjan Jankovic 2011-10-24 06:25:48 UTC Comment hidden (obsolete)
Comment 2 Paul Poulain 2011-10-24 11:38:13 UTC
Updating Version : This ENH will be for Koha 3.8
Comment 3 Srdjan Jankovic 2011-10-25 00:47:18 UTC Comment hidden (obsolete)
Comment 4 Paul Poulain 2011-10-25 15:05:47 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 5 Brendan Gallagher 2011-11-05 06:11:15 UTC
Testing.

1. All of the previously created notices now appear as blank when trying to edit them.  
2. Can not switch between branches - once you switch to one - you can not switch back to another
3. When clicking on the edit button the notice is completely blank.
4. After editing and placing information into the ISSUESLIP - If I click edit - then the notice is blank
5. Notices are in the database.
| circulation      | ISSUEQSLIP  | NULL       | Issue Quick Slip                         |       1 | Issue Quick Slip                                     | <h3><<branches.branchname>></h3>
Checked out to <<borrowers.title>> <<borrowers.firstname>> <<borrowers.initials>> <<borrowers.surname>> <br />
(<<borrowers.cardnumber>>) <br />

<<today>><br />

<h4>Checked Out Today</h4>
<checkedout>
<p>
<<biblio.title>> <br />
Barcode: <<items.barcode>><br />
Date due: <<issues.date_due>><br />
</p>
</checkedout>                                                                                                                                                                                                                                                                                                                     

6. when testing the printing of the notices - they all appear blank and do not work.


I did use git bz to apply the patch on 3.6

-Brendan
Comment 6 Srdjan Jankovic 2011-11-07 03:09:32 UTC
I have it on the latest master and it works fine. Can you please try it there.
Comment 7 Brendan Gallagher 2011-11-11 20:06:12 UTC
Sorry Srdjan -

It does not work on Master.  Please see all of my comments.  Did I miss a updatedatabase call?

-Brendan
Comment 8 Srdjan Jankovic 2011-11-14 04:30:37 UTC
Yes, there's a bunch of SQL statements in installer/data/mysql/updatedatabase.pl for 3.06.00.XXX. I am not sure how is that handled on merge, ie does that get run as auto update, or you need to run it manually.
Comment 9 Ian Walls 2011-11-19 15:27:47 UTC
Srdjan,


This patch does not apply to current master.  There is a conflict on acqui/booksellers.pl.  I've tried regular and three-way merge, and both fail.
Comment 10 Srdjan Jankovic 2011-11-21 01:01:52 UTC Comment hidden (obsolete)
Comment 11 Liz Rea 2011-12-16 18:10:12 UTC
Applying: bug_7001: Issue and Reserve slips are notices.
/home/liz/kohaclone/.git/rebase-apply/patch:700: trailing whitespace.
   
/home/liz/kohaclone/.git/rebase-apply/patch:1034: trailing whitespace.
ITEM RESERVED: 
/home/liz/kohaclone/.git/rebase-apply/patch:2073: trailing whitespace.
    [% UNLESS independant_branch %]        
/home/liz/kohaclone/.git/rebase-apply/patch:2086: trailing whitespace.
    [% END %]        
/home/liz/kohaclone/.git/rebase-apply/patch:2135: trailing whitespace.
        [% END %]        
error: short SHA1 2ba7584 is ambiguous.
fatal: sha1 information is lacking or useless (C4/Print.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 bug_7001: Issue and Reserve slips are notices.


I ran the following (patch was downloaded from bz with git bz): 
git am -3 /tmp/patch-OMWdbx.patch
Comment 12 Srdjan Jankovic 2011-12-19 01:57:32 UTC Comment hidden (obsolete)
Comment 13 Melia Meggs 2011-12-27 21:42:59 UTC
I think this patch still does not apply.  At least not for me...

When I did git bz apply:

Applying: bug_7001: Issue and Reserve slips are notices.
/home/melia/kohaclone/.git/rebase-apply/patch:703: trailing whitespace.
   
/home/melia/kohaclone/.git/rebase-apply/patch:1037: trailing whitespace.
ITEM RESERVED: 
/home/melia/kohaclone/.git/rebase-apply/patch:2076: trailing whitespace.
    [% UNLESS independant_branch %]        
/home/melia/kohaclone/.git/rebase-apply/patch:2089: trailing whitespace.
    [% END %]        
/home/melia/kohaclone/.git/rebase-apply/patch:2138: trailing whitespace.
        [% END %]        
error: koha-tmpl/intranet-tmpl/prog/en/modules/circ/printslip.tt: already exists in working directory
error: members/printslip.pl: already exists in working directory
error: patch failed: misc/cronjobs/overdue_notices.pl:643
error: misc/cronjobs/overdue_notices.pl: patch does not apply
Patch failed at 0001 bug_7001: Issue and Reserve slips are notices.

When I did git am -iu3:

Applying: bug_7001: Issue and Reserve slips are notices.
error: short SHA1 2ba7584 is ambiguous.
fatal: sha1 information is lacking or useless (C4/Print.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 bug_7001: Issue and Reserve slips are notices.
Comment 14 Chris Cormack 2011-12-27 21:45:16 UTC
I confirm, this doesn't apply
Comment 15 Srdjan Jankovic 2011-12-28 03:15:56 UTC Comment hidden (obsolete)
Comment 16 Chris Cormack 2011-12-28 03:18:28 UTC
New patch applies fine, should be ok to test now
Comment 17 Liz Rea 2012-01-04 21:19:34 UTC
when I did the updatedatabase, I had this problem: 

[Wed Jan 4 14:36:39 2012] updatedatabase.pl: DBD::mysql::db do failed: Error on rename of './kohadev/#sql-17c0_3f9b4' to './kohadev/letter' (errno: 150) at /home/liz/kohaclone/installer/data/mysql/updatedatabase.pl line 4589.
[Wed Jan 4 14:36:39 2012] updatedatabase.pl: DBD::mysql::db do failed: Multiple primary key defined at /home/liz/kohaclone/installer/data/mysql/updatedatabase.pl line 4591.
[Wed Jan 4 14:36:39 2012] updatedatabase.pl: DBD::mysql::db do failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')' at line 1 at /home/liz/kohaclone/installer/data/mysql/updatedatabase.pl line 4678.

I took a cursory look to see if I could figure it out, but I didn't see anything obvious.

Thoughts?
Comment 18 Srdjan Jankovic 2012-01-05 01:17:17 UTC Comment hidden (obsolete)
Comment 19 Srdjan Jankovic 2012-01-05 01:51:05 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2012-01-06 15:51:08 UTC
*** Bug 2349 has been marked as a duplicate of this bug. ***
Comment 21 Liz Rea 2012-01-09 15:40:33 UTC Comment hidden (obsolete)
Comment 22 Liz Rea 2012-01-09 15:41:05 UTC
Srdjan, I found a small problem in updatedatabase.pl (errant )), attached patch should fix it. Otherwise, patch application and update went fine this time - thanks! 

We are going to step through your test plan today - any suggestions for what we should put in our NoticeCSS/SlipCSS files that you specifically want tested?

(It's easy enough to reverse engineer what should go in those files, but I wonder if you have an example css file we could use for testing? It's ok if you don't - I can make one and attach it to this bug.)
Comment 23 Liz Rea 2012-01-09 22:50:56 UTC
A few notes:

* I don't appear to see the NoticeCSS syspref in my DB, but updatedatabase didn't give me any issue.
* Cannot delete a notice once it is created: http://screencast.com/t/mvKxllXJR3
* select a library -> add notice works differently from how I'd expect. I would expect that if I selected a library from the dropdown and clicked "add new" that the add notice screen would autopop the library I was looking at on the previous page as the library I'm making this notice for: http://screencast.com/t/X10BqdAUW
* docs will need to be updated :)
* It might be nice to have a cancel as well as a submit button on the edit notice screen.
* Copy notice works GREAT. Love that.
* What determines whether a notice can or can't be deleted? http://screencast.com/t/qA83PkRnCjTF

This is as far as I got today - more tomorrow.
Comment 24 Liz Rea 2012-01-09 22:54:45 UTC
Created attachment 7099 [details]
Sample Custom CSS file

I forgot to add that SlipCSS is in there, and I was testing it with the attached CSS file - this is no different from the original styling, but could be modified any old way for testing (cursive font, BOLD ITALIC everything, whatever).

The brief testing I did with it showed that changes in this CSS were in fact reflected in the slips = sweeet.
Comment 25 Liz Rea 2012-01-10 15:06:55 UTC
apologies - I found the NoticeCSS - it was under my nose and I didn't see it. Sorry!
Comment 26 Liz Rea 2012-01-10 21:32:35 UTC
More notes:

* "Notices" on the Tools page should probably be "Notices and Slips" or "Notices and Receipts."
* News items for slips are still global, but not in the scope of this dev. Needs to be fixed, just making a note.
* WORKS: Copy notice - works like I expect re: library selection, nice.
* Which code, if any, is used for the "Print Page" template?
* it would be nice to have an explicit indication of which entries are a "notice" and which are "slips" - I know it should be clear from the code name, but it could be clearer.
* How do you add the borrower extended attribute to the notice? I don't see it under the Borrowers section.
* Preview slip/notice might be nice, but probably beyond the scope of this dev. Just making a note.
* I wonder about the utility of "New Notice" at all - if we can only define notices within the codes in the default data (is that true?) then copying them from the defaults is probably the easiest way to go. Or, make the code field a drop down to select the type of notice so one doesn't mess up and put a quick slip under the cataloging module (like I did in my testing because I wasn't paying attention).
* WORKS: Checkout slips and quick slips work both in html mode and plain text mode for multiple branches. 

On to email notices!
Comment 27 Srdjan Jankovic 2012-01-11 04:11:28 UTC
There's a mysterious table message_transports. If letter code is in that table it is considered "protected". I am not familiar with logic behind that.

If system has patron attributes switched on, those should appear in the fields list for pick up as borrower_attribute.*

Also, we should compile a list of remarks and possibly make corrections for the inportant ones immediately, and leave the rest as later enhancements
Comment 28 Katrin Fischer 2012-01-11 05:20:33 UTC
>* I wonder about the utility of "New Notice" at all - if we can only define
>notices within the codes in the default data (is that true?) then copying them
>from the defaults is probably the easiest way to go. Or, make the code field a
>drop down to select the type of notice so one doesn't mess up and put a quick
>slip under the cataloging module (like I did in my testing because I wasn't
>paying attention).

Hi Liz, defining your own notices can make sense. One example is overdues where you tell the notice triggers which notices to use. It's not relying on a special code there. And there are some other places where it only relies on the correct module (acq and serials).
Comment 29 Liz Rea 2012-01-11 17:11:00 UTC
Thanks Katrin - I didn't realize that you could specify a code for overdue notices. That helps. A drop down still appeals to me - but it would have to include "Other" that lets you set the code name manually. That's probably beyond the scope of this, I'll let Srdjan decide that.

Srdjan:
These are the important ones:

Broken things:
* Cannot delete a notice/slip once it is created: http://screencast.com/t/mvKxllXJR3

User experience things:
* select a library -> add notice works differently from how I'd expect. I would
expect that if I selected a library from the dropdown and clicked "add new"
that the add notice screen would autopop the library I was looking at on the
previous page as the library I'm making this notice for:
http://screencast.com/t/X10BqdAUW

* It might be nice to have a cancel as well as a submit button on the edit
notice screen.

* "Notices" on the Tools page should probably be "Notices and Slips" or
"Notices and Receipts."

* it would be nice to have an explicit indication of which entries are a
"notice" and which are "slips" - I know it should be clear from the code name,
but it could be clearer.

Questions:
* Which code, if any, is used for the "Print Page" template?
Comment 30 Srdjan Jankovic 2012-01-12 08:28:45 UTC Comment hidden (obsolete)
Comment 31 Liz Rea 2012-01-12 17:35:32 UTC
Awesome - checking these things now and going on with more testing!
Comment 32 Liz Rea 2012-01-12 21:27:25 UTC
Broken things;
* the left hand menu has gone funny: http://screencast.com/t/FJMu0cI940Xm on every page in this module.
* I'm still not seeing the borrower attributes, maybe I'm misunderstanding where they should be - http://screencast.com/t/Aioq5Wb7M

User Experience things:
Nothing today :)

Confirmed fixed things:
* selected library is now passed to "new notice" form (yay!)
* Delete notice/slip is now working (yay!)
* I see the cancel button, though it is underneath the submit button. May not matter but isn't consistent with the rest of Koha - they are usually side by side. Can be cleaned up later.

Confirmed working:
* CHECKOUT notices work in both HTML and non-HTML incarnations, for multiple branches.


Extra comments:
Comment 33 Liz Rea 2012-01-12 21:47:43 UTC
Question: 
Where does Koha get the branches.branchname from for the notices? The home library of the logged in user or the set user?

I have observed that even with a set library of Springfield, the notices generated all claim to be from Midway (the home library of the logged in user).

I submit that the notices should use the set library, and not the home library of the logged in user - does that make sense?
Comment 34 Srdjan Jankovic 2012-01-13 04:01:45 UTC Comment hidden (obsolete)
Comment 35 Katrin Fischer 2012-01-13 05:18:51 UTC
I agree with Liz that for checkout and hold slips and basically all slips that are printed while working at the circulation desk the 'set library' is the one we want.
Comment 36 Srdjan Jankovic 2012-01-13 07:44:44 UTC Comment hidden (obsolete)
Comment 37 Liz Rea 2012-01-13 15:48:07 UTC
Srdjan,

Thanks for the quick fixes - will look at it again today.

Re: the notice pulling from home branch of logged in user - I will file a bug for that - though admittedly, for the notices, it's *probably* ok as the staff member that is logged in *should* have a home branch of the library they're standing in. I can attest, in practical usage, that it *is* a possibility to have a set branch and a home library that are different (rotating staff, for example). At the very least it needs to be documented that it is that way.

Another way to look at it would be to make sure the set library follows the home library of the logged in staff patron - change one you change them both. But that's not for this bug. 

Thanks for fixing the slips though, it would be "very bad (TM)" to print a slip at midway that said springfield on it. :)
Comment 38 Liz Rea 2012-01-13 16:46:50 UTC
Hmm, I'm getting an error when printing slips now:

Software error:

Undefined subroutine &main::get_session called at /home/liz/kohaclone/members/printslip.pl line 55.
Comment 39 Srdjan Jankovic 2012-01-16 07:01:46 UTC Comment hidden (obsolete)
Comment 40 Liz Rea 2012-01-17 17:31:07 UTC
Confirmed fixed:
* no more software error on print slip/quick slip
* slips now take their branch from set branch instead of home branch of logged in patron (thanks!)

Moving on with the rest of the testing :)
Comment 41 Liz Rea 2012-01-17 22:10:20 UTC
Broken things:
* upon further review, it looks like the notices aren't coming from the branch of the logged in user OR the set library. Check this out:

patron I was working with: http://screencast.com/t/6fzeSxMz Set library springfield, home library springfield.

Checking out an item for that patron gives this: http://screencast.com/t/rvZwLGdOS

Here's what's in my notice: http://screencast.com/t/f58vhwXmc

There are a number of ways this is right: it's using the default "all libraries" template, as I do not have a checkout notice configured for either midway or springfield. It's got the right item data in there. Just that pesky "which branch is it from" problem.

Check-in for a specific library seems to work correctly: http://screencast.com/t/SSNSgMaYw perhaps it's only a problem with the default notice(s)?
Comment 42 Srdjan Jankovic 2012-01-18 00:46:26 UTC
From what I can see it's been like that since 2009. This is the branch selection sub:

sub _GetCircControlBranch {
    my ($item, $borrower) = @_;
    my $circcontrol = C4::Context->preference('CircControl');
    my $branch;

    if ($circcontrol eq 'PickupLibrary') {
        $branch= C4::Context->userenv->{'branch'} if C4::Context->userenv;
    } elsif ($circcontrol eq 'PatronLibrary') {
        $branch=$borrower->{branchcode};
    } else {
        my $branchfield = C4::Context->preference('HomeOrHoldingBranch') || 'homebranch';
        $branch = $item->{$branchfield};
        # default to item home branch if holdingbranch is used
        # and is not defined
        if (!defined($branch) && $branchfield eq 'holdingbranch') {
            $branch = $item->{homebranch};
        }
    }
    return $branch;
}
Comment 43 Liz Rea 2012-01-18 18:27:31 UTC
oh snap it's circ control - that muddies things up a bit.

Hm. So. We have a bit of a problem here. 

Where do we want the notices to come from? Is Circ Control preferred (it totally may be). Does that still make sense when notices are configurable by branch?

Let me do a bit more testing - my preferences may not be set up in a way that exactly mirrors what we're using in our production (which of course I'm sure is a "mad scheme" - can't get away from quote 123. Sigh.)  which may change the outcome of the testing somewhat (in a good way).
Comment 44 Liz Rea 2012-01-18 18:59:27 UTC
You will be happy to know that with CircControl set to "the library you are logged in at" the notices all work the way one would expect.

This also means that for other values of CircControl, they work properly too.

So, that puts "wrong branch in notices" squarely in the "fixed" column.

Fixed: 
* branch is correct on notices (dependent on CircControl, that will need to be documented).

:)
Comment 45 Liz Rea 2012-01-19 23:01:36 UTC
Looking at regular notices now - 

To confirm: 
* if a patron has no email preferences defined, they get a -print notice for things like "hold filled," correct? And that's the only notice that has a -print counterpart, correct?
* Is this how the HTML notices are supposed to come out? http://screencast.com/t/9MjKxFurdLdM It looks odd to me. 

Confirmed working: 
* Hold-Print notice works for every branch
* Reserve slips appear to be working as intended.


ran out of time today, will continue tomorrow...
Comment 46 Liz Rea 2012-01-20 21:59:06 UTC
Passes prove t t/db_dependent 

but

prove xt gives the following: 
xt/permissions.t .. ok    
xt/tt_valid.t ..... 1/1 
#   Failed test 'TT syntax: not using TT directive within HTML tag'
#   at xt/tt_valid.t line 55.
# Files list: 
# intranet-tmpl/prog/en/modules/tools/letter.tt: 299
# Looks like you failed 1 test of 1.
xt/tt_valid.t ..... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests 

Test Summary Report
-------------------
xt/tt_valid.t   (Wstat: 256 Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
Files=2, Tests=27,  8 wallclock secs ( 0.06 usr  0.02 sys +  8.40 cusr  0.06 csys =  8.54 CPU)
Result: FAIL
Comment 47 Liz Rea 2012-01-20 22:57:29 UTC
urg, and the changes I fetched today, this patch will not apply on. argle bargle bargle.

Aside: you must be the most patient person alive. Thanks for that.


liz@koha-deb:~/kohaclone$ git am -3 /tmp/patch-ikn6Jg.patch
Applying: bug_7001: Issue and Reserve slips are notices.
/home/liz/kohaclone/.git/rebase-apply/patch:705: trailing whitespace.
   
/home/liz/kohaclone/.git/rebase-apply/patch:1068: trailing whitespace.
ITEM RESERVED: 
/home/liz/kohaclone/.git/rebase-apply/patch:2181: trailing whitespace.
    [% UNLESS independant_branch %]        
/home/liz/kohaclone/.git/rebase-apply/patch:2191: trailing whitespace.
    [% END %]        
/home/liz/kohaclone/.git/rebase-apply/patch:2241: trailing whitespace.
        [% END %]        
warning: squelched 2 whitespace errors
warning: 7 lines add whitespace errors.
Using index info to reconstruct a base tree...
<stdin>:705: trailing whitespace.
   
<stdin>:1068: trailing whitespace.
ITEM RESERVED: 
<stdin>:2181: trailing whitespace.
    [% UNLESS independant_branch %]        
<stdin>:2191: trailing whitespace.
    [% END %]        
<stdin>:2241: trailing whitespace.
        [% END %]        
warning: squelched 4 whitespace errors
warning: 7 lines applied after fixing whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging C4/Circulation.pm
Auto-merging C4/Letters.pm
CONFLICT (content): Merge conflict in C4/Letters.pm
Auto-merging installer/data/mysql/de-DE/mandatory/sample_notices.sql
Auto-merging installer/data/mysql/en/mandatory/sample_notices.sql
Auto-merging installer/data/mysql/es-ES/mandatory/sample_notices.sql
Auto-merging installer/data/mysql/fr-FR/1-Obligatoire/sample_notices.sql
Auto-merging installer/data/mysql/it-IT/necessari/notices.sql
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging installer/data/mysql/nb-NO/1-Obligatorisk/sample_notices.sql
Auto-merging installer/data/mysql/pl-PL/mandatory/sample_notices.sql
Auto-merging installer/data/mysql/ru-RU/mandatory/sample_notices.sql
Auto-merging installer/data/mysql/uk-UA/mandatory/sample_notices.sql
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Removing koha-tmpl/intranet-tmpl/prog/en/modules/circ/hold-transfer-slip.tt
Removing koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember-receipt.tt
Auto-merging tools/letter.pl
Failed to merge in the changes.
Patch failed at 0001 bug_7001: Issue and Reserve slips are notices.
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
Comment 48 Srdjan Jankovic 2012-01-23 03:41:07 UTC Comment hidden (obsolete)
Comment 49 Liz Rea 2012-01-23 19:03:07 UTC
Okies, patch applied today.

Broken things:
* no drop-down to select libraries
   - this may be intentional - I noticed that "new notice" now creates a notice for the set branch (hey! Cool!) but it's not super clear in the interface that you are creating the notice for the set library. Maybe only a user experience thing.
* Related to above: new notice does not allow library selection - http://screencast.com/t/xFd1lKcABxAc
* no edit link anymore for the "default" notices.
* no delete (but possibly not relevant?) - http://screencast.com/t/RDDkTAcMAb

Will play around with functionality here in a few but didn't want to lose these comments.
Comment 50 Srdjan Jankovic 2012-01-23 23:23:44 UTC
That does not sound right, as if the patch was not applied. Can you please double check that.
Comment 51 Liz Rea 2012-01-24 15:21:11 UTC
Hmm it said it applied ok. And the display is different before and after the patch. 

I will check it again.
Comment 52 Liz Rea 2012-01-24 15:35:43 UTC
You were right - sorry about that! 

http://screencast.com/t/eOkFc10Euae

I know that when I tried it yesterday it applied ok, but clearly something was amiss - today when I redid it on a fresh fetch I got the following: 


Applying: bug_7001: Issue and Reserve slips are notices.
/home/liz/kohaclone/.git/rebase-apply/patch:410: trailing whitespace.
    }    
/home/liz/kohaclone/.git/rebase-apply/patch:411: trailing whitespace.
   # send an "account details" notice to a newly created user 
/home/liz/kohaclone/.git/rebase-apply/patch:647: trailing whitespace.
   
/home/liz/kohaclone/.git/rebase-apply/patch:1010: trailing whitespace.
ITEM RESERVED: 
/home/liz/kohaclone/.git/rebase-apply/patch:2124: trailing whitespace.
    [% UNLESS independant_branch %]        
warning: squelched 4 whitespace errors
warning: 9 lines add whitespace errors.
Using index info to reconstruct a base tree...
<stdin>:410: trailing whitespace.
    }    
<stdin>:411: trailing whitespace.
   # send an "account details" notice to a newly created user 
<stdin>:647: trailing whitespace.
   
<stdin>:1010: trailing whitespace.
ITEM RESERVED: 
<stdin>:2124: trailing whitespace.
    [% UNLESS independant_branch %]        
warning: squelched 6 whitespace errors
warning: 9 lines applied after fixing whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging installer/data/mysql/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Removing koha-tmpl/intranet-tmpl/prog/en/modules/circ/hold-transfer-slip.tt
Removing koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember-receipt.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/tools-home.tt
Failed to merge in the changes.
Patch failed at 0001 bug_7001: Issue and Reserve slips are notices.


However, those things were easily resolved so I went ahead and did so - I have the commits, do you want them so you can squash them in or... what's your preferred course of action here?
Comment 53 Liz Rea 2012-01-24 15:43:09 UTC
oh and since now it's working - I'll test it! :)
Comment 54 Srdjan Jankovic 2012-01-25 06:35:41 UTC
For the commits, if your testing finds no other problems (wishful thinking) then I'll ask you to submit them as follow ups. Otherwise doesn't matter, I'll redo them myself with the next patch.
Comment 55 Liz Rea 2012-01-25 15:48:10 UTC
It looks like slip printing from moremember.pl throws a template error: 

http://koha.nekls.org/cgi-bin/koha/members/moremember.pl?borrowernumber=16&amp;print=qslip
 
Software error:
 
Template process failed: file error - /home/liz/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember-receipt.tt: not found at /home/liz/kohaclone/C4/Templates.pm line 126.
 
For help, please send mail to the webmaster (webmaster@koha-deb), giving this error message and the time and date of the error.
Comment 56 Srdjan Jankovic 2012-01-27 01:50:01 UTC Comment hidden (obsolete)
Comment 57 Liz Rea 2012-01-27 15:55:20 UTC
I wonder if for usability (please feel free to shoot this down, or save it for later, really) it would be easier to define the custom CSS in a "opacusercss" type way instead of a "custom file on the server" way. For hosted libraries, often they do not have access to the filesystem of their server - this is one of the reasons that opacusercss and intranetusercss exist - to allow users to help themselves through the UI.

Can you think of a reason not to do the notice and slip CSS that way instead of with a file? I can imagine that it might not be as lightning fast, but I can't think of any other reason not to allow that flexibility on the slips.

If that's an excessive scope creep I apologize, it's mostly just a thought experiment.
Comment 58 Liz Rea 2012-01-27 21:59:40 UTC
Also, this new patch looks pretty dang good. Nice work!
Comment 59 Liz Rea 2012-02-01 18:26:24 UTC
I'm going to go ahead and sign off on this - my multiple testers for this latest patch all agree that the functionality is as it should be. It can be refined later if need be. :)

Signoff incoming.
Comment 60 Liz Rea 2012-02-01 19:21:58 UTC Comment hidden (obsolete)
Comment 61 Srdjan Jankovic 2012-02-02 07:03:49 UTC
Re CSS: I saw it being done that way elsewhere (or maybe it was pointed out to me, I cannot remember any more, was some rime ago). I do apologise, I was not aware that it could be done some other way.
Comment 62 Liz Rea 2012-02-03 00:08:03 UTC
srdjan:
no worries. This one caused you no end of grief. It can be revised later. :)
Comment 63 Paul Poulain 2012-02-10 19:04:25 UTC
Created attachment 7572 [details]
no list of branches

I tried this bug, and had some problems to understand how it works.
I've attached a screenshot, because, if I compare with one of the screencast, I see that I don't have the "branch" selection.

As I understand it:
* I can't edit a (all libraries) notice, I can only copy it, and that's not what I wanted to do.
* when I copy a notice, I reach when validating a screen where I see only the "my library" notices, not the (all libraries ones)

Testing with a login "test" that is superlibrarian (but not the mysql user), could it be the reason ?

Marking failed QA until I understand what I made wrong or there's a follow-up to fix my problem if it's a bug
Comment 64 Paul Poulain 2012-02-10 19:09:28 UTC
Other QA comments from my pre-applypatch:

"my" variable @issuedata masks earlier declaration in same scope at members/moremember.pl line 259.

C4/Letters.pm contains warn (+          or warn( "No subscription for '$externalid'" ),)
C4/Letters.pm contains warn (+            warn "Bookseller $dataorders->[0]->{booksellerid} without emails";)
C4/Letters.pm contains warn (+        or warn( "No $module $letter_code letter"),)
C4/Letters.pm contains warn (+                warn "_parseletter_sth('$table') failed to return a valid sth.  No substitution will be done for that table.";)
C4/Letters.pm contains warn (+        warn "ERROR: No _parseletter_sth query for table '$table'";)
C4/Suggestions.pm contains warn (+            }) or warn "can't enqueue letter $letter";)
Comment 65 Liz Rea 2012-02-10 20:35:49 UTC
(In reply to comment #63)
> Created attachment 7572 [details]
> no list of branches
> 
> I tried this bug, and had some problems to understand how it works.
> I've attached a screenshot, because, if I compare with one of the screencast, I
> see that I don't have the "branch" selection.
I did not observe this

> 
> As I understand it:
> * I can't edit a (all libraries) notice, I can only copy it, and that's not
> what I wanted to do.

This isn't right. See comment 52 - the same thing happened to me on one apply, however the next time I applied it (on a clean DB) it worked fine - try again please?

I dumped out my testing DB, and started with a brand new fresh one using the 7001 code, and it worked just fine, this is what my screencap looks like: http://screencast.com/t/jwbDnCUexr

I also did it on the sandbox, the screencap looks the same: http://screencast.com/t/FCQbC1qoEh3c

Moreover, the functionality is still working as it did the last time I tested the patch.
Comment 66 Srdjan Jankovic 2012-02-12 23:04:28 UTC
(In reply to comment #64)
> Other QA comments from my pre-applypatch:
> 
> "my" variable @issuedata masks earlier declaration in same scope at
> members/moremember.pl line 259.

This came with bug 5952. It is easy to fix, line 254 needs removing.

> 
> C4/Letters.pm contains warn (+          or warn( "No subscription for
> '$externalid'" ),)

What is the policy re warns? There are situations when processing is not possible, we don't want to raise an exception but want to leave a trace of what happened. I could not see any logging.
Comment 67 Paul Poulain 2012-02-15 09:12:20 UTC
(In reply to comment #65)
> > As I understand it:
> > * I can't edit a (all libraries) notice, I can only copy it, and that's not
> > what I wanted to do.
> 
> This isn't right. See comment 52 - the same thing happened to me on one apply,
> however the next time I applied it (on a clean DB) it worked fine - try again
> please?

This is right, and I could understand how to reproduce the problem: it's IndependantBranches:
* when it's OFF, the behaviour is the one you have (and the one I was expecting)
* when it's ON, I get the http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=7572 screenshot. And I'm logged-in as superlibrarian, so I should be able to edit the "all libraries" notices, isn't it ?(In reply to comment #66)

> > C4/Letters.pm contains warn (+          or warn( "No subscription for
> > '$externalid'" ),)
> 
> What is the policy re warns? There are situations when processing is not
> possible, we don't want to raise an exception but want to leave a trace of what
> happened. I could not see any logging.
The rule is to have no unconditionnal warns. Your is conditionnal, so it's my test that was wrong, I fixed it.
Comment 68 Liz Rea 2012-02-15 14:43:00 UTC
Good catch on indy branches - srdjan, can you fix the behavior when independantbranches is on?
Comment 69 Srdjan Jankovic 2012-02-16 02:56:39 UTC Comment hidden (obsolete)
Comment 70 Liz Rea 2012-02-16 03:20:15 UTC
That's not a bad question actually.

I don't feel the need to have them restricted - but I can imagine it might come up *someday*.

I'm "ok" with leaving it the way it is now - all being able to edit all notices. In theory, librarians with tools permissions are grown ups and will know not to touch other people's things.
Comment 71 Paul Poulain 2012-02-17 14:39:07 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 72 Liz Rea 2012-02-17 18:01:53 UTC
Additionally, this appears to be *really* broken now, I don't see the edit notice table at all :(
Comment 73 Srdjan Jankovic 2012-02-21 04:56:21 UTC Comment hidden (obsolete)
Comment 74 Liz Rea 2012-02-21 15:23:08 UTC
I had to run the updatedatabase.pl manually, but I finally got it to work. Will test.
Comment 75 Liz Rea 2012-02-21 15:41:06 UTC
with independantbranches on, I get the following when trying to issue an item from another library: 

Software error:

Can't use string ("CPL") as a SCALAR ref while "strict refs" in use at /home/liz/kohaclone/C4/Letters.pm line 124.

It is fine with independantbranches set to "don't prevent."

Otherwise, functionality seems unchanged
Comment 76 Srdjan Jankovic 2012-02-22 05:52:04 UTC Comment hidden (obsolete)
Comment 77 Liz Rea 2012-02-22 15:29:44 UTC Comment hidden (obsolete)
Comment 78 Paul Poulain 2012-02-27 14:10:54 UTC
Note that bug 7467 has introduced a new slip for checked-in items without holds, should be also added in the list of custom notices
Comment 79 Paul Poulain 2012-02-27 14:12:25 UTC
liz, didn't you forget to update the status to "signed-off" ? it seems you've uploaded a signed-of version of this patch
Comment 80 Srdjan Jankovic 2012-02-29 01:09:13 UTC Comment hidden (obsolete)
Comment 81 Srdjan Jankovic 2012-02-29 01:12:06 UTC
I've just realised that I potentially did something silly - did not separate updatedatabase patch. Do you want me to resubmit?
Comment 82 Liz Rea 2012-03-01 17:08:49 UTC
would like to see this get QA before adding in the followup for TRANSFERSLIP, so changing to "Signed Off." Should go back to signoff after it gets through QA for testing of the followup.
Comment 83 Ian Walls 2012-03-01 21:48:08 UTC
Main patch is looking good...I need a little more time with it to do a full analysis for QA, but one thing has come up.

In C4/Letters getletter() subroutine, there is a hash my %letter defined outside the subroutine.  This is later modified in the subroutine.  I'm not exactly sure why, and to the best of my knowledge, this will cause scope problems with mod_perl and other fast CGI alternatives.  I think we'll get a

Variable "%letter" will not stay shared at...

if we try to use mod_perl.

Can you confirm the reasoning behind this coding set up, and whether it would be a lot of work to reimplement not to use this method?  Or perhaps you can confirm that this is not actually a problem in mod_perl?  My understanding may be flawed here.
Comment 84 Srdjan Jankovic 2012-03-01 23:53:00 UTC
(In reply to comment #83)
> Main patch is looking good...I need a little more time with it to do a full
> analysis for QA, but one thing has come up.
> 
> In C4/Letters getletter() subroutine, there is a hash my %letter defined
> outside the subroutine.  This is later modified in the subroutine.  I'm not
> exactly sure why,

That is a workaround for not using objects. It is a cache/stash.

> and to the best of my knowledge, this will cause scope
> problems with mod_perl and other fast CGI alternatives.  I think we'll get a
> 
> Variable "%letter" will not stay shared at...
> 
> if we try to use mod_perl.

That definitely should not happen. "will not stay shared" would happen if using perl threads, but I'm not sure if anyone is using that any more.

> 
> Can you confirm the reasoning behind this coding set up,

Just saves resources when getletter() is called multiple times in one run, like batch notice generation.

> and whether it would
> be a lot of work to reimplement not to use this method?

As it is now, we can only drop it. But a little bird told me that C4 will be replaced with Koha, which will be object based, and the problem will disappear then.

>  Or perhaps you can
> confirm that this is not actually a problem in mod_perl?  My understanding may
> be flawed here.

It has a potential to make problems in mod_perl, if notice records on the database are changed and server process not reloaded.
Comment 85 Ian Walls 2012-03-02 15:50:04 UTC
Srdjan,


Ok, it sounds like this was a performance and future-proofing enhancement along a different track than I was thinking (but still towards the same goal).  I recommend we take this out, though, given that it is not strictly needed, and could as you point out cause problems.

How do others feel about this recommendation?
Comment 86 Ian Walls 2012-03-02 21:39:40 UTC
Another nit-picky detail:

The copyright statement and introductory POD for members/printslip.pl are copied straight out of members/moremember.pl, and should be cleaned up to reflect both the correct copyright, and the actual purpose of this particular page.


And another:

Also, the checkbox for is_html in the letters.tt is not properly attributed.  It's

<input type="checkbox" id="is_html" name="is_html" value="1" checked />

instead of 

<input type="checkbox" id="is_html" name="is_html" value="1" checked="checked" />


And a call for documentation for the <checkedout> and <reserves> tags and how to use them.


Okay, so I think this is pretty solid stuff, the minor issues I've found notwithstanding.  I would highly recommend we get some followup patches to deal with what I've found, but I don't think any of them are significant enough to warrant block QA.  The advantages imparted by the consolidation and cleanup to the Letters code are very, very good, and if we do find bugs down the line (which is always a risk with any code), it'll be easier to trace them and fix them with this set up.

Marking as Passed QA.
Comment 87 Paul Poulain 2012-03-08 15:45:31 UTC
yikes... could you please squash both patches, I can't deal with 2:
* the 1st can't apply because of updatedatabase merge problem (of course). I deal with that, but then,
* the 2nd can't be applied at all anymore:
 Applying: bug_7001: Added TRANSFERSLIP to the letters
fatal: sha1 information is lacking or useless (installer/data/mysql/en/mandatory/sample_notices.sql).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 bug_7001: Added TRANSFERSLIP to the letters

a squashed patch will work like a charm probably !
Comment 88 Srdjan Jankovic 2012-03-09 00:28:28 UTC
Created attachment 8104 [details] [review]
patch

Not sure about that charm thing...
Comment 89 Paul Poulain 2012-03-09 10:31:24 UTC
(In reply to comment #69)
> Created attachment 7673 [details] [review]
> follow-up patch
> 
> I may regret for asking this, so it is ok when we do not have independant
> branches that everyone can edit notices for all branches?

well, usually (at least in France) libraries with independantBranches=OFF are "centralized". There is only one (or a small team) person responsible for managing parameters. So yes, the librarian that is responsible for letters.pl will be responsible for all the libraries.

QA comment:
* epic fail ( ;-) ) on this squashed patch = it removes the eXecutable flag on updatedatabase.pl, so the update is no more working ! (I take care of restoring it)

QUESTIONS:
* why did you not set the "html" flag on the result list ? (I won't reject this patch for this reason, but I think it's *really* usefull)
* when you have a "mandatory" notice defined for a given library, it's not possible to delete it (and switch back to the "default" notification). isn't this a (small) bug ? was it intended ?

HOWEVER: this enhancement deserve a testing period as long as possible as it changes many many files. So i'll push it now, and ask for as many tests as possible on koha-devel
Comment 90 Jonathan Druart 2012-03-09 16:07:08 UTC
This patch is in conflict with Bug 5347, It seems, few changements can fix it:

In C4::Letters::SendAlerts:
Error SQL :
 DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 7 at /.../C4/Letters.pm line 329.

The query if $type eq 'claimacquisition' become: "SELECT [...] WHERE aqorders.ordernumber IN (;"


And I need an error if bookseller have no email (for display):
unless (@email) {
   warn "Bookseller $dataorders->[0]->{booksellerid} without emails";
   return;
}

In my patch, I returned: return {error => "no_email" };

Perharps, it's just simple. I have not retest all cases of my patch.

Sorry I didn't tried earlier
Comment 91 Srdjan Jankovic 2012-03-11 22:46:59 UTC
(In reply to comment #89)

> QA comment:
> * epic fail ( ;-) ) on this squashed patch = it removes the eXecutable flag
> on updatedatabase.pl, so the update is no more working ! (I take care of
> restoring it)

Oops, have no idea how I managed that.

> 
> QUESTIONS:
> * why did you not set the "html" flag on the result list ? (I won't reject
> this patch for this reason, but I think it's *really* usefull)

Paul, I have no idea what you are saying here. If it is useful, I'll try to add it just tell me which result list.

> * when you have a "mandatory" notice defined for a given library, it's not
> possible to delete it (and switch back to the "default" notification). isn't
> this a (small) bug ? was it intended ?

It is a bug, and I'll fix it.
Comment 92 Srdjan Jankovic 2012-03-11 22:49:13 UTC
This has status of pushed to master, which confuses me a bit. Do you want me to provide a replacement patch, or a follow-up?
Comment 93 Srdjan Jankovic 2012-03-11 23:47:47 UTC
(In reply to comment #90)
> This patch is in conflict with Bug 5347, It seems, few changements can fix
> it:
> 
> In C4::Letters::SendAlerts:
> Error SQL :
>  DBD::mysql::st execute failed: You have an error in your SQL syntax; check
> the manual that corresponds to your MySQL server version for the right
> syntax to use near '' at line 7 at /.../C4/Letters.pm line 329.
> 
> The query if $type eq 'claimacquisition' become: "SELECT [...] WHERE
> aqorders.ordernumber IN (;"

I did not get that error. It looks to me as if $externalid array may be corrupt. SendAlerts() does not sanitise input (which is fair enough I suppose). Can you please provide parameters to SendAlerts() that cause the error?

 
> 
> And I need an error if bookseller have no email (for display):
> unless (@email) {
>    warn "Bookseller $dataorders->[0]->{booksellerid} without emails";
>    return;
> }
> 
> In my patch, I returned: return {error => "no_email" };

That is fine however then the whole interface needs to change, not only one return value, so I suggest we leave it like it is for the moment.
Comment 94 Srdjan Jankovic 2012-03-12 00:06:57 UTC
Created attachment 8169 [details] [review]
follow-up patch fix protected letters
Comment 95 Katrin Fischer 2012-03-12 06:38:56 UTC
Hi Srdjan,

I retested late order management in hope to give you some pointers about the problems. I spent quite a lot of time testing this feature before it got in (acquisition > late orders)

In order to test this you will need to create a basket with some orders. Close the basket and set the date filter on late orders to 0.

1) It used to give me an error message about a missing email address in the vendor. Now it always tells me "email has been sent."

2) If I don't select any order to be claimed, I also get "Email has been sent."

3) When claiming the late order I get the MySQL error Jonathan pointed out:
[Mon Mar 12 07:35:23 2012] [error] [client 127.0.0.1] [Mon Mar 12 07:35:23 2012] lateorders.pl: DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 7 at /home/katrin/kohaclone/C4/Letters.pm line 329., referer: http://localhost:8080/cgi-bin/koha/acqui/lateorders.pl

4) I don't get any email... (I suspect 3), but could also be my provider)
Comment 96 Jonathan Druart 2012-03-13 09:04:48 UTC
Created attachment 8196 [details] [review]
Bug 7167 Followup FIX conflicts with Bug 5347

    - Fix sql syntax error
    - Fix Encoding
    - SendAlerts must return an error if no email is defined
    - Get error if no email in memberentry

ps: the 'no email' error for serials/claims.pl is caught in Bug 5342
Comment 97 Paul Poulain 2012-03-13 11:37:22 UTC
I just pushed 
* follow-up patch fix protected letters = thx srdjan, it fixes my comment 89 !
* Bug 7167 Followup FIX conflicts with Bug 5347 = thx jonathan, it fixes the SQL error.

About this last one, jonathan, i've a question:
I can't reproduce the "no_email" display = isn't it supposed to appear when you have mails on new patrons set and don't enter a mail ? In this case, the message is not appearing or i'm doing something wrong.

I've pushed the patch because it fixes the SQL error (& the utf-8 encoding), so it's worth pushing even if the no_mail thing don't work.

If you have an explanation, it will be welcomed though ;-)

Katrin, you're welcomed to test once again if you want !
Comment 98 Paul Poulain 2012-03-13 11:38:05 UTC
oups... hit "save changes" too quickly... I also have pushed a small POD update, to fix a jenkins complain about C4/Print POD invalid !
Comment 99 Katrin Fischer 2012-03-13 11:52:10 UTC
Hi Paul, 

the message is for when you try to send out claims to a vendor with no email address. There is still a small bug though - it will still increase the claim count and set a claim date.
Comment 100 Srdjan Jankovic 2012-03-13 21:58:13 UTC
Comment on attachment 8196 [details] [review]
Bug 7167 Followup FIX conflicts with Bug 5347

I'm not against the patch, but I have to comment:
* Having to use Encode::encode() is wrong - all content comes from the database, therefore should be utf8 already. This looks as curing the symptom, not the cause.
* Even if it needs to be done, I suggest the right place would be where the $letter is generated.
Comment 101 Chris Hall 2012-05-07 20:38:39 UTC
When I have the "CircAutoPrintQuickSlip" set to "Clear the screen" and then I enter a blank barcode in the checkout out box from a borrowers page I get the following error "Undefined subroutine &main::PrintIssueSlip called at /home/chrish/devel/koha/circ/circulation.pl line 175."

Grepping through the codebase the only mention of PrintIssueSlip is in circulation.pl

Tracing it back with git blame I found PrintIssueSlip was added in a9ded4fa008668df8c485fbbc76efa3cc9cc00d7 "bug_7001: Issue and Reserve slips are notices."

If I then go back another commit I find no mentions of PrintIssueSlip.

If "CircAutoPrintQuickSlip" is set to "open a quick print slip window" then the error does not occur.
Comment 102 Srdjan Jankovic 2012-05-09 01:07:59 UTC
New bug created http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8056
Comment 103 Owen Leonard 2012-05-17 20:23:40 UTC
(In reply to comment #96)
>     - Get error if no email in memberentry

Where do these errors appear? I don't see any errors when I add a patron with no email address, and it doesn't look like the errors can ever make it to the template. Doesn't the script proceed with the add and go directly to the redirect?
Comment 104 Jonathan Druart 2012-05-18 10:32:38 UTC
Hi Owen, 
Yes, You are right.
In this case, we can't have an error "no_email" returned by sendAlerts. But others errors are possible (catched with eval into $@). I use the same code as lateorders (for the routine SendAlerts).
Comment 105 Owen Leonard 2012-05-18 12:35:34 UTC
(In reply to comment #104)
> others errors are possible (catched with eval into $@)

Under what circumstances would a patron insert be aborted and one of these errors displayed by the template?
Comment 106 Jonathan Druart 2012-05-18 14:06:02 UTC
(In reply to comment #105)
> Under what circumstances would a patron insert be aborted and one of these
> errors displayed by the template?

We don't abort the patron creation. But the routine (SendAlerts) can raise an error (if email is not sent or GetPreparedLetter die, or ...).
If SendAlerts raises an error, the script continues and doesn't abort. I just want to display a potential error. Else, the template displays "Email has been sent.".
Comment 107 Melia Meggs 2012-05-29 18:36:52 UTC
Hi,

We are finding that as we upgrade libraries to 3.8.1, the quick slip is not printing item info.  So far two libraries are reporting that quick slips only print the patron name, date, and the words "Checked out today" but no items or due dates.  The syntax looks like this after the upgrade - is this correct?

<checkedout>
<p>
<<biblio.title>> <br />
Barcode: <<items.barcode>><br />
Date due: <<issues.date_due>><br />
</p>
</checkedout>

Thanks,

Melia
Comment 108 Katrin Fischer 2012-06-17 16:34:49 UTC
I am not sure why this is in discussion and which patches have been applied already? Can someone explain?
Comment 109 Srdjan Jankovic 2012-06-18 00:30:33 UTC
This one is pushed to release and definitely should not be discussed here any more. All issues should be opening a new bug and refer to this bug.
Comment 110 Melia Meggs 2013-01-04 23:18:31 UTC
*** Bug 3246 has been marked as a duplicate of this bug. ***