Description
Kyle M Hall (khall)
2012-07-20 15:33:53 UTC
Created attachment 11053 [details] [review] Bug 8484 - Add ability to choose of lost items are removed from a borrowers record. Adds the system preference ReturnLostItems to control whether setting an item as lost will remove the item from the borrower's record. Note, this system preference does not affect misc/cronjobs/longoverdue.pl which has its own command line switch to choose this behavior. Wanted to sign-off, patch does not apply, sorry. The conflict is a simple one in updatedatabase.pl, and should (IMO) be resolved by the QA-er rather than requiring a follow-up patch. i test the patch in a sandbox To test the patch I made the following steps I record a check out I edit the item and I put in 995$2 (UNIMARC) value 1 (items.lost) I return to the account of the borrower -> The items has not been checked in But regardless the value (check in, don('t check in) under the preference system ReturnLostItems the preference systeme work only if the item is update by the script misc/cronjobs/longoverdue.pl ?... This patch once again makes the behavior of marking an item lost different depending whether you use moredetail or additem to make the change. The new syspref should affect every workflow that sets an item's lost status. longoverdue.pl has its own command line switch to decide if a lost item should be returned or not. This patch only affects items marked as lost via moredetail.pl. (In reply to comment #4) > i test the patch in a sandbox > > To test the patch I made the following steps > > I record a check out > I edit the item and I put in 995$2 (UNIMARC) value 1 (items.lost) > I return to the account of the borrower > -> The items has not been checked in > > But regardless the value (check in, don('t check in) under the preference > system ReturnLostItems > > the preference systeme work only if the item is update by the script > misc/cronjobs/longoverdue.pl ?... (In reply to comment #5) > This patch once again makes the behavior of marking an item lost different > depending whether you use moredetail or additem to make the change. The new > syspref should affect every workflow that sets an item's lost status. That is a non-issue. Currently, that behavior is already different. Marking an item lost via moredetail.pl applies the LostItem subroutine to the given item, while using additem.pl does not. It never has, and that is why the itemlost field is hidden by default on additem.pl Created attachment 15147 [details] [review] [SIGNED-OFF] Bug 8484 - Add ability to choose of lost items are removed from a borrowers record. Adds the system preference ReturnLostItems to control whether setting an item as lost will remove the item from the borrower's record. Note, this system preference does not affect misc/cronjobs/longoverdue.pl which has its own command line switch to choose this behavior. Test plan: Apply the patch and let the database update run. Test the process of marking something lost with the ReturnLostItems preference in the default "on" state: - Find an item which is checked out and mark it lost from moredetail.pl - Find an item which is checked out and mark it lost using the batch item modification tool In both cases the item should be removed from the patron's account (checked in) and a fine applied. Next test with the ReturnLostItems preference turned off. The same procedure should result in the item being checked in but the fine applied. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Passes the test plan which I made up. (In reply to comment #6) > longoverdue.pl has its own command line switch to decide if a lost item > should be returned or not. This patch only affects items marked as lost via > moredetail.pl. I think it is easy to set the default value for the mark-returned parameter to the syspref's value instead of 0. Is there a good reason not to use it in this cronjob script? Created attachment 15169 [details] [review] Bug 8484 - Add ability to choose if lost items are removed from a borrowers record. - Followup Created attachment 15170 [details] [review] Bug 8484 - Add ability to choose if lost items are removed from a borrowers record. - Followup I agree it would be better to control this behavior from one place in Koha. This patch removes the --mark-returned from longoverdue.pl and replaces it with the system preference. I think we should just note this in the upgrade instructions, as trying to support them both will end up with confusing behavior if the flag is not enabled, but the system pref is. Plus, it just adds more cruft to the code base. I've also renamed the system preference to be more in lie with Bug 7639, which ass the system preferences WhenLostForgiveFine and WhenLostChargeReplacementFee Something went wrong ! Applying: Bug 8484 - Add ability to choose of lost items are removed from a borrowers record. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... 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 Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref Failed to merge in the changes. Patch failed at 0001 Bug 8484 - Add ability to choose of lost items are removed from a borrowers record. When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". Bug 8484 - Add ability to choose if lost items are removed from a borrowers record. [SIGNED-OFF] Bug 8484 - Add ability to choose of lost items are removed from a borrowers record. Apply? [yn] Patch left in /tmp/SIGNED-OFF-Bug-8484---Add-ability-to-choose-of-los-Xm4uAj.patch Created attachment 17619 [details] [review] Bug 8484 - Add ability to choose of lost items are removed from a borrowers record. Adds the system preference ReturnLostItems to control whether setting an item as lost will remove the item from the borrower's record. Note, this system preference does not affect misc/cronjobs/longoverdue.pl which has its own command line switch to choose this behavior. Test plan: Apply the patch and let the database update run. Test the process of marking something lost with the ReturnLostItems preference in the default "on" state: - Find an item which is checked out and mark it lost from moredetail.pl - Find an item which is checked out and mark it lost using the batch item modification tool In both cases the item should be removed from the patron's account (checked in) and a fine applied. Next test with the ReturnLostItems preference turned off. The same procedure should result in the item being checked in but the fine applied. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Passes the test plan which I made up. Created attachment 17620 [details] [review] Bug 8484 - Add ability to choose if lost items are removed from a borrowers record. - Followup Kyle if you could please squash these two patches I will test again. Created attachment 20702 [details] [review] Bug 8484 - Add ability to choose of lost items are removed from a borrowers record. Adds the system preference ReturnLostItems to control whether setting an item as lost will remove the item from the borrower's record. Note, this system preference does not affect misc/cronjobs/longoverdue.pl which has its own command line switch to choose this behavior. Test plan: Apply the patch and let the database update run. Test the process of marking something lost with the ReturnLostItems preference in the default "on" state: - Find an item which is checked out and mark it lost from moredetail.pl - Find an item which is checked out and mark it lost using the batch item modification tool In both cases the item should be removed from the patron's account (checked in) and a fine applied. Next test with the ReturnLostItems preference turned off. The same procedure should result in the item being checked in but the fine applied. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Passes the test plan which I made up. Created attachment 20703 [details] [review] Bug 8484 - Add ability to choose if lost items are removed from a borrowers record. - Followup QA comment: I am still not sure it is a good idea to remove the parameter for the longoverdue cronjob. As Kyle suggests it could be a note in the upgrade instruction (I let the RM takes the decision). Note: I will squash these two patches. Marked as Passed QA. Created attachment 21083 [details] [review] Bug 8484 - Add ability to choose of lost items are removed from a borrowers record. Adds the system preference ReturnLostItems to control whether setting an item as lost will remove the item from the borrower's record. Important note: this system preference affects misc/cronjobs/longoverdue.pl! Test plan: Apply the patch and let the database update run. Test the process of marking something lost with the ReturnLostItems preference in the default "on" state: - Find an item which is checked out and mark it lost from moredetail.pl - Find an item which is checked out and mark it lost using the batch item modification tool In both cases the item should be removed from the patron's account (checked in) and a fine applied. Next test with the ReturnLostItems preference turned off. The same procedure should result in the item being checked in but the fine applied. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Passes the test plan which I made up. Bug 8484 - Add ability to choose if lost items are removed from a borrowers record. - Followup I agree that removing the option could cause problems. Why not change the behaviour so the switch can overwrite the system preference? Or would that cause problems? (In reply to Katrin Fischer from comment #21) > I agree that removing the option could cause problems. Why not change the > behaviour so the switch can overwrite the system preference? Or would that > cause problems? I'm pondering this. Since the new system preference defaults to ON, folks used to running longoverdue.pl without the --mark-returned switch (and not just used to it, but who explicitly do *not* want loans to be checked in when the item is marked lost) would be in for a surprise. And since this entails circulation and money, it would be the sort of surprise where a positive response to "you should have read the release notes first" is ... unlikely. To toss out an idea, maybe make the default value (on upgrade) of WhenLostReturnItems be NULL, then add some code to make the staff client header area display a complaint until the system preferences is set to a specific value? And maybe make longoverdue.pl continue to keep its old behavior if the system preference is NULL? This is would all be rather crufty, I know, but overdue and lost item processing is definitely an area where Read the Fine Release Notes doesn't fly. Also, it's not clear to me whether ON or OFF is the better default value for WhenLostReturnItems. Is this one dead in the water... wondering if it's going to move anywhere? This is something that came up at the hackfest - it's an option that a few people here would still like to see make it in to Koha. Trying to get this moving again. I think we have 3 places to consider: - moredetail.pl - checks item in - additem.pl - doesn't check item in - longoverdue.pl - has a switch I agree, what we need here is a way to leave the behaviour unchanged after update, while putting a note somewhere, that encourages people to set it to a value. Galen made a suggestion about this on comment#22. The about.pl system information tab could be a good place for a warning. What do people think? Still an issue? |