|
Description
Donna
2019-06-25 16:49:09 UTC
Created attachment 114212 [details] [review] Bug 23207: Add test Created attachment 114213 [details] [review] Bug 23207: Add automatic_checkin column to itemtypes table installer/data/mysql/kohastructure.sql Created attachment 114214 [details] [review] Bug 23207: Set automatic_checkin column as boolean in Itemtype schema Created attachment 114215 [details] [review] Bug 23207: Add automatic checkin feature This patch adds the posibility to set an itemtype with automatic checkin. It means that when the checkout is due, it will automatically check in. To test: 1. apply patches 2. updatedatabase 3. go to koha administration -> item types and edin an item type (from now on called itemtype1) CHECK => there is a checkbox almost at the end called automatic checkin 4. check that checkbox and save 5. checkout 2 items from itemtype1 and one item from another itemtype (from now on called itemtype2) 6. go to mysql database console (koha-mysql) and manually set date_due = current_timestamp in issues table for the item of itemtype2 and only one of the items of itemtype1 7. run cronjob at misc/cronjobs/automatic_checkin.pl 8. go to mysql database console again and select * from issues SUCCESS => All issues are present except for the issue of itemtype1 which had it's date_due set to current_timestamp. That one was automatically checked in. 9. prove t/db_dependent/Koha/Checkouts.t This sounds like a really interesting feature to handle our ILL copies. Could you please rebase? *** Bug 7716 has been marked as a duplicate of this bug. *** Created attachment 117970 [details] [review] Bug 23207: Add test Created attachment 117971 [details] [review] Bug 23207: Add automatic_checkin column to itemtypes table installer/data/mysql/kohastructure.sql Created attachment 117972 [details] [review] Bug 23207: Set automatic_checkin column as boolean in Itemtype schema Created attachment 117973 [details] [review] Bug 23207: Add automatic checkin feature This patch adds the posibility to set an itemtype with automatic checkin. It means that when the checkout is due, it will automatically check in. To test: 1. apply patches 2. updatedatabase 3. go to koha administration -> item types and edin an item type (from now on called itemtype1) CHECK => there is a checkbox almost at the end called automatic checkin 4. check that checkbox and save 5. checkout 2 items from itemtype1 and one item from another itemtype (from now on called itemtype2) 6. go to mysql database console (koha-mysql) and manually set date_due = current_timestamp in issues table for the item of itemtype2 and only one of the items of itemtype1 7. run cronjob at misc/cronjobs/automatic_checkin.pl 8. go to mysql database console again and select * from issues SUCCESS => All issues are present except for the issue of itemtype1 which had it's date_due set to current_timestamp. That one was automatically checked in. 9. prove t/db_dependent/Koha/Checkouts.t Created attachment 117984 [details] [review] Bug 23207: Add test Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 117985 [details] [review] Bug 23207: Add automatic_checkin column to itemtypes table installer/data/mysql/kohastructure.sql Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 117986 [details] [review] Bug 23207: Set automatic_checkin column as boolean in Itemtype schema Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 117987 [details] [review] Bug 23207: Add automatic checkin feature This patch adds the posibility to set an itemtype with automatic checkin. It means that when the checkout is due, it will automatically check in. To test: 1. apply patches 2. updatedatabase 3. go to koha administration -> item types and edin an item type (from now on called itemtype1) CHECK => there is a checkbox almost at the end called automatic checkin 4. check that checkbox and save 5. checkout 2 items from itemtype1 and one item from another itemtype (from now on called itemtype2) 6. go to mysql database console (koha-mysql) and manually set date_due = current_timestamp in issues table for the item of itemtype2 and only one of the items of itemtype1 7. run cronjob at misc/cronjobs/automatic_checkin.pl 8. go to mysql database console again and select * from issues SUCCESS => All issues are present except for the issue of itemtype1 which had it's date_due set to current_timestamp. That one was automatically checked in. 9. prove t/db_dependent/Koha/Checkouts.t Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Two tiny bits of feedback. On the item type setup page, I'd like to see the checkbox for automatic checkin moved up just underneath the checkbox for Not for Loan. That groups things a bit more thematically. Additionally, the text there currently reads: "If checked, items will be checked in automatically after a period of time. Useful for materials where you really don't care for fisical return, like museum passes, etc." I would suggest: "If checked, items will be automatically checked in once they've reached their due date." This clarifies how the checkin time is specified. I don't think we need a note here suggesting possible usage, that could go in the release notes and manual. If we want to retain that note, however, I 'd reword it as "This is useful for materials that do not require a physical checkin, such as museum passes, etc." Created attachment 117991 [details] [review] Bug 23207: Add test Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Hasina Akhter <HasinaA@pascolibraries.org> Created attachment 117992 [details] [review] Bug 23207: Add automatic_checkin column to itemtypes table installer/data/mysql/kohastructure.sql Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Hasina Akhter <HasinaA@pascolibraries.org> Created attachment 117993 [details] [review] Bug 23207: Set automatic_checkin column as boolean in Itemtype schema Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Hasina Akhter <HasinaA@pascolibraries.org> Created attachment 117994 [details] [review] Bug 23207: Add automatic checkin feature This patch adds the posibility to set an itemtype with automatic checkin. It means that when the checkout is due, it will automatically check in. To test: 1. apply patches 2. updatedatabase 3. go to koha administration -> item types and edin an item type (from now on called itemtype1) CHECK => there is a checkbox almost at the end called automatic checkin 4. check that checkbox and save 5. checkout 2 items from itemtype1 and one item from another itemtype (from now on called itemtype2) 6. go to mysql database console (koha-mysql) and manually set date_due = current_timestamp in issues table for the item of itemtype2 and only one of the items of itemtype1 7. run cronjob at misc/cronjobs/automatic_checkin.pl 8. go to mysql database console again and select * from issues SUCCESS => All issues are present except for the issue of itemtype1 which had it's date_due set to current_timestamp. That one was automatically checked in. 9. prove t/db_dependent/Koha/Checkouts.t Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Hasina Akhter <HasinaA@pascolibraries.org> Created attachment 119726 [details] [review] Bug 23207: Add test Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Hasina Akhter <HasinaA@pascolibraries.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 119727 [details] [review] Bug 23207: Add automatic_checkin column to itemtypes table installer/data/mysql/kohastructure.sql Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Hasina Akhter <HasinaA@pascolibraries.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 119728 [details] [review] Bug 23207: Set automatic_checkin column as boolean in Itemtype schema Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Hasina Akhter <HasinaA@pascolibraries.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 119729 [details] [review] Bug 23207: Add automatic checkin feature This patch adds the posibility to set an itemtype with automatic checkin. It means that when the checkout is due, it will automatically check in. To test: 1. apply patches 2. updatedatabase 3. go to koha administration -> item types and edin an item type (from now on called itemtype1) CHECK => there is a checkbox almost at the end called automatic checkin 4. check that checkbox and save 5. checkout 2 items from itemtype1 and one item from another itemtype (from now on called itemtype2) 6. go to mysql database console (koha-mysql) and manually set date_due = current_timestamp in issues table for the item of itemtype2 and only one of the items of itemtype1 7. run cronjob at misc/cronjobs/automatic_checkin.pl 8. go to mysql database console again and select * from issues SUCCESS => All issues are present except for the issue of itemtype1 which had it's date_due set to current_timestamp. That one was automatically checked in. 9. prove t/db_dependent/Koha/Checkouts.t Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Hasina Akhter <HasinaA@pascolibraries.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 119730 [details] [review] Bug 23207: (QA follow-up) Move option This patch impliment the changes as suggested by Andrew on the bug report. I agree that the suggested location fits better in the page and the updated description is clearer. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Interesting feature.. works as described and contains tests and passes QA scripts. Passing QA I can see lots of use cases for this, it'll be interesting to see how it plays out.. Electronic resources, Inter-library loans etc.. I liked Andrews suggestions, so I went ahead an implemented them as a QA follow-up. Happy to see this in PQA, trying to provide the release notes note. 1. Entry missing in misc/cronjobs/crontab.example 2. Any good reasons to not add the info in a new column of the itemtypes table? 3. (just grumbling) we really need to remove the item type at biblio level (item-level_itypes syspref) and have a FK on items.itemtype. Not having the ability to join here is terrible, we have to loop over all the checkouts... Not considering blocker but we may want to improve perf here in the future using the usual (ugly) condition for the join. 4. Thinking about the edge cases, date handling: don't we want to pass the due date for the return date, in order to not flag the checkin as overdue in the checkout history list? (In reply to Jonathan Druart from comment #27) > 1. Entry missing in misc/cronjobs/crontab.example Good catch. > 2. Any good reasons to not add the info in a new column of the itemtypes > table? You mean is missing the COMMENT? > 3. (just grumbling) we really need to remove the item type at biblio level > (item-level_itypes syspref) and have a FK on items.itemtype. Not having the > ability to join here is terrible, we have to loop over all the checkouts... > Not considering blocker but we may want to improve perf here in the future > using the usual (ugly) condition for the join. This is a broader discussion, but biblio-level itype is really a thing, because it is used for filtering and facets as well. > 4. Thinking about the edge cases, date handling: don't we want to pass the > due date for the return date, in order to not flag the checkin as overdue in > the checkout history list? (In reply to Tomás Cohen Arazi from comment #28) > (In reply to Jonathan Druart from comment #27) > > 1. Entry missing in misc/cronjobs/crontab.example > > Good catch. > > > 2. Any good reasons to not add the info in a new column of the itemtypes > > table? > > You mean is missing the COMMENT? Not sure if it#s there already, but I think waht's meant was administration > itemtypes - showing the setting there in column, so you can filter etc. > > > 3. (just grumbling) we really need to remove the item type at biblio level > > (item-level_itypes syspref) and have a FK on items.itemtype. Not having the > > ability to join here is terrible, we have to loop over all the checkouts... > > Not considering blocker but we may want to improve perf here in the future > > using the usual (ugly) condition for the join. > > This is a broader discussion, but biblio-level itype is really a thing, > because it is used for filtering and facets as well. I agree, we cannot remove the itemtype on biblio-level, it's tied to varioud features and checks (max reserves check for biblio level holds, article requests without items etc.) We could maybe remove it from MARC/only add it for indexing maybe? (In reply to Tomás Cohen Arazi from comment #28) > (In reply to Jonathan Druart from comment #27) > > 1. Entry missing in misc/cronjobs/crontab.example > > Good catch. > > > 2. Any good reasons to not add the info in a new column of the itemtypes > > table? > > You mean is missing the COMMENT? admin/itemtypes.pl => The info is not in the table. (In reply to Katrin Fischer from comment #29) > (In reply to Tomás Cohen Arazi from comment #28) > > (In reply to Jonathan Druart from comment #27) > > > 1. Entry missing in misc/cronjobs/crontab.example > > > > Good catch. > > > > > 2. Any good reasons to not add the info in a new column of the itemtypes > > > table? > > > > You mean is missing the COMMENT? > > Not sure if it#s there already, but I think waht's meant was administration > > itemtypes - showing the setting there in column, so you can filter etc. > > > > > > 3. (just grumbling) we really need to remove the item type at biblio level > > > (item-level_itypes syspref) and have a FK on items.itemtype. Not having the > > > ability to join here is terrible, we have to loop over all the checkouts... > > > Not considering blocker but we may want to improve perf here in the future > > > using the usual (ugly) condition for the join. > > > > This is a broader discussion, but biblio-level itype is really a thing, > > because it is used for filtering and facets as well. > I agree, we cannot remove the itemtype on biblio-level, it's tied to varioud > features and checks (max reserves check for biblio level holds, article > requests without items etc.) We could maybe remove it from MARC/only add it > for indexing maybe? Definitely not the place to have the discussion, but we discussed it already several times and mostly agreed on removing it... Created attachment 119966 [details] [review] Bug 23207: (QA follow-up) Add COMMENT to new column Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 119967 [details] [review] Bug 23207: (QA follow-up) Add new column on item types UI table Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 119973 [details] [review] Bug 23207: (QA follow-up) Add to cron files Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Jonathan Druart from comment #27) > 4. Thinking about the edge cases, date handling: don't we want to pass the > due date for the return date, in order to not flag the checkin as overdue in > the checkout history list? No answer provided. Created attachment 119985 [details] [review] Bug 23207: (QA follow-up) Add COMMENT to new column Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> JD amended patch: FIX syntax error.(swap COMMENT and AFTER) Created attachment 119999 [details] [review] Bug 23207: Regression tests This patch introduces a regression test for the case the automatic checkin happens after the date due. In that case the item souldn't really be overdue, and the checkin should be recorded with the date due as return date. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Checkouts.t => FAIL: The feature doesn't behave correctly Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 120000 [details] [review] Bug 23207: Automatic checkin items should never be overdue This patch makes Koha::Checkouts->automatic_checkin pass the date_due as return date to AddReturn. To test: 1. Apply the regression tests 2. Run: $ kshell k$ prove t/db_dependent/Koha/Checkouts.t => FAIL: The feature is not working properly 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> One last thing, shouldn't we display the "auto checkin" info in the checkout list (current checkouts at circulation.pl and circulation history)? Pushing as it, feel free to follow-up here or open a separate bug report if you think it is a good one to have for 21.05. (In reply to Jonathan Druart from comment #38) > One last thing, shouldn't we display the "auto checkin" info in the checkout > list (current checkouts at circulation.pl and circulation history)? > > Pushing as it, feel free to follow-up here or open a separate bug report if > you think it is a good one to have for 21.05. I will ask the sponsors, but it makes sense. Pushed to master for 21.05, thanks to everybody involved! Enhancement not pushed to 20.11.x |