Description
Kyle M Hall (khall)
2014-01-28 16:42:23 UTC
Created attachment 24848 [details] [review] Bug 11629 - Add ability to update not for loan status on checkin Some libraries would like to streamline the cataloging process my automatically updating notforloan values on checkin. For example, an item is set to notforloan of -1 ( ordered ). The item, is received, processed, and checked in for the first time before being shelved. The checkin automatically changes the nfl value from -1 to 0. The same workflow could be used for damaged items as well. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Set the new system preference UpdateNotForLoanStatusOnCheckin to the following: -1: 0 0: 1 4) Create an item, set it's notforloan value to -1 5) Check in the item, note its not for loan value is now 0 6) Check in the item again, note its not for loan value is now 1 7) Check in the item again, note its not for loan value reamins 1 Created attachment 24849 [details] [review] Bug 11629 - Add ability to update not for loan status on checkin Some libraries would like to streamline the cataloging process my automatically updating notforloan values on checkin. For example, an item is set to notforloan of -1 ( ordered ). The item, is received, processed, and checked in for the first time before being shelved. The checkin automatically changes the nfl value from -1 to 0. The same workflow could be used for damaged items as well. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Set the new system preference UpdateNotForLoanStatusOnCheckin to the following: -1: 0 0: 1 4) Create an item, set it's notforloan value to -1 5) Check in the item, note its not for loan value is now 0 6) Check in the item again, note its not for loan value is now 1 7) Check in the item again, note its not for loan value reamins 1 I tested this. It works fine for us. Created attachment 24962 [details] [review] Bug 11629 - Add ability to update not for loan status on checkin Created attachment 24963 [details] [review] Bug 11629 [QA Followup] - Add message for librarian that status was updated Created attachment 24964 [details] [review] Bug 11629 [QA Followup] - Add message for librarian that status was updated Hi Kyle, 2 things: - Can you please move the syspref so the alphabetic order is correct? - Are you very attached to the <strong> around the not for loan values in the librarian message? It breaks the string into pieces in translation: msgid "Not for loan status updated from " And somewhere a 'to' I was too lazy to locate. QA comment: 1/ I am not sure the code you added to AddReturn is put at the right place. I think the code should be put into the if ( $doreturn ) statement (if the return is not done, I suppose we don't want to update the not for loan status). Moreover, you will be able to use the existing call to ModItem (and avoid 1 call to it). 2/ Could you add unit tests for this new syspref please? Marked as Failed QA. Created attachment 25229 [details] [review] Bug 11629 [QA Followup 2] - Move code to inside of doreturn if statement Created attachment 25580 [details] [review] Bug 11629 - Add ability to update not for loan status on checkin Some libraries would like to streamline the cataloging process my automatically updating notforloan values on checkin. For example, an item is set to notforloan of -1 ( ordered ). The item, is received, processed, and checked in for the first time before being shelved. The checkin automatically changes the nfl value from -1 to 0. The same workflow could be used for damaged items as well. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Set the new system preference UpdateNotForLoanStatusOnCheckin to the following: -1: 0 0: 1 4) Create an item, set it's notforloan value to -1 5) Check in the item, note its not for loan value is now 0 6) Check in the item again, note its not for loan value is now 1 7) Check in the item again, note its not for loan value reamins 1 Signed-off-by: Havilah Lyon <havilah@aflibrary.org> Created attachment 25581 [details] [review] Bug 11629 [QA Followup] - Add message for librarian that status was updated Created attachment 25582 [details] [review] Bug 11629 [QA Followup] - Unit Tests (In reply to Jonathan Druart from comment #8) > QA comment: > 1/ I am not sure the code you added to AddReturn is put at the right place. > I think the code should be put into the > if ( $doreturn ) > statement (if the return is not done, I suppose we don't want to update the > not for loan status). > Moreover, you will be able to use the existing call to ModItem (and avoid 1 > call to it). > > 2/ Could you add unit tests for this new syspref please? > > Marked as Failed QA. I deprecated the patch for point 1, as we *do* want to update the not for loan status even if the return is not done. In fact, the primary reason for this is to cause a not for loan item to become for loan. I wasn't thinking when I wrote the qa followup! Created attachment 25583 [details] [review] Bug 11629 [QA Followup] - Remove <strong> tags to ease translation Hm, some problems to fix here: FAIL t/db_dependent/Circulation_issue.t OK pod OK forbidden patterns FAIL valid "my" variable $biblionumber masks earlier declaration in same scope "my" variable $biblioitemnumber masks earlier declaration in same scope OK critic FAIL installer/data/mysql/sysprefs.sql FAIL sysprefs_order Not blocker: Sysprefs UNIMARCAuthorityField100 is bad placed (see bug 10610) Created attachment 27233 [details] [review] Bug 11629 [QA Followup] Created attachment 27244 [details] [review] Bug 11629 [QA Followup] - Updating notforloan to 0 gives no "to" status in message Created attachment 27245 [details] [review] Bug 11629 [QA Followup] - Updating notforloan to 0 gives no "to" status in message Created attachment 27246 [details] [review] Bug 11629 [QA Followup] - Updating notforloan to 0 gives no "to" status in message Created attachment 27247 [details] [review] [PASSED QA] Bug 11629 - Add ability to update not for loan status on checkin Some libraries would like to streamline the cataloging process my automatically updating notforloan values on checkin. For example, an item is set to notforloan of -1 ( ordered ). The item, is received, processed, and checked in for the first time before being shelved. The checkin automatically changes the nfl value from -1 to 0. The same workflow could be used for damaged items as well. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Set the new system preference UpdateNotForLoanStatusOnCheckin to the following: -1: 0 0: 1 4) Create an item, set it's notforloan value to -1 5) Check in the item, note its not for loan value is now 0 6) Check in the item again, note its not for loan value is now 1 7) Check in the item again, note its not for loan value reamins 1 Signed-off-by: Havilah Lyon <havilah@aflibrary.org> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Testing notes on last patch. Created attachment 27248 [details] [review] [PASSED QA] Bug 11629 [QA Followup] - Add message for librarian that status was updated Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 27249 [details] [review] [PASSED QA] Bug 11629 [QA Followup] - Unit Tests Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 27250 [details] [review] [PASSED QA] Bug 11629 [QA Followup] - Remove <strong> tags to ease translation Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 27251 [details] [review] [PASSED QA] Bug 11629 [QA Followup] Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Changed the syspref.sql file to remove one of the doubled up UNIMARCAuthrityField100 that slipped in with an earlier patch on this bug. Passes all tests and QA script, also t/db_dependent/Circulation_issue.t Tested with items that had - no notforloan value set - notforloan value not used in updateNotforLoanStatusOnCheckin - the notforloan value defined in UpdateNotForLoanStatusOnCheckin Also verified it works the same, when the item was checked out or not checked out on return. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 27252 [details] [review] [PASSED QA] Bug 11629 [QA Followup] - Updating notforloan to 0 gives no "to" status in message Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Patch does not apply. Created attachment 28147 [details] [review] Bug 11629 - Add ability to update not for loan status on checkin Some libraries would like to streamline the cataloging process my automatically updating notforloan values on checkin. For example, an item is set to notforloan of -1 ( ordered ). The item, is received, processed, and checked in for the first time before being shelved. The checkin automatically changes the nfl value from -1 to 0. The same workflow could be used for damaged items as well. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Set the new system preference UpdateNotForLoanStatusOnCheckin to the following: -1: 0 0: 1 4) Create an item, set it's notforloan value to -1 5) Check in the item, note its not for loan value is now 0 6) Check in the item again, note its not for loan value is now 1 7) Check in the item again, note its not for loan value reamins 1 Signed-off-by: Havilah Lyon <havilah@aflibrary.org> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Testing notes on last patch. Created attachment 28148 [details] [review] Bug 11629 [QA Followup] - Add message for librarian that status was updated Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 28149 [details] [review] Bug 11629 [QA Followup] - Unit Tests Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 28150 [details] [review] Bug 11629 [QA Followup] - Remove <strong> tags to ease translation Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 28151 [details] [review] Bug 11629 [QA Followup] Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Changed the syspref.sql file to remove one of the doubled up UNIMARCAuthrityField100 that slipped in with an earlier patch on this bug. Passes all tests and QA script, also t/db_dependent/Circulation_issue.t Tested with items that had - no notforloan value set - notforloan value not used in updateNotforLoanStatusOnCheckin - the notforloan value defined in UpdateNotForLoanStatusOnCheckin Also verified it works the same, when the item was checked out or not checked out on return. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 28152 [details] [review] Bug 11629 [QA Followup] - Updating notforloan to 0 gives no "to" status in message Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Pushed to master. Thanks, Kyle! *** Bug 6382 has been marked as a duplicate of this bug. *** |