Bug 11629 - Add ability to update not for loan status on checkin
Summary: Add ability to update not for loan status on checkin
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
: 6382 (view as bug list)
Depends on:
Blocks: 17480 17104
  Show dependency treegraph
 
Reported: 2014-01-28 16:42 UTC by Kyle M Hall
Modified: 2016-10-21 14:51 UTC (History)
7 users (show)

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


Attachments
Bug 11629 - Add ability to update not for loan status on checkin (5.69 KB, patch)
2014-01-28 16:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11629 - Add ability to update not for loan status on checkin (5.75 KB, patch)
2014-01-28 16:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11629 - Add ability to update not for loan status on checkin (5.80 KB, patch)
2014-01-31 18:37 UTC, Melissa Lefebvre
Details | Diff | Splinter Review
Bug 11629 [QA Followup] - Add message for librarian that status was updated (2.96 KB, patch)
2014-01-31 18:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11629 [QA Followup] - Add message for librarian that status was updated (2.96 KB, patch)
2014-01-31 19:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11629 [QA Followup 2] - Move code to inside of doreturn if statement (2.73 KB, patch)
2014-02-13 18:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11629 - Add ability to update not for loan status on checkin (6.18 KB, patch)
2014-02-24 13:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11629 [QA Followup] - Add message for librarian that status was updated (2.96 KB, patch)
2014-02-24 13:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11629 [QA Followup] - Unit Tests (1.88 KB, patch)
2014-02-24 13:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11629 [QA Followup] - Remove <strong> tags to ease translation (1.46 KB, patch)
2014-02-24 13:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11629 [QA Followup] (3.14 KB, patch)
2014-04-17 11:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11629 [QA Followup] - Updating notforloan to 0 gives no "to" status in message (1.51 KB, patch)
2014-04-17 13:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11629 [QA Followup] - Updating notforloan to 0 gives no "to" status in message (1.94 KB, patch)
2014-04-17 13:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11629 [QA Followup] - Updating notforloan to 0 gives no "to" status in message (1.94 KB, patch)
2014-04-17 13:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 11629 - Add ability to update not for loan status on checkin (6.22 KB, patch)
2014-04-17 14:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11629 [QA Followup] - Add message for librarian that status was updated (3.01 KB, patch)
2014-04-17 14:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11629 [QA Followup] - Unit Tests (1.94 KB, patch)
2014-04-17 14:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11629 [QA Followup] - Remove <strong> tags to ease translation (1.53 KB, patch)
2014-04-17 14:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11629 [QA Followup] (3.73 KB, patch)
2014-04-17 14:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11629 [QA Followup] - Updating notforloan to 0 gives no "to" status in message (2.01 KB, patch)
2014-04-17 14:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11629 - Add ability to update not for loan status on checkin (6.22 KB, patch)
2014-05-09 16:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11629 [QA Followup] - Add message for librarian that status was updated (2.99 KB, patch)
2014-05-09 16:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11629 [QA Followup] - Unit Tests (2.18 KB, patch)
2014-05-09 16:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11629 [QA Followup] - Remove <strong> tags to ease translation (1.52 KB, patch)
2014-05-09 16:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11629 [QA Followup] (3.85 KB, patch)
2014-05-09 16:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11629 [QA Followup] - Updating notforloan to 0 gives no "to" status in message (1.99 KB, patch)
2014-05-09 16:58 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2014-01-28 16:42:23 UTC
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.
Comment 1 Kyle M Hall 2014-01-28 16:48:55 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2014-01-28 16:49:27 UTC Comment hidden (obsolete)
Comment 3 Havilah Lyon 2014-01-31 18:31:07 UTC
I tested this. It works fine for us.
Comment 4 Melissa Lefebvre 2014-01-31 18:37:08 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2014-01-31 18:58:16 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2014-01-31 19:00:01 UTC Comment hidden (obsolete)
Comment 7 Katrin Fischer 2014-02-03 21:27:46 UTC
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.
Comment 8 Jonathan Druart 2014-02-12 09:51:44 UTC
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.
Comment 9 Kyle M Hall 2014-02-13 18:50:26 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2014-02-24 13:39:53 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2014-02-24 13:40:12 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2014-02-24 13:40:23 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2014-02-24 13:42:33 UTC
(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!
Comment 14 Kyle M Hall 2014-02-24 13:47:06 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2014-04-07 23:16:41 UTC
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)
Comment 16 Kyle M Hall 2014-04-17 11:10:20 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2014-04-17 13:49:04 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2014-04-17 13:53:36 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2014-04-17 13:58:38 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2014-04-17 14:00:21 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2014-04-17 14:00:25 UTC Comment hidden (obsolete)
Comment 22 Katrin Fischer 2014-04-17 14:00:29 UTC Comment hidden (obsolete)
Comment 23 Katrin Fischer 2014-04-17 14:00:33 UTC Comment hidden (obsolete)
Comment 24 Katrin Fischer 2014-04-17 14:00:37 UTC Comment hidden (obsolete)
Comment 25 Katrin Fischer 2014-04-17 14:00:41 UTC Comment hidden (obsolete)
Comment 26 Galen Charlton 2014-05-05 22:41:26 UTC
Patch does not apply.
Comment 27 Kyle M Hall 2014-05-09 16:57:25 UTC
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.
Comment 28 Kyle M Hall 2014-05-09 16:57:52 UTC
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>
Comment 29 Kyle M Hall 2014-05-09 16:57:54 UTC
Created attachment 28149 [details] [review]
Bug 11629 [QA Followup] - Unit Tests

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 30 Kyle M Hall 2014-05-09 16:57:57 UTC
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>
Comment 31 Kyle M Hall 2014-05-09 16:58:00 UTC
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>
Comment 32 Kyle M Hall 2014-05-09 16:58:02 UTC
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>
Comment 33 Galen Charlton 2014-05-26 03:35:58 UTC
Pushed to master.  Thanks, Kyle!
Comment 34 Katrin Fischer 2014-11-02 11:34:39 UTC
*** Bug 6382 has been marked as a duplicate of this bug. ***