|
Description
vimal kumar
2011-08-27 05:23:07 UTC
You can allow librarians to override the not for loan setting via a system preference. Also the configuration can be changed from not for loan to an overnight (1 day loan period) loan. I am not sure, if that fixes the problem, so please reopen if there is more functionality needed. I think this needs to be reopened and I have another example. Basically if you have a 1 day loan then the item is due anytime the next day, but we need it to be due in the morning - at a specific time. For example let's say you check out a 2 hour loan 1 hour before closing. It would be overdue if returned the next day, but most libraries allow for the item to be returned at opening the next morning. This is a big issue - bigger than the circ matrix - but it is a desired feature. Maybe the addition of library hours to the calendar and a check of that to figure out if the items should be due an hour after the library closes (which is silly) or the next morning during the first open hour. Having an Opening Hours element to the Calendar would be useful for us, we have a few lapsafe machines that show the date and time of return on the machine - it currently says that laptops are due back at 23:59 because the circ rules say that they are loaned for 0 days (i.e. same day return). This is misleading as the libraries are not open at that time. In our scenario, the item would have to be due back before the library closes, which is different from what Nicole mentions. This could work in a similar way to the hard due date in the circ rules, it would be a hard due time for a particular item type. Lapsafe are not able to alter the display, and I'm not aware of a way in which Koha can exclude a time element from the issue transaction. Hi all, this is a required feature for one of our libraries so I will give it a go! Just wanting to clarify some behaviour - would we prefer for the item to be due at closing time, or does their loan period get extended to opening time the following open day? For example, my library closes at midnight (12am) and opens at 8am tomorrow. If I issue a 2 hour loan at 11pm, should the item be due in one hour at 12am (closing time), or at 8am tomorrow (opening time)? Created attachment 124669 [details] [review] Bug 6796: Add branch_hours table and set opening hours for library Created attachment 124670 [details] [review] Bug 6796: Updated schema files Created attachment 124671 [details] [review] Bug 6796: ConsiderLibraryHoursWhenIssuing system preference Created attachment 124672 [details] [review] Bug 6796: Consider library hours when calculating due date + tests This feature adds the ability to set opening and closing hours for your library and for these hours to be considered when calculating due dates for hourly loans. If the due date for an hourly loan falls after the library closes, the library can choose for the due date to be shortened to meet the close time, or extended to meet the open time the next day. This feature adds a new table 'branch_hours' for storing the open and close times per day for each library, and a new system preference 'ConsiderLibraryHoursWhenIssuing' to choose which behaviour should be followed when calculating due dates. To test: 1. Apply patches and update database. Upgrade schema if not applying patch with schema changes. Restart services. 2. Go to Administration -> Libraries. Edit a library and scroll to the bottom to find the 'opening hours' section. Test adding and removing open and close times on various days. Confirm saving works as expected. 3. Add a new library and test adding open and close times works as expected. 4. Edit your default library and save open and close times for each day. 5. Go to Administration -> Circulation and fine rules. Edit a rule, set the unit to 'hours' and set the loan period to a number that would cause a checkout to be due after the close time you just set, i.e. if you set your close time to be 5pm and your system time is currently 1pm, set the loan period to be 5 (5 hours) so the calculated due date would be 6pm. 6. Go to Administration -> system preferences. Search for ConsiderLibraryHoursWhenIssuing. It should be under 'Checkout policy' in the Circulation system preferences. Confirm the pre-selected option is 'ignore'. Keep this tab open. 6. In a new tab, get the barcode for an item that has an itemtype matching the circulation rule you just set. 7. Go to the checkouts for a patron that has a categorycode matching the circulation rule you just set. 8. Check out your item. Confirm that the checkout is due at the end of the loan period, not taking closing hours into consideration. Return the item. 9. Back in your other tab, set ConsiderLibraryHoursWhenIssuing to 'close', so the due date should be shortened to meet the close time. 10. Check out your item. Confirm the checkout is due when the library closes. Return the item. 11. Back in your other tab, set ConsiderLibraryHoursWhenIssuing to 'open', so the due date should be extended to meet the opening time. 12. Check out your item. Confirm the checkout is due the next day when the library opens. 13. Confirm tests pass t/db_dependent/Circulation/CalcDateDue.t Sponsored-by: Catalyst IT Created attachment 134323 [details] [review] Bug 6796: Add branch_hours table and set opening hours for library Created attachment 134324 [details] [review] Bug 6796: Updated schema files Created attachment 134325 [details] [review] Bug 6796: ConsiderLibraryHoursWhenIssuing system preference Created attachment 134326 [details] [review] Bug 6796: Consider library hours when calculating due date + tests This feature adds the ability to set opening and closing hours for your library and for these hours to be considered when calculating due dates for hourly loans. If the due date for an hourly loan falls after the library closes, the library can choose for the due date to be shortened to meet the close time, or extended to meet the open time the next day. This feature adds a new table 'branch_hours' for storing the open and close times per day for each library, and a new system preference 'ConsiderLibraryHoursWhenIssuing' to choose which behaviour should be followed when calculating due dates. To test: 1. Apply patches and update database. Upgrade schema if not applying patch with schema changes. Restart services. 2. Go to Administration -> Libraries. Edit a library and scroll to the bottom to find the 'opening hours' section. Test adding and removing open and close times on various days. Confirm saving works as expected. 3. Add a new library and test adding open and close times works as expected. 4. Edit your default library and save open and close times for each day. 5. Go to Administration -> Circulation and fine rules. Edit a rule, set the unit to 'hours' and set the loan period to a number that would cause a checkout to be due after the close time you just set, i.e. if you set your close time to be 5pm and your system time is currently 1pm, set the loan period to be 5 (5 hours) so the calculated due date would be 6pm. 6. Go to Administration -> system preferences. Search for ConsiderLibraryHoursWhenIssuing. It should be under 'Checkout policy' in the Circulation system preferences. Confirm the pre-selected option is 'ignore'. Keep this tab open. 6. In a new tab, get the barcode for an item that has an itemtype matching the circulation rule you just set. 7. Go to the checkouts for a patron that has a categorycode matching the circulation rule you just set. 8. Check out your item. Confirm that the checkout is due at the end of the loan period, not taking closing hours into consideration. Return the item. 9. Back in your other tab, set ConsiderLibraryHoursWhenIssuing to 'close', so the due date should be shortened to meet the close time. 10. Check out your item. Confirm the checkout is due when the library closes. Return the item. 11. Back in your other tab, set ConsiderLibraryHoursWhenIssuing to 'open', so the due date should be extended to meet the opening time. 12. Check out your item. Confirm the checkout is due the next day when the library opens. 13. Confirm tests pass t/db_dependent/Circulation/CalcDateDue.t Sponsored-by: Catalyst IT Created attachment 134327 [details] [review] Bug 6796: (follow-up) Fix logic for calculating following day's open hours Since days for branch hours are stored as 0-6 in the database, when it's a Saturday (6) incrementing the date leads to an error when issuing an hourly loan because no opening hours are found for the non-existent day (7). This patch fixes this by calculating the tomorrow day and setting it to 0 if it's greater than 6. This patch also corrects the mappings for days, where local_day_of_week caluclates the date with Sunday first, which put it out of sync with the database opening hours days. Sponsored-by: Auckland University of Technology Sponsored-by: Catalyst IT Could not get patches to apply in sandbox Created attachment 145669 [details] [review] Bug 6796: Add branch_hours table and set opening hours for library Created attachment 145670 [details] [review] Bug 6796: ConsiderLibraryHoursWhenIssuing system preference Created attachment 145671 [details] [review] Bug 6796: Consider library hours when calculating due date + tests This feature adds the ability to set opening and closing hours for your library and for these hours to be considered when calculating due dates for hourly loans. If the due date for an hourly loan falls after the library closes, the library can choose for the due date to be shortened to meet the close time, or extended to meet the open time the next day. This feature adds a new table 'branch_hours' for storing the open and close times per day for each library, and a new system preference 'ConsiderLibraryHoursWhenIssuing' to choose which behaviour should be followed when calculating due dates. To test: 1. Apply patches and update database. Upgrade schema if not applying patch with schema changes. Restart services. 2. Go to Administration -> Libraries. Edit a library and scroll to the bottom to find the 'opening hours' section. Test adding and removing open and close times on various days. Confirm saving works as expected. 3. Add a new library and test adding open and close times works as expected. 4. Edit your default library and save open and close times for each day. 5. Go to Administration -> Circulation and fine rules. Edit a rule, set the unit to 'hours' and set the loan period to a number that would cause a checkout to be due after the close time you just set, i.e. if you set your close time to be 5pm and your system time is currently 1pm, set the loan period to be 5 (5 hours) so the calculated due date would be 6pm. 6. Go to Administration -> system preferences. Search for ConsiderLibraryHoursWhenIssuing. It should be under 'Checkout policy' in the Circulation system preferences. Confirm the pre-selected option is 'ignore'. Keep this tab open. 6. In a new tab, get the barcode for an item that has an itemtype matching the circulation rule you just set. 7. Go to the checkouts for a patron that has a categorycode matching the circulation rule you just set. 8. Check out your item. Confirm that the checkout is due at the end of the loan period, not taking closing hours into consideration. Return the item. 9. Back in your other tab, set ConsiderLibraryHoursWhenIssuing to 'close', so the due date should be shortened to meet the close time. 10. Check out your item. Confirm the checkout is due when the library closes. Return the item. 11. Back in your other tab, set ConsiderLibraryHoursWhenIssuing to 'open', so the due date should be extended to meet the opening time. 12. Check out your item. Confirm the checkout is due the next day when the library opens. 13. Confirm tests pass t/db_dependent/Circulation/CalcDateDue.t Sponsored-by: Catalyst IT Created attachment 145672 [details] [review] Bug 6796: (follow-up) Fix logic for calculating following day's open hours Since days for branch hours are stored as 0-6 in the database, when it's a Saturday (6) incrementing the date leads to an error when issuing an hourly loan because no opening hours are found for the non-existent day (7). This patch fixes this by calculating the tomorrow day and setting it to 0 if it's greater than 6. This patch also corrects the mappings for days, where local_day_of_week caluclates the date with Sunday first, which put it out of sync with the database opening hours days. Sponsored-by: Auckland University of Technology Sponsored-by: Catalyst IT Created attachment 145673 [details] [review] Bug 6796: Updated schema files
> 2. Go to Administration -> Libraries. Edit a library and scroll to the
> bottom to find the 'opening hours' section. Test adding and removing
> open and close times on various days. Confirm saving works as expected.
When I clicked the button to edit a library, I got the error: Template process failed: undef error - The method Koha::SMTP::Servers->id is not covered by tests!
These are the first few lines of the stack trace, in case it is helpful:
Trace begun at /kohadevbox/koha/Koha/Objects.pm line 572
Koha::Objects::AUTOLOAD('Koha::SMTP::Servers=HASH(0x55588e5e86a0)') called at /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/modules/admin/branches.tt line 237
eval {...} at /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/modules/admin/branches.tt line 237
eval {...} at /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/modules/admin/branches.tt line 252
eval {...} at /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/modules/admin/branches.tt line 18
Thanks Emily, please mark bug reports as Failed QA if they don't pass the test plan :) Created attachment 146988 [details] [review] Bug 6796: Add branch_hours table and set opening hours for library Created attachment 146989 [details] [review] Bug 6796: ConsiderLibraryHoursWhenIssuing system preference Created attachment 146990 [details] [review] Bug 6796: Consider library hours when calculating due date + tests This feature adds the ability to set opening and closing hours for your library and for these hours to be considered when calculating due dates for hourly loans. If the due date for an hourly loan falls after the library closes, the library can choose for the due date to be shortened to meet the close time, or extended to meet the open time the next day. This feature adds a new table 'branch_hours' for storing the open and close times per day for each library, and a new system preference 'ConsiderLibraryHoursWhenIssuing' to choose which behaviour should be followed when calculating due dates. To test: 1. Apply patches and update database. Upgrade schema if not applying patch with schema changes. Restart services. 2. Go to Administration -> Libraries. Edit a library and scroll to the bottom to find the 'opening hours' section. Test adding and removing open and close times on various days. Confirm saving works as expected. 3. Add a new library and test adding open and close times works as expected. 4. Edit your default library and save open and close times for each day. 5. Go to Administration -> Circulation and fine rules. Edit a rule, set the unit to 'hours' and set the loan period to a number that would cause a checkout to be due after the close time you just set, i.e. if you set your close time to be 5pm and your system time is currently 1pm, set the loan period to be 5 (5 hours) so the calculated due date would be 6pm. 6. Go to Administration -> system preferences. Search for ConsiderLibraryHoursWhenIssuing. It should be under 'Checkout policy' in the Circulation system preferences. Confirm the pre-selected option is 'ignore'. Keep this tab open. 6. In a new tab, get the barcode for an item that has an itemtype matching the circulation rule you just set. 7. Go to the checkouts for a patron that has a categorycode matching the circulation rule you just set. 8. Check out your item. Confirm that the checkout is due at the end of the loan period, not taking closing hours into consideration. Return the item. 9. Back in your other tab, set ConsiderLibraryHoursWhenIssuing to 'close', so the due date should be shortened to meet the close time. 10. Check out your item. Confirm the checkout is due when the library closes. Return the item. 11. Back in your other tab, set ConsiderLibraryHoursWhenIssuing to 'open', so the due date should be extended to meet the opening time. 12. Check out your item. Confirm the checkout is due the next day when the library opens. 13. Confirm tests pass t/db_dependent/Circulation/CalcDateDue.t Sponsored-by: Catalyst IT Created attachment 146991 [details] [review] Bug 6796: (follow-up) Fix logic for calculating following day's open hours Since days for branch hours are stored as 0-6 in the database, when it's a Saturday (6) incrementing the date leads to an error when issuing an hourly loan because no opening hours are found for the non-existent day (7). This patch fixes this by calculating the tomorrow day and setting it to 0 if it's greater than 6. This patch also corrects the mappings for days, where local_day_of_week caluclates the date with Sunday first, which put it out of sync with the database opening hours days. Sponsored-by: Auckland University of Technology Sponsored-by: Catalyst IT Created attachment 146992 [details] [review] Bug 6796: Updated schema files Thanks for the follow-up Aleisha! Unfortunately, I'm still getting some errors when trying to edit a library (testing on KTD). When editing an existing library, upon saving: "An error occurred when updating this library. Perhaps it already exists." When creating a new library, upon saving: "An error occurred when adding this library. The branchcode might already exist." The above two errors occurred consistently regardless of which (if any) fields I edited or what values I entered in those fields. --- It's also failing tests: # Failed test 'Loan period was shortened (but considers the holiday) because ConsiderLibraryHoursWhenIssuing is set to close time' # at t/db_dependent/Circulation/CalcDateDue.t line 413. # got: '2023-02-27T20:35:41' # expected: '2023-03-01T20:35:41' # Failed test 'Loan period was extended (but considers the holiday) because ConsiderLibraryHoursWhenIssuing is set to open time' # at t/db_dependent/Circulation/CalcDateDue.t line 420. # got: '2023-02-28T14:35:41' # expected: '2023-03-01T14:35:41' Created attachment 150442 [details] [review] Bug 6796: Add branch_hours table and set opening hours for library Created attachment 150443 [details] [review] Bug 6796: ConsiderLibraryHoursWhenIssuing system preference Created attachment 150444 [details] [review] Bug 6796: Consider library hours when calculating due date + tests This feature adds the ability to set opening and closing hours for your library and for these hours to be considered when calculating due dates for hourly loans. If the due date for an hourly loan falls after the library closes, the library can choose for the due date to be shortened to meet the close time, or extended to meet the open time the next day. This feature adds a new table 'branch_hours' for storing the open and close times per day for each library, and a new system preference 'ConsiderLibraryHoursWhenIssuing' to choose which behaviour should be followed when calculating due dates. To test: 1. Apply patches and update database. Upgrade schema if not applying patch with schema changes. Restart services. 2. Go to Administration -> Libraries. Edit a library and scroll to the bottom to find the 'opening hours' section. Test adding and removing open and close times on various days. Confirm saving works as expected. 3. Add a new library and test adding open and close times works as expected. 4. Edit your default library and save open and close times for each day. 5. Go to Administration -> Circulation and fine rules. Edit a rule, set the unit to 'hours' and set the loan period to a number that would cause a checkout to be due after the close time you just set, i.e. if you set your close time to be 5pm and your system time is currently 1pm, set the loan period to be 5 (5 hours) so the calculated due date would be 6pm. 6. Go to Administration -> system preferences. Search for ConsiderLibraryHoursWhenIssuing. It should be under 'Checkout policy' in the Circulation system preferences. Confirm the pre-selected option is 'ignore'. Keep this tab open. 6. In a new tab, get the barcode for an item that has an itemtype matching the circulation rule you just set. 7. Go to the checkouts for a patron that has a categorycode matching the circulation rule you just set. 8. Check out your item. Confirm that the checkout is due at the end of the loan period, not taking closing hours into consideration. Return the item. 9. Back in your other tab, set ConsiderLibraryHoursWhenIssuing to 'close', so the due date should be shortened to meet the close time. 10. Check out your item. Confirm the checkout is due when the library closes. Return the item. 11. Back in your other tab, set ConsiderLibraryHoursWhenIssuing to 'open', so the due date should be extended to meet the opening time. 12. Check out your item. Confirm the checkout is due the next day when the library opens. 13. Confirm tests pass t/db_dependent/Circulation/CalcDateDue.t Sponsored-by: Catalyst IT Created attachment 150445 [details] [review] Bug 6796: (follow-up) Fix logic for calculating following day's open hours Since days for branch hours are stored as 0-6 in the database, when it's a Saturday (6) incrementing the date leads to an error when issuing an hourly loan because no opening hours are found for the non-existent day (7). This patch fixes this by calculating the tomorrow day and setting it to 0 if it's greater than 6. This patch also corrects the mappings for days, where local_day_of_week caluclates the date with Sunday first, which put it out of sync with the database opening hours days. Sponsored-by: Auckland University of Technology Sponsored-by: Catalyst IT Created attachment 150446 [details] [review] Bug 6796: Updated schema files Created attachment 150447 [details] [review] Bug 6796: Fix atomic updates Created attachment 150448 [details] [review] Bug 6796: Fix saving of libraries and tests Aleisha, There are some things the QA script caught that need to be cleaned up: Missing filters: OK installer/data/mysql/mandatory/sysprefs.sql FAIL koha-tmpl/intranet-tmpl/prog/en/modules/admin/branches.tt FAIL filters missing_filter at line 292 ( <tr id="hours_[% daycount %]">) missing_filter at line 295 ( <input type="hidden" value="[% daycount %]" name="day">) missing_filter at line 299 ( <input type="time" value="[% hr.open_time %]" name="open_time">) missing_filter at line 306 ( <input type="time" value="[% hr.close_time %]" name="close_time">) missing_filter at line 316 ( <tr id="hours_[% day %]">) missing_filter at line 319 ( <input type="hidden" value="[% daycount %]" name="day">) DB columns: FAIL installer/data/mysql/kohastructure.sql FAIL boolean_vs_tinyint WARNING - The new column (day) for table branch_hours is using INT(1) as type, must be TINYINT(1) if it has a boolean purpose, see the SQL12 coding guideline Created attachment 150477 [details] [review] Bug 6796: (follow-up) Fix branch_hours.day to enum and add filters Created attachment 150478 [details] [review] Bug 6796: (follow-up) Schema changes I'm in the process of testing this and so far shortening the loan period seems to be working for me, but extending the loan period isn't working as expected. Steps taken to replicate my issue 1) Set library opening time to 8:30am closing time to 9pm 2) set loan period to be 3 hours. 3) Set the "ConsiderLibraryHoursWhenIssuing" preference to the extend option. 4) checked out an item. My checkout time and date was 06/01/2023 19:07 - with the loan period I selected (3 hours), the book would be due on 06/01/2023 @ 22:07, but since the library closes at 9pm (21:00), the due date should be extended to the next day at opening time. 5) The shown due date is 06/03/2023 at 08:30 - The due date that was expected was 06/02/2023 at 08:30 so it skipped over a day for some reason. Created attachment 153252 [details] [review] Bug 6796: Add branch_hours table and set opening hours for library Created attachment 153253 [details] [review] Bug 6796: ConsiderLibraryHoursWhenIssuing system preference Created attachment 153254 [details] [review] Bug 6796: Consider library hours when calculating due date + tests This feature adds the ability to set opening and closing hours for your library and for these hours to be considered when calculating due dates for hourly loans. If the due date for an hourly loan falls after the library closes, the library can choose for the due date to be shortened to meet the close time, or extended to meet the open time the next day. This feature adds a new table 'branch_hours' for storing the open and close times per day for each library, and a new system preference 'ConsiderLibraryHoursWhenIssuing' to choose which behaviour should be followed when calculating due dates. To test: 1. Apply patches and update database. Upgrade schema if not applying patch with schema changes. Restart services. 2. Go to Administration -> Libraries. Edit a library and scroll to the bottom to find the 'opening hours' section. Test adding and removing open and close times on various days. Confirm saving works as expected. 3. Add a new library and test adding open and close times works as expected. 4. Edit your default library and save open and close times for each day. 5. Go to Administration -> Circulation and fine rules. Edit a rule, set the unit to 'hours' and set the loan period to a number that would cause a checkout to be due after the close time you just set, i.e. if you set your close time to be 5pm and your system time is currently 1pm, set the loan period to be 5 (5 hours) so the calculated due date would be 6pm. 6. Go to Administration -> system preferences. Search for ConsiderLibraryHoursWhenIssuing. It should be under 'Checkout policy' in the Circulation system preferences. Confirm the pre-selected option is 'ignore'. Keep this tab open. 6. In a new tab, get the barcode for an item that has an itemtype matching the circulation rule you just set. 7. Go to the checkouts for a patron that has a categorycode matching the circulation rule you just set. 8. Check out your item. Confirm that the checkout is due at the end of the loan period, not taking closing hours into consideration. Return the item. 9. Back in your other tab, set ConsiderLibraryHoursWhenIssuing to 'close', so the due date should be shortened to meet the close time. 10. Check out your item. Confirm the checkout is due when the library closes. Return the item. 11. Back in your other tab, set ConsiderLibraryHoursWhenIssuing to 'open', so the due date should be extended to meet the opening time. 12. Check out your item. Confirm the checkout is due the next day when the library opens. 13. Confirm tests pass t/db_dependent/Circulation/CalcDateDue.t Sponsored-by: Catalyst IT Created attachment 153255 [details] [review] Bug 6796: (follow-up) Fix logic for calculating following day's open hours Since days for branch hours are stored as 0-6 in the database, when it's a Saturday (6) incrementing the date leads to an error when issuing an hourly loan because no opening hours are found for the non-existent day (7). This patch fixes this by calculating the tomorrow day and setting it to 0 if it's greater than 6. This patch also corrects the mappings for days, where local_day_of_week caluclates the date with Sunday first, which put it out of sync with the database opening hours days. Sponsored-by: Auckland University of Technology Sponsored-by: Catalyst IT Created attachment 153256 [details] [review] Bug 6796: Updated schema files Created attachment 153257 [details] [review] Bug 6796: Fix atomic updates Created attachment 153258 [details] [review] Bug 6796: Fix saving of libraries and tests Created attachment 153259 [details] [review] Bug 6796: (follow-up) Fix branch_hours.day to enum and add filters Created attachment 153260 [details] [review] Bug 6796: (follow-up) Schema changes Created attachment 153261 [details] [review] Bug 6796: Don't add a day if hourly loan period pushes due date If ConsiderLibraryHoursWhenIssuing is set to shorten the loan period to the closing time, if the loan period initially pushes the due date to the following day, the day still gets added when calculating the due date. We simply need to hardcode the due time here as the due day is the same as the issue day. We only need to calculate a due date if ConsiderLibraryHoursWhenIssuing is set to extend the loan period to the next opening day, as we'll need to consider holidays/closed days. Created attachment 153271 [details] [review] Bug 6796: Add branch_hours table and set opening hours for library Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 153272 [details] [review] Bug 6796: ConsiderLibraryHoursWhenIssuing system preference Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 153273 [details] [review] Bug 6796: Consider library hours when calculating due date + tests This feature adds the ability to set opening and closing hours for your library and for these hours to be considered when calculating due dates for hourly loans. If the due date for an hourly loan falls after the library closes, the library can choose for the due date to be shortened to meet the close time, or extended to meet the open time the next day. This feature adds a new table 'branch_hours' for storing the open and close times per day for each library, and a new system preference 'ConsiderLibraryHoursWhenIssuing' to choose which behaviour should be followed when calculating due dates. To test: 1. Apply patches and update database. Upgrade schema if not applying patch with schema changes. Restart services. 2. Go to Administration -> Libraries. Edit a library and scroll to the bottom to find the 'opening hours' section. Test adding and removing open and close times on various days. Confirm saving works as expected. 3. Add a new library and test adding open and close times works as expected. 4. Edit your default library and save open and close times for each day. 5. Go to Administration -> Circulation and fine rules. Edit a rule, set the unit to 'hours' and set the loan period to a number that would cause a checkout to be due after the close time you just set, i.e. if you set your close time to be 5pm and your system time is currently 1pm, set the loan period to be 5 (5 hours) so the calculated due date would be 6pm. 6. Go to Administration -> system preferences. Search for ConsiderLibraryHoursWhenIssuing. It should be under 'Checkout policy' in the Circulation system preferences. Confirm the pre-selected option is 'ignore'. Keep this tab open. 6. In a new tab, get the barcode for an item that has an itemtype matching the circulation rule you just set. 7. Go to the checkouts for a patron that has a categorycode matching the circulation rule you just set. 8. Check out your item. Confirm that the checkout is due at the end of the loan period, not taking closing hours into consideration. Return the item. 9. Back in your other tab, set ConsiderLibraryHoursWhenIssuing to 'close', so the due date should be shortened to meet the close time. 10. Check out your item. Confirm the checkout is due when the library closes. Return the item. 11. Back in your other tab, set ConsiderLibraryHoursWhenIssuing to 'open', so the due date should be extended to meet the opening time. 12. Check out your item. Confirm the checkout is due the next day when the library opens. 13. Confirm tests pass t/db_dependent/Circulation/CalcDateDue.t Sponsored-by: Catalyst IT Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 153274 [details] [review] Bug 6796: (follow-up) Fix logic for calculating following day's open hours Since days for branch hours are stored as 0-6 in the database, when it's a Saturday (6) incrementing the date leads to an error when issuing an hourly loan because no opening hours are found for the non-existent day (7). This patch fixes this by calculating the tomorrow day and setting it to 0 if it's greater than 6. This patch also corrects the mappings for days, where local_day_of_week caluclates the date with Sunday first, which put it out of sync with the database opening hours days. Sponsored-by: Auckland University of Technology Sponsored-by: Catalyst IT Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 153275 [details] [review] Bug 6796: Updated schema files Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 153276 [details] [review] Bug 6796: Fix atomic updates Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 153277 [details] [review] Bug 6796: Fix saving of libraries and tests Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 153278 [details] [review] Bug 6796: (follow-up) Fix branch_hours.day to enum and add filters Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 153279 [details] [review] Bug 6796: (follow-up) Schema changes Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 153280 [details] [review] Bug 6796: Don't add a day if hourly loan period pushes due date If ConsiderLibraryHoursWhenIssuing is set to shorten the loan period to the closing time, if the loan period initially pushes the due date to the following day, the day still gets added when calculating the due date. We simply need to hardcode the due time here as the due day is the same as the issue day. We only need to calculate a due date if ConsiderLibraryHoursWhenIssuing is set to extend the loan period to the next opening day, as we'll need to consider holidays/closed days. Signed-off-by: Sam Lau <samalau@gmail.com> Trying to do a first run through here, but I feel like we might want another set of eyes maybe on the date calculations later on: 1) QA Test Tools FAIL C4/Circulation.pm FAIL critic # Variables::ProhibitUnusedVariables: Got 1 violation(s). WARN tidiness The file is less tidy than before (bad/messy lines before: 1100, now: 1106) WARN Koha/Schema/Result/Branch.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 222, now: 226) FAIL Koha/Schema/Result/BranchHour.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 18) WARN admin/branches.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 47, now: 58) WARN installer/data/mysql/atomicupdate/bug_6796_-_add_ConsiderLibraryHoursWhenIssuing_syspref.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 4) WARN installer/data/mysql/atomicupdate/bug_6796_-_add_branch_hours_table.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 8) FAIL t/db_dependent/Circulation/CalcDateDue.t WARN tidiness The file is less tidy than before (bad/messy lines before: 138, now: 164) FAIL valid Global symbol "$categorycode" requires explicit package name (did you forget to declare "my $categorycode"?) Global symbol "$categorycode" requires explicit package name (did you forget to declare "my $categorycode"?) t/db_dependent/Circulation/CalcDateDue.t had compilation errors. The last one I believe might be responsible for the unit tests not passing. Please make always sure to run QA tests and unit tests on rebase: Global symbol "$categorycode" requires explicit package name (did you forget to declare "my $categorycode"?) at t/db_dependent/Circulation/CalcDateDue.t line 414. Execution of t/db_dependent/Circulation/CalcDateDue.t aborted due to compilation errors. # Looks like your test exited with 255 before it could output anything. t/db_dependent/Circulation/CalcDateDue.t .. Dubious, test returned 255 (wstat 65280, 0xff00) 2) Terminology a) ConsiderLibraryHoursWhenIssuing = instead of issue we usually use checkout, but maybe ConsiderLibraryHoursInCirculation would be a good fit? b) We have Koha::Library::Hours.pm, but Koha::Schema::Results::BranchHour - why not use LibraryHour and also library_hours for the table name? c) Issuing should be 'checking out' in system preference description: Take library opening hours into consideration to calculate due date when issuing. 3) Database update The 2 separate database updates for adding the pref and the table could have been combined (not blocker). a) The code comments should be removed form the kohastructure.sql file. +/*!40101 SET @saved_cs_client = @@character_set_client */; +/*!40101 SET character_set_client = utf8 */; +/*!40101 SET character_set_client = @saved_cs_client */; 4) Translatability Our scripts always look for strings inside of HTML tags. Wrapping these in spans will make it much easier on translators and possibly allow to 'reuse' the existing translations for week days from other parts of Koha. +[% BLOCK dayname %] + [% IF day == 0 %] + Monday + [% ELSIF day == 1 %] + Tuesday + [% ELSIF day == 2 %] + Wednesday + [% ELSIF day == 3 %] + Thursday + [% ELSIF day == 4 %] + Friday + [% ELSIF day == 5 %] + Saturday + [% ELSE %] + Sunday + [% END %] +[% END %] 4) Library administration a) The opening and closing times should also appear in the 'summary' table of all libraries. (Don't forget to add to add to column configuration as well!) b) It should also be present in the library 'view' page, that is accessible if you don't have admin permissions. http://localhost:8081/cgi-bin/koha/admin/branches.pl?op=view&branchcode=CPL c) TimeFormat is set to 24h format, but I still get presented with AM/PM on data entry. It also doesn't follow CalendarFirstDayOfWeek. I'd think the first would be more important. Maybe the curbside pickup GUI could give some ideas here as I think we did fix the TimeFormat issue there. Created attachment 156772 [details] [review] Bug 6796: Add library_hours table and set opening hours for library Created attachment 156773 [details] [review] Bug 6796: ConsiderLibraryHoursInCirculation system preference Created attachment 156774 [details] [review] Bug 6796: Consider library hours when calculating due date + tests This feature adds the ability to set opening and closing hours for your library and for these hours to be considered when calculating due dates for hourly loans. If the due date for an hourly loan falls after the library closes, the library can choose for the due date to be shortened to meet the close time, or extended to meet the open time the next day. This feature adds a new table 'branch_hours' for storing the open and close times per day for each library, and a new system preference 'ConsiderLibraryHoursInCirculation' to choose which behaviour should be followed when calculating due dates. To test: 1. Apply patches and update database. Upgrade schema if not applying patch with schema changes. Restart services. 2. Go to Administration -> Libraries. Edit a library and scroll to the bottom to find the 'opening hours' section. Test adding and removing open and close times on various days. Confirm saving works as expected. 3. Add a new library and test adding open and close times works as expected. 4. Edit your default library and save open and close times for each day. 5. Go to Administration -> Circulation and fine rules. Edit a rule, set the unit to 'hours' and set the loan period to a number that would cause a checkout to be due after the close time you just set, i.e. if you set your close time to be 5pm and your system time is currently 1pm, set the loan period to be 5 (5 hours) so the calculated due date would be 6pm. 6. Go to Administration -> system preferences. Search for ConsiderLibraryHoursInCirculation. It should be under 'Checkout policy' in the Circulation system preferences. Confirm the pre-selected option is 'ignore'. Keep this tab open. 6. In a new tab, get the barcode for an item that has an itemtype matching the circulation rule you just set. 7. Go to the checkouts for a patron that has a categorycode matching the circulation rule you just set. 8. Check out your item. Confirm that the checkout is due at the end of the loan period, not taking closing hours into consideration. Return the item. 9. Back in your other tab, set ConsiderLibraryHoursInCirculation to 'close', so the due date should be shortened to meet the close time. 10. Check out your item. Confirm the checkout is due when the library closes. Return the item. 11. Back in your other tab, set ConsiderLibraryHoursInCirculation to 'open', so the due date should be extended to meet the opening time. 12. Check out your item. Confirm the checkout is due the next day when the library opens. 13. Confirm tests pass t/db_dependent/Circulation/CalcDateDue.t Sponsored-by: Catalyst IT Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 156775 [details] [review] Bug 6796: (follow-up) Fix logic for calculating following day's open hours Since days for branch hours are stored as 0-6 in the database, when it's a Saturday (6) incrementing the date leads to an error when issuing an hourly loan because no opening hours are found for the non-existent day (7). This patch fixes this by calculating the tomorrow day and setting it to 0 if it's greater than 6. This patch also corrects the mappings for days, where local_day_of_week caluclates the date with Sunday first, which put it out of sync with the database opening hours days. Sponsored-by: Auckland University of Technology Sponsored-by: Catalyst IT Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 156776 [details] [review] Bug 6796: Fix saving of libraries and tests Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 156777 [details] [review] Bug 6796: Don't add a day if hourly loan period pushes due date If ConsiderLibraryHoursWhenIssuing is set to shorten the loan period to the closing time, if the loan period initially pushes the due date to the following day, the day still gets added when calculating the due date. We simply need to hardcode the due time here as the due day is the same as the issue day. We only need to calculate a due date if ConsiderLibraryHoursWhenIssuing is set to extend the loan period to the next opening day, as we'll need to consider holidays/closed days. Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 156778 [details] [review] Bug 6796: Updated schema files Created attachment 156779 [details] [review] Bug 6796: (follow-up) QA test tool fixes Created attachment 156780 [details] [review] Bug 6796: [WIP] (follow-up) Displaying library hours This patch shows the set library hours on a library's view page. Still to be done: - show on the Libraries summary page - write tests - consider time format settings - fix translateability of day names in Libraries summary page The attached patchset addresses the QA test tools and fixes the erroring test. I could not fix the POD and perltidy warnings about Koha/Schema/Result/Library.pm, Koha/Schema/Result/LibraryHour.pm, and Koha/Library/Hour.pm - any guidance on that would be appreciated. (In reply to Katrin Fischer from comment #59) > > 2) Terminology > > a) ConsiderLibraryHoursWhenIssuing = instead of issue we usually use > checkout, but maybe ConsiderLibraryHoursInCirculation would be a good fit? done > > b) We have Koha::Library::Hours.pm, but Koha::Schema::Results::BranchHour - > why not use LibraryHour and also library_hours for the table name? done > > c) Issuing should be 'checking out' in system preference description: > Take library opening hours into consideration to calculate due date when > issuing. changed to 'when circulating' to match the syspref name > > 3) Database update > > The 2 separate database updates for adding the pref and the table could have > been combined (not blocker). Done, QA tools complained about this too > > a) The code comments should be removed form the kohastructure.sql file. > > +/*!40101 SET @saved_cs_client = @@character_set_client */; > +/*!40101 SET character_set_client = utf8 */; > +/*!40101 SET character_set_client = @saved_cs_client */; done > > 4) Translatability > > Our scripts always look for strings inside of HTML tags. Wrapping these in > spans will make it much easier on translators and possibly allow to 'reuse' > the existing translations for week days from other parts of Koha. > > +[% BLOCK dayname %] > + [% IF day == 0 %] > + Monday > + [% ELSIF day == 1 %] > + Tuesday > + [% ELSIF day == 2 %] > + Wednesday > + [% ELSIF day == 3 %] > + Thursday > + [% ELSIF day == 4 %] > + Friday > + [% ELSIF day == 5 %] > + Saturday > + [% ELSE %] > + Sunday > + [% END %] > +[% END %] > done ... still WIP for the libraries summary page > > 5) Library administration > > b) It should also be present in the library 'view' page, that is accessible > if you don't have admin permissions. > http://localhost:8081/cgi-bin/koha/admin/branches.pl?op=view&branchcode=CPL > Done > > a) The opening and closing times should also appear in the 'summary' table > of all libraries. (Don't forget to add to add to column configuration as > well!) > c) TimeFormat is set to 24h format, but I still get presented with AM/PM on > data entry. It also doesn't follow CalendarFirstDayOfWeek. I'd think the > first would be more important. Maybe the curbside pickup GUI could give some > ideas here as I think we did fix the TimeFormat issue there. These two requests have added a lot of complexity to this - adding new columns to an API generated table is not documented anywhere. I don't have capacity to work on this for a while, but maybe someone can build upon what I've done in the most recent patch. Are you still intending to follow-up here Aleisha? Created attachment 161970 [details] [review] Bug 6796: Add library_hours table and set opening hours for library Created attachment 161971 [details] [review] Bug 6796: ConsiderLibraryHoursInCirculation system preference Created attachment 161972 [details] [review] Bug 6796: Consider library hours when calculating due date + tests This feature adds the ability to set opening and closing hours for your library and for these hours to be considered when calculating due dates for hourly loans. If the due date for an hourly loan falls after the library closes, the library can choose for the due date to be shortened to meet the close time, or extended to meet the open time the next day. This feature adds a new table 'branch_hours' for storing the open and close times per day for each library, and a new system preference 'ConsiderLibraryHoursInCirculation' to choose which behaviour should be followed when calculating due dates. To test: 1. Apply patches and update database. Upgrade schema if not applying patch with schema changes. Restart services. 2. Go to Administration -> Libraries. Edit a library and scroll to the bottom to find the 'opening hours' section. Test adding and removing open and close times on various days. Confirm saving works as expected. 3. Add a new library and test adding open and close times works as expected. 4. Edit your default library and save open and close times for each day. 5. Go to Administration -> Circulation and fine rules. Edit a rule, set the unit to 'hours' and set the loan period to a number that would cause a checkout to be due after the close time you just set, i.e. if you set your close time to be 5pm and your system time is currently 1pm, set the loan period to be 5 (5 hours) so the calculated due date would be 6pm. 6. Go to Administration -> system preferences. Search for ConsiderLibraryHoursInCirculation. It should be under 'Checkout policy' in the Circulation system preferences. Confirm the pre-selected option is 'ignore'. Keep this tab open. 6. In a new tab, get the barcode for an item that has an itemtype matching the circulation rule you just set. 7. Go to the checkouts for a patron that has a categorycode matching the circulation rule you just set. 8. Check out your item. Confirm that the checkout is due at the end of the loan period, not taking closing hours into consideration. Return the item. 9. Back in your other tab, set ConsiderLibraryHoursInCirculation to 'close', so the due date should be shortened to meet the close time. 10. Check out your item. Confirm the checkout is due when the library closes. Return the item. 11. Back in your other tab, set ConsiderLibraryHoursInCirculation to 'open', so the due date should be extended to meet the opening time. 12. Check out your item. Confirm the checkout is due the next day when the library opens. 13. Confirm tests pass t/db_dependent/Circulation/CalcDateDue.t Sponsored-by: Catalyst IT Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 161973 [details] [review] Bug 6796: (follow-up) Fix logic for calculating following day's open hours Since days for branch hours are stored as 0-6 in the database, when it's a Saturday (6) incrementing the date leads to an error when issuing an hourly loan because no opening hours are found for the non-existent day (7). This patch fixes this by calculating the tomorrow day and setting it to 0 if it's greater than 6. This patch also corrects the mappings for days, where local_day_of_week caluclates the date with Sunday first, which put it out of sync with the database opening hours days. Sponsored-by: Auckland University of Technology Sponsored-by: Catalyst IT Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 161974 [details] [review] Bug 6796: Fix saving of libraries and tests Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 161975 [details] [review] Bug 6796: Don't add a day if hourly loan period pushes due date If ConsiderLibraryHoursWhenIssuing is set to shorten the loan period to the closing time, if the loan period initially pushes the due date to the following day, the day still gets added when calculating the due date. We simply need to hardcode the due time here as the due day is the same as the issue day. We only need to calculate a due date if ConsiderLibraryHoursWhenIssuing is set to extend the loan period to the next opening day, as we'll need to consider holidays/closed days. Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 161976 [details] [review] Bug 6796: Updated schema files Created attachment 161977 [details] [review] Bug 6796: (follow-up) QA test tool fixes Created attachment 161978 [details] [review] Bug 6796: [WIP] (follow-up) Displaying library hours This patch shows the set library hours on a library's view page. Still to be done: - show on the Libraries summary page - write tests - consider time format settings - fix translateability of day names in Libraries summary page Created attachment 161979 [details] [review] Bug 6796: Fix api specifications and embed hours in librarly management Created attachment 161980 [details] [review] Bug 6796: Fix display of libraries table I rebased it for you and added a couple fo fixes to get it to work.. now I'm working through testing. Created attachment 161986 [details] [review] Bug 6796: (follow-up) Displaying library hours This patch shows the set library hours on a library's view page. Still to be done: - show on the Libraries summary page - write tests - consider time format settings - fix translateability of day names in Libraries summary page Created attachment 161987 [details] [review] Bug 6796: Fix api specifications and embed hours in librarly management Created attachment 161988 [details] [review] Bug 6796: Fix display of libraries table Created attachment 161989 [details] [review] Bug 6796: Take CalenderFirstDayOfWeek into account This patch adds accounting for the CalendarFirstDayOfWeek preference in the libraries summary display table OK, I'm grabbing QA here as Katrin is unlikely to get back to it as the current RM. I'm leaving it at assigned whilst I work through the final touches to your last WIP followup.. those are mostly working now.. so I'm going to now QA. Note to self.. I'd love to display the 'closed days' inline here too.. and it would be great to have a fallback to default for opening hours so you could set at the global level and override at the branch level.. like many of our other options of this style. (In reply to Martin Renvoize from comment #87) > OK, I'm grabbing QA here as Katrin is unlikely to get back to it as the > current RM. > > I'm leaving it at assigned whilst I work through the final touches to your > last WIP followup.. those are mostly working now.. so I'm going to now QA. Thanks Martin. This is something I've had to deprioritise and I'm not sure when I can get back to it, but please let me know if there are specific fixes/problems you want me to look at for you. Created attachment 162043 [details] [review] Bug 6796: Add library_hours table and set opening hours for library Created attachment 162044 [details] [review] Bug 6796: ConsiderLibraryHoursInCirculation system preference Created attachment 162045 [details] [review] Bug 6796: Consider library hours when calculating due date + tests This feature adds the ability to set opening and closing hours for your library and for these hours to be considered when calculating due dates for hourly loans. If the due date for an hourly loan falls after the library closes, the library can choose for the due date to be shortened to meet the close time, or extended to meet the open time the next day. This feature adds a new table 'branch_hours' for storing the open and close times per day for each library, and a new system preference 'ConsiderLibraryHoursInCirculation' to choose which behaviour should be followed when calculating due dates. To test: 1. Apply patches and update database. Upgrade schema if not applying patch with schema changes. Restart services. 2. Go to Administration -> Libraries. Edit a library and scroll to the bottom to find the 'opening hours' section. Test adding and removing open and close times on various days. Confirm saving works as expected. 3. Add a new library and test adding open and close times works as expected. 4. Edit your default library and save open and close times for each day. 5. Go to Administration -> Circulation and fine rules. Edit a rule, set the unit to 'hours' and set the loan period to a number that would cause a checkout to be due after the close time you just set, i.e. if you set your close time to be 5pm and your system time is currently 1pm, set the loan period to be 5 (5 hours) so the calculated due date would be 6pm. 6. Go to Administration -> system preferences. Search for ConsiderLibraryHoursInCirculation. It should be under 'Checkout policy' in the Circulation system preferences. Confirm the pre-selected option is 'ignore'. Keep this tab open. 6. In a new tab, get the barcode for an item that has an itemtype matching the circulation rule you just set. 7. Go to the checkouts for a patron that has a categorycode matching the circulation rule you just set. 8. Check out your item. Confirm that the checkout is due at the end of the loan period, not taking closing hours into consideration. Return the item. 9. Back in your other tab, set ConsiderLibraryHoursInCirculation to 'close', so the due date should be shortened to meet the close time. 10. Check out your item. Confirm the checkout is due when the library closes. Return the item. 11. Back in your other tab, set ConsiderLibraryHoursInCirculation to 'open', so the due date should be extended to meet the opening time. 12. Check out your item. Confirm the checkout is due the next day when the library opens. 13. Confirm tests pass t/db_dependent/Circulation/CalcDateDue.t Sponsored-by: Catalyst IT Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 162046 [details] [review] Bug 6796: (follow-up) Fix logic for calculating following day's open hours Since days for branch hours are stored as 0-6 in the database, when it's a Saturday (6) incrementing the date leads to an error when issuing an hourly loan because no opening hours are found for the non-existent day (7). This patch fixes this by calculating the tomorrow day and setting it to 0 if it's greater than 6. This patch also corrects the mappings for days, where local_day_of_week caluclates the date with Sunday first, which put it out of sync with the database opening hours days. Sponsored-by: Auckland University of Technology Sponsored-by: Catalyst IT Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 162047 [details] [review] Bug 6796: Fix saving of libraries and tests Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 162048 [details] [review] Bug 6796: Don't add a day if hourly loan period pushes due date If ConsiderLibraryHoursWhenIssuing is set to shorten the loan period to the closing time, if the loan period initially pushes the due date to the following day, the day still gets added when calculating the due date. We simply need to hardcode the due time here as the due day is the same as the issue day. We only need to calculate a due date if ConsiderLibraryHoursWhenIssuing is set to extend the loan period to the next opening day, as we'll need to consider holidays/closed days. Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 162049 [details] [review] Bug 6796: Updated schema files Created attachment 162050 [details] [review] Bug 6796: (follow-up) QA test tool fixes Created attachment 162051 [details] [review] Bug 6796: (follow-up) Displaying library hours This patch shows the set library hours on a library's view page. Still to be done: - show on the Libraries summary page - write tests - consider time format settings - fix translateability of day names in Libraries summary page Created attachment 162052 [details] [review] Bug 6796: Fix api specifications and embed hours in librarly management Created attachment 162053 [details] [review] Bug 6796: Fix display of libraries table Created attachment 162054 [details] [review] Bug 6796: Take CalenderFirstDayOfWeek and TimeFormat into account This patch adds accounting for the `CalendarFirstDayOfWeek` and `TimeFormat` preferences in the libraries management pages. We respect CalendarFirstDayOfWeek for both input and display, but we only respect TimeFormat for input at this time. We need a new template helper and js helper for formatting just time in the appropriate format, currently we only have such formatters for full datetimes. Created attachment 162055 [details] [review] Bug 6796: Merge and correct database update Created attachment 162056 [details] [review] Bug 6796: day_of_week representation fix The code here was floored, DateTime returns a 1-7 indexed array with the first day being Monday, Koha expects an 0-6 indexed array with the first day being Sunday. I've fixed a whole bunch of little issues to get this vaguely working now, but as I run the test suit against it I see there are more issues hidden. I've fixed some array indexing issues where your code worked in opposition to how Koha's calendar indexes days of the week already.. but Circulation tests still fail. This is because, as far as I can tell, your code expects every branch to always have opening hours set.. i.e. there MUST be rows for days 0 to 6 for every branch, even if those rows have hours of null-null. I've fixed the database update to handle this, however the failing tests expose the lack of treatment for this in t/lib/TestBuilder. We'll need that resolving before this can work it's way through QA. I'm not sure whether we should be adding this handling to TestBuilder (i.e. to always ensure there's 7 rows per branch for any time a branch is added in testing), or whether really we should not be making this assumption in the first place and instead we should be allowing for missing rows in the library_hours table. Coming back to comment #88 > and it would be great to have a fallback to default for opening hours so you could set at the global level and override at the branch level.. like many of our other options of this style. I think this is the resolution we need for tests.. if we allow library_id to be nullable in library_hours and add a row for each day with default opening hours to fall back to, then handling this in code should mean we can get tests passing more easily. A restless night later and I did a complete about turn.. I came up with a way to defensively code around this instead to protect from bad data ended up in the database somehow. Created attachment 162090 [details] [review] Bug 6796: Add library_hours table and set opening hours for library Sponsored-by: Catalyst IT Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 162091 [details] [review] Bug 6796: ConsiderLibraryHoursInCirculation system preference Sponsored-by: Catalyst IT Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 162092 [details] [review] Bug 6796: Consider library hours when calculating due date + tests This feature adds the ability to set opening and closing hours for your library and for these hours to be considered when calculating due dates for hourly loans. If the due date for an hourly loan falls after the library closes, the library can choose for the due date to be shortened to meet the close time, or extended to meet the open time the next day. This feature adds a new table 'branch_hours' for storing the open and close times per day for each library, and a new system preference 'ConsiderLibraryHoursInCirculation' to choose which behaviour should be followed when calculating due dates. To test: 1. Apply patches and update database. Upgrade schema if not applying patch with schema changes. Restart services. 2. Go to Administration -> Libraries. Edit a library and scroll to the bottom to find the 'opening hours' section. Test adding and removing open and close times on various days. Confirm saving works as expected. 3. Add a new library and test adding open and close times works as expected. 4. Edit your default library and save open and close times for each day. 5. Go to Administration -> Circulation and fine rules. Edit a rule, set the unit to 'hours' and set the loan period to a number that would cause a checkout to be due after the close time you just set, i.e. if you set your close time to be 5pm and your system time is currently 1pm, set the loan period to be 5 (5 hours) so the calculated due date would be 6pm. 6. Go to Administration -> system preferences. Search for ConsiderLibraryHoursInCirculation. It should be under 'Checkout policy' in the Circulation system preferences. Confirm the pre-selected option is 'ignore'. Keep this tab open. 6. In a new tab, get the barcode for an item that has an itemtype matching the circulation rule you just set. 7. Go to the checkouts for a patron that has a categorycode matching the circulation rule you just set. 8. Check out your item. Confirm that the checkout is due at the end of the loan period, not taking closing hours into consideration. Return the item. 9. Back in your other tab, set ConsiderLibraryHoursInCirculation to 'close', so the due date should be shortened to meet the close time. 10. Check out your item. Confirm the checkout is due when the library closes. Return the item. 11. Back in your other tab, set ConsiderLibraryHoursInCirculation to 'open', so the due date should be extended to meet the opening time. 12. Check out your item. Confirm the checkout is due the next day when the library opens. 13. Confirm tests pass t/db_dependent/Circulation/CalcDateDue.t Sponsored-by: Catalyst IT Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 162093 [details] [review] Bug 6796: (follow-up) Fix logic for calculating following day's open hours Since days for branch hours are stored as 0-6 in the database, when it's a Saturday (6) incrementing the date leads to an error when issuing an hourly loan because no opening hours are found for the non-existent day (7). This patch fixes this by calculating the tomorrow day and setting it to 0 if it's greater than 6. This patch also corrects the mappings for days, where local_day_of_week caluclates the date with Sunday first, which put it out of sync with the database opening hours days. Sponsored-by: Catalyst IT Sponsored-by: Auckland University of Technology Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 162094 [details] [review] Bug 6796: Fix saving of libraries and tests Sponsored-by: Catalyst IT Sponsored-by: Auckland University of Technology Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 162095 [details] [review] Bug 6796: Don't add a day if hourly loan period pushes due date If ConsiderLibraryHoursWhenIssuing is set to shorten the loan period to the closing time, if the loan period initially pushes the due date to the following day, the day still gets added when calculating the due date. We simply need to hardcode the due time here as the due day is the same as the issue day. We only need to calculate a due date if ConsiderLibraryHoursWhenIssuing is set to extend the loan period to the next opening day, as we'll need to consider holidays/closed days. Sponsored-by: Catalyst IT Sponsored-by: Auckland University of Technology Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 162096 [details] [review] Bug 6796: Updated schema files Sponsored-by: Catalyst IT Sponsored-by: Auckland University of Technology Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 162097 [details] [review] Bug 6796: (follow-up) QA test tool fixes Sponsored-by: Catalyst IT Sponsored-by: Auckland University of Technology Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 162098 [details] [review] Bug 6796: (follow-up) Displaying library hours This patch shows the set library hours on a library's view page. Still to be done: - show on the Libraries summary page - write tests - consider time format settings - fix translateability of day names in Libraries summary page Sponsored-by: Catalyst IT Sponsored-by: Auckland University of Technology Sponsored-by: PTFS Europe Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 162099 [details] [review] Bug 6796: Fix api specifications and embed hours in librarly management Sponsored-by: PTFS Europe Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 162100 [details] [review] Bug 6796: Fix display of libraries table Sponsored-by: PTFS Europe Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 162101 [details] [review] Bug 6796: Take CalenderFirstDayOfWeek and TimeFormat into account This patch adds accounting for the `CalendarFirstDayOfWeek` and `TimeFormat` preferences in the libraries management pages. We respect CalendarFirstDayOfWeek for both input and display, but we only respect TimeFormat for input at this time. We need a new template helper and js helper for formatting just time in the appropriate format, currently we only have such formatters for full datetimes. Sponsored-by: PTFS Europe Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 162102 [details] [review] Bug 6796: Merge and correct database update Sponsored-by: PTFS Europe Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 162103 [details] [review] Bug 6796: day_of_week representation fix The code here was flawed, DateTime returns a 1-7 indexed array with the first day being Monday, Koha expects an 0-6 indexed array with the first day being Sunday. Sponsored-by: PTFS Europe Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 162104 [details] [review] Bug 6796: Code defensively for if library hours are not set Sponsored-by: PTFS Europe Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> OK.. I think this is 'good enough' at this point. There are follow-ups I'd like to see so I'm going to post bugs for them and link them to here.. but I think the basic feature all works and this is a long long overdue piece of work. We have a foundation to build upon now. I see the status is Passed QA but I'd love to see the changes this bugfix offers... in action. Is a test plan possible so a signoff can be tried in a sandbox? I note the test plan in Comment 109 has 'Apply patches and update database. Upgrade schema if not applying patch with schema changes. Restart services'. I don't see an 'Update database' as an Action. I see Apply patches, Refresh schema and Restart services. Is this one not appropriate for a newbie Signoff-er? I don't believe this should be pushed without the follow-up bug worked on.. it is confusing to the end user as it stands without any meaningful link the the UI to closed days from the calendar. There is a proposal on how to achieve such a link at the data level such that such display and manipulation would be more future-proof and easy to implement in my opinion.. however the mere mention of that idea was immediately dismissed in chat so I'm stepping away again. Created attachment 164511 [details] [review] Bug 6796: Add library_hours table and set opening hours for library Created attachment 164512 [details] [review] Bug 6796: ConsiderLibraryHoursInCirculation system preference Created attachment 164513 [details] [review] Bug 6796: Consider library hours when calculating due date + tests This feature adds the ability to set opening and closing hours for your library and for these hours to be considered when calculating due dates for hourly loans. If the due date for an hourly loan falls after the library closes, the library can choose for the due date to be shortened to meet the close time, or extended to meet the open time the next day. This feature adds a new table 'branch_hours' for storing the open and close times per day for each library, and a new system preference 'ConsiderLibraryHoursInCirculation' to choose which behaviour should be followed when calculating due dates. To test: 1. Apply patches and update database. Upgrade schema if not applying patch with schema changes. Restart services. 2. Go to Administration -> Libraries. Edit a library and scroll to the bottom to find the 'opening hours' section. Test adding and removing open and close times on various days. Confirm saving works as expected. 3. Add a new library and test adding open and close times works as expected. 4. Edit your default library and save open and close times for each day. 5. Go to Administration -> Circulation and fine rules. Edit a rule, set the unit to 'hours' and set the loan period to a number that would cause a checkout to be due after the close time you just set, i.e. if you set your close time to be 5pm and your system time is currently 1pm, set the loan period to be 5 (5 hours) so the calculated due date would be 6pm. 6. Go to Administration -> system preferences. Search for ConsiderLibraryHoursInCirculation. It should be under 'Checkout policy' in the Circulation system preferences. Confirm the pre-selected option is 'ignore'. Keep this tab open. 6. In a new tab, get the barcode for an item that has an itemtype matching the circulation rule you just set. 7. Go to the checkouts for a patron that has a categorycode matching the circulation rule you just set. 8. Check out your item. Confirm that the checkout is due at the end of the loan period, not taking closing hours into consideration. Return the item. 9. Back in your other tab, set ConsiderLibraryHoursInCirculation to 'close', so the due date should be shortened to meet the close time. 10. Check out your item. Confirm the checkout is due when the library closes. Return the item. 11. Back in your other tab, set ConsiderLibraryHoursInCirculation to 'open', so the due date should be extended to meet the opening time. 12. Check out your item. Confirm the checkout is due the next day when the library opens. 13. Confirm tests pass t/db_dependent/Circulation/CalcDateDue.t Sponsored-by: Catalyst IT Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 164514 [details] [review] Bug 6796: (follow-up) Fix logic for calculating following day's open hours Since days for branch hours are stored as 0-6 in the database, when it's a Saturday (6) incrementing the date leads to an error when issuing an hourly loan because no opening hours are found for the non-existent day (7). This patch fixes this by calculating the tomorrow day and setting it to 0 if it's greater than 6. This patch also corrects the mappings for days, where local_day_of_week caluclates the date with Sunday first, which put it out of sync with the database opening hours days. Sponsored-by: Auckland University of Technology Sponsored-by: Catalyst IT Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 164515 [details] [review] Bug 6796: Fix saving of libraries and tests Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 164516 [details] [review] Bug 6796: Don't add a day if hourly loan period pushes due date If ConsiderLibraryHoursWhenIssuing is set to shorten the loan period to the closing time, if the loan period initially pushes the due date to the following day, the day still gets added when calculating the due date. We simply need to hardcode the due time here as the due day is the same as the issue day. We only need to calculate a due date if ConsiderLibraryHoursWhenIssuing is set to extend the loan period to the next opening day, as we'll need to consider holidays/closed days. Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 164517 [details] [review] Bug 6796: Updated schema files Created attachment 164518 [details] [review] Bug 6796: (follow-up) QA test tool fixes CSRF has somehow broken this, but I'm struggling to work out why. Created attachment 164538 [details] [review] Bug 6796: Add library_hours table and set opening hours for library Sponsored-by: Catalyst IT Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 164539 [details] [review] Bug 6796: ConsiderLibraryHoursInCirculation system preference Sponsored-by: Catalyst IT Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 164540 [details] [review] Bug 6796: Consider library hours when calculating due date + tests This feature adds the ability to set opening and closing hours for your library and for these hours to be considered when calculating due dates for hourly loans. If the due date for an hourly loan falls after the library closes, the library can choose for the due date to be shortened to meet the close time, or extended to meet the open time the next day. This feature adds a new table 'branch_hours' for storing the open and close times per day for each library, and a new system preference 'ConsiderLibraryHoursInCirculation' to choose which behaviour should be followed when calculating due dates. To test: 1. Apply patches and update database. Upgrade schema if not applying patch with schema changes. Restart services. 2. Go to Administration -> Libraries. Edit a library and scroll to the bottom to find the 'opening hours' section. Test adding and removing open and close times on various days. Confirm saving works as expected. 3. Add a new library and test adding open and close times works as expected. 4. Edit your default library and save open and close times for each day. 5. Go to Administration -> Circulation and fine rules. Edit a rule, set the unit to 'hours' and set the loan period to a number that would cause a checkout to be due after the close time you just set, i.e. if you set your close time to be 5pm and your system time is currently 1pm, set the loan period to be 5 (5 hours) so the calculated due date would be 6pm. 6. Go to Administration -> system preferences. Search for ConsiderLibraryHoursInCirculation. It should be under 'Checkout policy' in the Circulation system preferences. Confirm the pre-selected option is 'ignore'. Keep this tab open. 6. In a new tab, get the barcode for an item that has an itemtype matching the circulation rule you just set. 7. Go to the checkouts for a patron that has a categorycode matching the circulation rule you just set. 8. Check out your item. Confirm that the checkout is due at the end of the loan period, not taking closing hours into consideration. Return the item. 9. Back in your other tab, set ConsiderLibraryHoursInCirculation to 'close', so the due date should be shortened to meet the close time. 10. Check out your item. Confirm the checkout is due when the library closes. Return the item. 11. Back in your other tab, set ConsiderLibraryHoursInCirculation to 'open', so the due date should be extended to meet the opening time. 12. Check out your item. Confirm the checkout is due the next day when the library opens. 13. Confirm tests pass t/db_dependent/Circulation/CalcDateDue.t Sponsored-by: Catalyst IT Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 164541 [details] [review] Bug 6796: (follow-up) Fix logic for calculating following day's open hours Since days for branch hours are stored as 0-6 in the database, when it's a Saturday (6) incrementing the date leads to an error when issuing an hourly loan because no opening hours are found for the non-existent day (7). This patch fixes this by calculating the tomorrow day and setting it to 0 if it's greater than 6. This patch also corrects the mappings for days, where local_day_of_week caluclates the date with Sunday first, which put it out of sync with the database opening hours days. Sponsored-by: Catalyst IT Sponsored-by: Auckland University of Technology Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 164542 [details] [review] Bug 6796: Fix saving of libraries and tests Sponsored-by: Catalyst IT Sponsored-by: Auckland University of Technology Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 164543 [details] [review] Bug 6796: Don't add a day if hourly loan period pushes due date If ConsiderLibraryHoursWhenIssuing is set to shorten the loan period to the closing time, if the loan period initially pushes the due date to the following day, the day still gets added when calculating the due date. We simply need to hardcode the due time here as the due day is the same as the issue day. We only need to calculate a due date if ConsiderLibraryHoursWhenIssuing is set to extend the loan period to the next opening day, as we'll need to consider holidays/closed days. Sponsored-by: Catalyst IT Sponsored-by: Auckland University of Technology Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 164544 [details] [review] Bug 6796: Updated schema files Sponsored-by: Catalyst IT Sponsored-by: Auckland University of Technology Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 164545 [details] [review] Bug 6796: (follow-up) QA test tool fixes Sponsored-by: Catalyst IT Sponsored-by: Auckland University of Technology Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 164546 [details] [review] Bug 6796: (follow-up) Displaying library hours This patch shows the set library hours on a library's view page. Still to be done: - show on the Libraries summary page - write tests - consider time format settings - fix translateability of day names in Libraries summary page Sponsored-by: Catalyst IT Sponsored-by: Auckland University of Technology Sponsored-by: PTFS Europe Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 164547 [details] [review] Bug 6796: Fix api specifications and embed hours in librarly management Sponsored-by: PTFS Europe Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 164548 [details] [review] Bug 6796: Fix display of libraries table Sponsored-by: PTFS Europe Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 164549 [details] [review] Bug 6796: Take CalenderFirstDayOfWeek and TimeFormat into account This patch adds accounting for the `CalendarFirstDayOfWeek` and `TimeFormat` preferences in the libraries management pages. We respect CalendarFirstDayOfWeek for both input and display, but we only respect TimeFormat for input at this time. We need a new template helper and js helper for formatting just time in the appropriate format, currently we only have such formatters for full datetimes. Sponsored-by: PTFS Europe Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 164550 [details] [review] Bug 6796: Merge and correct database update Sponsored-by: PTFS Europe Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 164551 [details] [review] Bug 6796: day_of_week representation fix The code here was flawed, DateTime returns a 1-7 indexed array with the first day being Monday, Koha expects an 0-6 indexed array with the first day being Sunday. Sponsored-by: PTFS Europe Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 164552 [details] [review] Bug 6796: Code defensively for if library hours are not set Sponsored-by: PTFS Europe Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Ack.. the patches had all got confused.. it is all working again now. Push, push, push... Pushed for 24.05! Well done everyone, thank you! This feature introduced the first usage of the `time` column type, and TestBuilder is not prepared for it. I filed bug 36593 to deal with it, as the failing tests [1] are out of the scope of this particular report. [1] https://jenkins.koha-community.org/job/Koha_Master_D10/lastCompletedBuild/console https://snipboard.io/mpDPSu.jpg I am seeing "null" everywhere (just edited and saved a library, not touched at the opening hours). (In reply to Jonathan Druart from comment #152) > https://snipboard.io/mpDPSu.jpg > > I am seeing "null" everywhere (just edited and saved a library, not touched > at the opening hours). Looks like it has been reported already on bug 36594. New feature, not backported to 23.11.x Is it on purpose that the database update sets the value of the preference to 'ignore', but the sysprefs SQL file sets it to 'close'? |
