Description
Francois Charbonnier
2014-08-15 13:05:00 UTC
I understand DefaultNoticeFee - but I am not sure about putting a variable for the notices system into a system preference. Wouldn't it be better if the library could just add the note as part of the notice configuration in the text? The goal of the "NoticeFeeNote" preference is to populate the database table "accountlines". That way, you will know, looking at the fine tab for a patron, that the fee comes from the notice fee. Then, there is another development (#12770) that will display this information (actually, all information from the accountlines table) within a reminder letter. Created attachment 34335 [details] [review] Bug 12769 - Notice fee management To Test: Apply patch and update the database. Set up : The "DefaultNoticeFee" system preference allows to set up how much a notice is charged to a patron when sent. The "NoticeFeeNote" systeme preference contains the value which will populate the note in the "accountlines" table when the notice fee is charged. On the "Slips and notices" page (see tools module), a new field allow librarian to set up a specific notice fee for each notice. If nothing is set up, the system preference value will be used. If set up to 0, no fee will be charged to the patron when the the notice is sent. For each test scenario below : 1. Checkout an item with a due date in the past to get the ODUE notice 2. Checkout an item due for today to get the DUE notice 3. Checkout an item due soon to get the PREDUE notice 4. Run overdue_notices.pl 5. Run advance_notices.pl 6. Go to the patron page and open the fine tab. Test scenario 1 : Set up : "Default notice fee" syspref = 0 "NoticeFeeNote" syspref = Notice fee ODUE notice fee = 50$ DUE notice fee = leave empty PREDUE notice fee = leave empty Check that : 7. There is a new 50$ entry (ODUE notice fee) 8. This entry note is "notice fee" Test scenario 2 : Set up : "Default notice fee" syspref = 0 "NoticeFeeNote" syspref = Notice fee ODUE notice fee = 50$ DUE notice fee = 20$ PREDUE notice fee = leave empty Check that : 7. There are two new entries, one is 50$ (ODUE notice fee), the other 20$ (DUE notice fee) 8. Both entries have "Notice fee" in their notes. Test scenario 3 : Set up : "Default notice fee" syspref = 50$ "NoticeFeeNote" syspref = Notice fee ODUE notice fee = leave empty DUE notice fee = leave empty PREDUE notice fee = leave empty Check that : 7. There are three 50$ entries (ODUE, DUE and PREDUE notice fees) 8. Each has a "Notice fee" note. Test scenario 4 : Set up : "Default notice fee" syspref = 50$ "NoticeFeeNote" syspref = Notice fee ODUE notice fee = 55$ DUE notice fee = 20$ PREDUE notice fee = leave empty Check that : 7. There are three entries : 55$ for the ODUE notice, 20$ for the DUE notice, 50$ for the PREDUE notice 8. Each has a "Notice fee" note. Test scenario 5 : Set up : "Default notice fee" syspref = 50$ "NoticeFeeNote" syspref = Notice fee ODUE notice fee = 55$ DUE notice fee = 20$ PREDUE notice fee = 0 Check that : 7. There are two entries : 55$ for the ODUE notice, 20$ for the DUE notice 8. Each has a "Notice fee" note. Created attachment 36318 [details] [review] Bug 12769 - Notice fee management (In reply to Charles Farmer from comment #5) > Created attachment 36318 [details] [review] [review] > Bug 12769 - Notice fee management This last patch was solely made to solve merge-conflicts in updatedatabase.pl Created attachment 36351 [details] [review] Bug 12769 - Notice fee management Created attachment 36504 [details] [review] Bug 12769 - Display fee in notice fee This attachment correct the problem with the last attachment: the fee was not displayed in the notice fee input I get this when trying to apply the patch to an up to date branch: pplying: Bug 12769 - Notice fee management fatal: sha1 information is lacking or useless (installer/data/mysql/kohastructure.sql). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 12769 - Notice fee management Is there any dependency not listed or perhaps the patch must be rebased? Comment on attachment 36351 [details] [review] Bug 12769 - Notice fee management Review of attachment 36351 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/updatedatabase.pl @@ +9793,4 @@ > SetVersion($DBversion); > } > > +$DBversion = "XXX"; Please convert this to an atomic update. See http://wiki.koha-community.org/wiki/Database_updates#updatedatabase.pl @@ +9794,5 @@ > } > > +$DBversion = "XXX"; > +if ( CheckVersion($DBversion) ) { > + $dbh->do("ALTER TABLE `letter` add `fee` decimal(10,2) DEFAULT NULL"); https://dev.mysql.com/doc/refman/5.1/en/alter-table.html Please include AFTER so that this alter will generate same column order as kohastructure.sql See reasons in comment #11 Created attachment 39900 [details] [review] Bug 12769 - Notice fee management Created an atomic update .sql file. Comment on attachment 39900 [details] [review] Bug 12769 - Notice fee management Review of attachment 39900 [details] [review]: ----------------------------------------------------------------- ::: misc/cronjobs/advance_notices.pl @@ +525,5 @@ > + } else { > + $sendfee = C4::Context->preference('DefaultNoticeFee'); > + } > + > + if ($sendfee > 0){ $sendfee could be undefined. This would generate noise. ::: misc/cronjobs/overdue_notices.pl @@ +599,5 @@ > + } else { > + $sendfee = C4::Context->preference('DefaultNoticeFee'); > + } > + > + if ($sendfee > 0){ Check if defined. :) Comment on attachment 39900 [details] [review] Bug 12769 - Notice fee management Review of attachment 39900 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/kohastructure.sql @@ +1393,4 @@ > `title` varchar(200) NOT NULL default '', -- subject line of the notice > `content` text, -- body text for the notice or slip > `message_transport_type` varchar(20) NOT NULL DEFAULT 'email', -- transport type for this notice > + `fee` decimal(10,2) DEFAULT NULL, -- set up the cost of sending a notice Null = Don't send fee notice? How does 0 and NULL differ in intended behaviour? Created attachment 39918 [details] [review] Bug 12769 - Follow up to comments 14 & 15 The scripts now check if the variable is defined. As for 0 and NULL in the letter's fee column: If the fee is NULL, we use the default value stored in the system preference. If the syspref is 0 or NULL the patron is not charged. If the fee is 0, we use that value and the fine is not created. Created attachment 39919 [details] [review] Bug 12769 - Follow up to comments 14 & 15 Removed useless change in previous patch Hi Maxime, I am trying to test this to sign off and the patch is applied and database updated. However when I follow the test plan there is never set any fines. The accountlines table is not populated and there are not any entries there from before. Do you have any advice for troubleshooting this/testing further? (In reply to Eivin Giske Skaaren from comment #18) > Do you have any advice for troubleshooting this/testing further? Which test scenario is failing? Which script is failing (overdue or advance) ? What options are given to overdue_notices.pl and advance_notices.pl? Use the --help option to see what's available. For advance, you should be using the confirm option (-c) and verbose (-v). Check that the EnhancedMessaging syspref On. Check that the patron has these notices activated : Advance notice, Item Due Make sure the items' due dates (For due and overdue), the current date and the 'Days in advance' setting for the borrower match together to trigger the notice. For overdue, the notice should be triggering if the patron has an item overdue. The scripts are not failing but they don´t actually do anything. So all scenarios are failing for me at this point.. > Check that the EnhancedMessaging syspref On. Do you mean the EnhancedMessagingPreferences? It can be set to Allow or Don´t allow. I have tested with both. > Check that the patron has these notices activated : Advance notice, Item Due There are mostly dashes shown in the table in Patron messaging preferences. The only thing that can be selected is days in advance for Advance notice. (In reply to Eivin Giske Skaaren from comment #20) > There are mostly dashes shown in the table in Patron messaging preferences. > The only thing that can be selected is days in advance for Advance notice. Check the message_attributes table. If it's empty, try this: insert into `message_attributes` (`message_attribute_id`, message_name, `takes_days`) values (1,'Item Due', 0), (2,'Advance Notice', 1), (4,'Hold Filled', 0), (5,'Item Check-in', 0), (6,'Item Checkout', 0); (In reply to Maxime Beaulieu from comment #21) Thanks for the suggestion. The values are there in the message_attributes table. The problem is that the checkboxes are missing. I have now confirmed that it must be some problem with the vagrant devbox. I set up a completely new installation and got the same problem. I checked on another installation that I had done manually on a server and there the checkboxes exist. (In reply to Eivin Giske Skaaren from comment #22) Check the message_transports table. insert into `message_transports` (`message_attribute_id`, `message_transport_type`, `is_digest`, `letter_module`, `letter_code`) values (1, 'email', 0, 'circulation', 'DUE'), (1, 'email', 1, 'circulation', 'DUEDGST'), (2, 'email', 0, 'circulation', 'PREDUE'), (2, 'email', 1, 'circulation', 'PREDUEDGST'), (In reply to Maxime Beaulieu from comment #23) > (In reply to Eivin Giske Skaaren from comment #22) > > Check the message_transports table. > > insert into `message_transports` > (`message_attribute_id`, `message_transport_type`, `is_digest`, > `letter_module`, `letter_code`) > values > (1, 'email', 0, 'circulation', 'DUE'), > (1, 'email', 1, 'circulation', 'DUEDGST'), > (2, 'email', 0, 'circulation', 'PREDUE'), > (2, 'email', 1, 'circulation', 'PREDUEDGST'), Yes that made some checkboxes appear. Created attachment 40081 [details] [review] Bug 12769 - Notice fee management Created an atomic update .sql file. Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com> Created attachment 40082 [details] [review] Bug 12769 - Follow up to comments 14 & 15 Removed useless change in previous patch Test scenario 1: OK Test scenario 2: OK Test scenario 3: OK Test scenario 4: OK Test scenario 5: OK Some notes: In test scenario 3 I noticed that running the cron scripts several times would add several fines for the same items due/overdue. I did not test for this during the other scenarios. If an item is checked out without having a branch selected in the staff client the branchcode will be set to NO_LIBRARY and then no fines will be triggered when running the scripts. Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com> I developed something similar for a BibLibre's customer some years ago. I added the "fee" value to the overduerules table, which, I think, is much more flexible. For instance, you could specify different fee for a given notice. Your way could add flexibility, it will be possible to add fees for something else than overdue notices. But I am not sure where it could apply. My concern is that the field will be display for all notices but will only be take into account in few cases. Putting in discussion and ccing some BibLibre guys. Here is the commit: https://git.biblibre.com/biblibre/kohac/commit/80193b171eddb93fdbba5bea0b83ea97ca590c1a Since June, did the BibLibre team had any chance to test/review/give their opinion on the matter? (In reply to Charles Farmer from comment #29) > Since June, did the BibLibre team had any chance to test/review/give their > opinion on the matter? +1 Since this is a sponsored patch the need is real. What can be done to make progress here? There are 2 other vug reports (see also field) which propose the same enhancement. You can try and contact the authors of these patches and work all together to find a solution that please everybody. (In reply to Jonathan Druart from comment #31) > You can try and contact the authors of these patches and work all together > to find a solution that please everybody. The collaborative approach is not bearing any fruits, I'm afraid. Bug 14666's assignee is uninterested in looking elsewhere for his needs. Compared to the other patches I've seen for this feature, this one allows a fine to be added to any kind of notice, including overdue notices. Adding a fee solely to overdue notices feels rather incomplete in comparison. (In reply to Jonathan Druart from comment #27) > My concern is that the field will be display for all notices but will only > be take into account in few cases. Yes, the added flexibility means we would see the fee box appear every time. I don't know enough about Koha's UX to really give you an alternative, but perhaps you have an idea on how we could answer your concern? (In reply to Charles Farmer from comment #32) > Bug 14666's assignee is uninterested in looking elsewhere for his needs. Scratch that, there was a misunderstanding in the other thread :) (In reply to Charles Farmer from comment #32) > (In reply to Jonathan Druart from comment #27) > > My concern is that the field will be display for all notices but will only > > be take into account in few cases. > Yes, the added flexibility means we would see the fee box appear every time. > I don't know enough about Koha's UX to really give you an alternative, but > perhaps you have an idea on how we could answer your concern? Heh, on one hand you add flexibility (ability to specify fee for any types of notices) but on the other hand you don't: the need on bug 13816 and bug 14666 is to define different fees for the same letter. You could want to charge a student for an overdue, but not a staff member, using the same notice template. I don't have a good idea to give you right now, but maybe an email on koha-devel to explain the problem could bring us feedback/help/ideas? There is something else wrong with your patch: You can only define a fee on editing a circulation notice (not on creating), minor. ---Short Version--- Do we want to be able to charge users on arbitrary notifications, and if we do, are we willing to provide users with a design that will require maintaining all the different variants of the notifications? ---Long Prose--- I went and read the RFC mentioned at the top of this thread first, which seems to be where the initial requirement was expressed. Bug 14666 is obviously trying to achieve something quite similar. I see two possibilities: - The requirement is to charge on notifications for late documents, something that indeed cannot be done in Koha right now. That is my understanding of the RFC actually, but I didn't write it, and I don't know the sponsor;). I have had libraries asking me how to have a one time charge when patrons have late documents. Koha makes some complicated calculations regarding each document and the number of days late, according to the circulation matrix, which definitely doesn't fit everyone's needs, and which will generate odd amounts for fines. My point is that there clearly is a need to be able to charge a simple one time amount to patrons with late documents. If this is the only scenario for the requirement, then I think adding this possibility at the notification level is a bad idea. It makes it difficult to charge different amounts for different patron categories or different libraries since you will have to edit different notifications for these, I find the "notices and slips" tool quite complicated already, while the "overdue status trigger" tool is very clear. It also makes it possible to charge for notifications for which I cannot see the need, like PREDUE. There are other notifications for which I think "fair enough, why not", like "hold available for pick up", but frankly none of them make as much sense to me as the overdue notices (and I don't see myself needing this anytime soon). - But maybe that is precisely the requirement, to be able to charge users on arbitrary notifications and not just for late items. If this is the case then in the current design libraries who want to manage different fine amounts for different patron categories / libraries combinations for overdue notices will have to deal with having a really long list of notifications to maintain, since they will have one for each combination of patron category / library for which the amount is different. If the wording has to change in the notification, all of them will need to be edited. This adds up with the different notifications for each stage (1st reminder, 2nd reminder, etc.), so it will be a lot of lines in the tool. So I would say the question is: do we want to be able to charge users on arbitrary notifications, and if we do, are we willing to provide users with a design that will require maintaining all the different variants of the notifications? The BibLibre development (bug 14666) is not about charging for arbitrary notifications. You have my opinion on the above question ;) InLibro and the Halland County Library should give their input and we'll see how to move forward! (Although I work for BibLibre, I am not in charge of the development on bug 14666. So if the answer is that the need is to send notifications for arbitrary notifications, I will not block the proposed functionality: it is possible to achieve our needs with it. I just think the interface is going to make it painful. I will ask Stéphane Delaye to give his opinion here though.) Hi Gaetan, I just read your last comment and I can think of different use cases for both developments: - Charge a postage fee for notices you sent out as print or a fee for a text message, as the library has to pay for those. In this case it could make sense to set the fee on notice level, but also on message transport level. - A overdue fee independent on the number of items overdue - a static overdue fee. I think this could work well from the overdue notice triggers page. So maybe both options have merit. Hi all, I wonder why this development ended a year ago without an agreed change. There is a need for notice fees in many libraries. Summarizing the discussion there seem to be the following requirements: - charge notice fees depending on the delivery channel (mail, email, sms) - charge notice fees depending on the message type or letter - charge notice fees depending on the branch and user group The original RFC requires for each different combination of a above parameters to create a different letter. As it can be a complex task to manage all letter configurations, it also does not feel logical to assign a notice fee to a letter independently to whom and when I send it. On the other hand, assigning one and the same price for mail, email and sms (as Jonathans solution suggests) will also not meet requirements of all users if since costs for a mail post, email or sms are typically different. And it would only cover notice fees for overdues. A possible solution for a notice fee configuration that could meet all above requirements would be a separate notice fee configuration that provides the following parameters: - branch library ('' for all libraries) - channel ('' for all channnels) - letter ('' for all letters) - user group ('' for all user groups) - notice fee As with circulation and fine rules, there could be general and specific settings. A general rule could be for example to charge 0.70 for each mail letter and 0.5 for sms independent of the branch or user group. A more specific rule could be to charge nothing for staff members for both delivery types. I think with the ability to send notices by SMS, phone and print even simultanously a general notice fee won't work so well. I think what we'd need here is the option to define a fee per transport type at least. Ideally: branch - patron category - letter - transport type patron category to be able to distinguish between print sent by mail and delivered 'in-house'. I believe with all the comments and more so changes to the notices (think TT syntax) and fee management (configurable debit types) these patches will need rethinking and changes. I am marking Failed QA, but they will also no longer apply. I personally think it'd be great to have a separate configuration page and table for this that ties in with the configurable debit types. That would take care of descriptions and entries to the accountlines tables quite nicely. |