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
Created attachment 6028 [details] [review] patch This proved to be a bit of an ordeal. Letter processing was all over the place. In order to implement html support and branch selection I just had to make a single entry point. Some notes: * Ossue Slip and Quick slip are different - I solved that by having two notes, ISSUESLIP and ISSUEQSLIP * Notices are now either plain or html. I can see some problems with that. What we should really have is a plain text and html version, and pick the right one as required * I've made letter replacement routine that supports all possible usages at the time of making this patch. I have improved it a bit, but it still talks far too much to the database. Is that perceived as a problem?
Updating Version : This ENH will be for Koha 3.8
Created attachment 6047 [details] [review] patch Accounted for independant branches in notices maint
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)
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
I have it on the latest master and it works fine. Can you please try it there.
Sorry Srdjan - It does not work on Master. Please see all of my comments. Did I miss a updatedatabase call? -Brendan
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.
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.
Created attachment 6357 [details] [review] patch That's strange. I had no problems with rebasing. I'm sending another patch that is marginally different, just in case, and if it does not apply please send me the commands you are executing.
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
Created attachment 6854 [details] [review] patch
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.
I confirm, this doesn't apply
Created attachment 6942 [details] [review] patch
New patch applies fine, should be ok to test now
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?
Created attachment 7042 [details] [review] patch This was because of the message_transports table. It seems that I have a database that is not identical with a newly created one, and I was missing a foreign key from message_transports to letter. Thus I did not get the error message. Furthermore, my message_transports and message_attributes tables were empty. I've loaded those with installation records, and that seemed to make no difference. I am not worried to much about all that, it was a production database, however: Foreign key from message_transports to letter seems to be pointless. It is not used anywhere for joining and I'm not sure how is that supposed to work. I've corrected the patch, so it will be accurate in terms of the database changes, but can anyone please comment/explain to me connection between message_transports and letter tables? Maybe that is not structured as it should be?
Created attachment 7045 [details] [review] patch
*** Bug 2349 has been marked as a duplicate of this bug. ***
Created attachment 7096 [details] [review] bug 7001 - fix errant ) that broke updatedatabase
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.)
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.
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.
apologies - I found the NoticeCSS - it was under my nose and I didn't see it. Sorry!
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!
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
>* 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).
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?
Created attachment 7111 [details] [review] patch * Delete fixed * Keeping selected library is a reasonable expectation, so it's there now * Cancel added, was not a big deal * It is "Notices and Slips", sounds nicer when spoken * System does not know of slips, those are all "letters"; if you can come up with some reliable way of knowing which are slips (oncluding checking that code matches "SLIP") we can add that later * "Print Page" is just an ordinary dosplay template, nothing to do with notices
Awesome - checking these things now and going on with more testing!
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:
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?
Created attachment 7119 [details] [review] patch * Menu fixed * I apologise, attributes were not listed, I've added them now (bottom of the borrowers section) * Re branch for the notices: that depends on the notice, each particular notice handler passes it to the letter composer. A couple of handlers that I looked at are passing the home library of the logged in user. If that is wrong it should be dealt with in separate bug reports on per letter (notice) bases.
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.
Created attachment 7129 [details] [review] patch I have changed slip printing so branch is taken from the session.
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. :)
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.
Created attachment 7169 [details] [review] patch Apologies, missed that one. Should be fine now.
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 :)
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)?
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; }
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).
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). :)
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...
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
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".
Created attachment 7283 [details] [review] patch * 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? I cannot confirm that it is the only notice that has a -print counterpart. Maybe if you come up with a suspects list. * Is this how the HTML notices are supposed to come out? It is possible. I need to have the template as entered in the notices tool. Other issues fixed. And yes, patience is an indispensable tool if one wants to do this job :) But so far I can say the largest number of difficulties comes from the way community projects operate, and there's nothing we can do about it really.
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.
That does not sound right, as if the patch was not applied. Can you please double check that.
Hmm it said it applied ok. And the display is different before and after the patch. I will check it again.
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?
oh and since now it's working - I'll test it! :)
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.
It looks like slip printing from moremember.pl throws a template error: http://koha.nekls.org/cgi-bin/koha/members/moremember.pl?borrowernumber=16&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.
Created attachment 7340 [details] [review] patch Ooops, I removed that template in error. Restored. That print page does not use ISSUESLIP notice template. And it cannot, it is different, just need to mention it
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.
Also, this new patch looks pretty dang good. Nice work!
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.
Created attachment 7416 [details] [review] [Signed Off] bug_7001: Issue and Reserve slips are notices. Branches can have their own version of notices - added branchcode to letter table. Support html notices - added is_html to letter table. Support for borrower attributes in templates. GetPreparedletter() is the interface for compiling letters (notices). Sysprefs for notice and slips stylesheets. Signed-off-by: Liz Rea <wizzyrea@gmail.com> Test plan stepped through - all tests confirmed working as far as we can tell. Passes prove t t/db_dependent xt tests. (noted that there are other test - unrelated - that do fail, but nothing this patch concerns itself with) Additional notes: * special character handling seems to be working very nicely (may have always been this way) * further refinement of this feature may be desired - but this is an excellent start, and I believe it should be allowed as-is. * Will file enhancement bugs for the following refinements when this is approved into Koha: * *UserCSS type customization handling for custom CSS (in addition to or in place of SlipCSS and NoticeCSS preferences) * per branch CSS using per-branch CSS classes on the <body> of slips (Thanks to MJ Ray for the rather brilliant idea) Special thanks to: Sean McGarvey and the circulation staff at the Hernando Public Library System for extra testing Cathi Wiggins from Arcadia Public Library for additional testing
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.
srdjan: no worries. This one caused you no end of grief. It can be revised later. :)
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
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";)
(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.
(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.
(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.
Good catch on indy branches - srdjan, can you fix the behavior when independantbranches is on?
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?
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.
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
Additionally, this appears to be *really* broken now, I don't see the edit notice table at all :(
Created attachment 7783 [details] [review] patch It was only an updatedatabase.pl conflict, but rebasing anyway because of the failure report. I tried it and could not see that it misbehaves - all links (edit, delete) are there, as well as the copy selection. Can we please retest it now.
I had to run the updatedatabase.pl manually, but I finally got it to work. Will test.
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
Created attachment 7796 [details] [review] patch Sorry, a typo
Created attachment 7804 [details] [review] bug_7001: Issue and Reserve slips are notices. Branches can have their own version of notices - added branchcode to letter table. Support html notices - added is_html to letter table. Support for borrower attributes in templates. GetPreparedletter() is the interface for compiling letters (notices). Sysprefs for notice and slips stylesheets. Signed-off-by: Liz Rea <wizzyrea@gmail.com> Additional notes: * now handles independantbranches properly * special character handling seems to be working very nicely (may have always been this way) * further refinement of this feature may be desired - but this is an excellent start, and I believe it should be allowed as-is. * Will file enhancement bugs for the following refinements when this is approved into Koha: * *UserCSS type customization handling for custom CSS (in addition to or in place of SlipCSS and NoticeCSS preferences) * per branch CSS using per-branch CSS classes on the <body> of slips (Thanks to MJ Ray for the rather brilliant idea) Special thanks to: Sean McGarvey and the circulation staff at the Hernando Public Library System for extra testing Cathi Wiggins from Arcadia Public Library for additional testing
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
liz, didn't you forget to update the status to "signed-off" ? it seems you've uploaded a signed-of version of this patch
Created attachment 7936 [details] [review] patch Follow-up patch for TRANSFERSLIP
I've just realised that I potentially did something silly - did not separate updatedatabase patch. Do you want me to resubmit?
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.
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.
(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.
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?
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.
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 !
Created attachment 8104 [details] [review] patch Not sure about that charm thing...
(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
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
(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.
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?
(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.
Created attachment 8169 [details] [review] follow-up patch fix protected letters
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)
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
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 !
oups... hit "save changes" too quickly... I also have pushed a small POD update, to fix a jenkins complain about C4/Print POD invalid !
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 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.
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.
New bug created http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8056
(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?
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).
(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?
(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.".
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
I am not sure why this is in discussion and which patches have been applied already? Can someone explain?
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.
*** Bug 3246 has been marked as a duplicate of this bug. ***