Libriotech / koha-illbackend-libris

Koha ILL Backend for Libris ILL (used in Sweden)
3 stars 9 forks source link

Add support for renewals #114

Closed taskula closed 2 years ago

taskula commented 2 years ago
MagnusEnger commented 2 years ago

The renew screen looks fine and the comment is saved as expected. But in my testing, the illrequestattributes due_date_guar and due_date_max are not updated. And I do not see a mention of the date_due_period in the new code. Am I missing something because I'm not testing on top of #112?

taskula commented 2 years ago

But in my testing, the illrequestattributes due_date_guar and due_date_max are not updated

Fixed by 77b06bb

And I do not see a mention of the date_due_period in the new code. Am I missing something because I'm not testing on top of https://github.com/Libriotech/koha-illbackend-libris/pull/112?

No, you are not missing anything. I forgot to use the new configuration variable. Fixed by 205e1b9.

MagnusEnger commented 2 years ago

Sorry, one more thing... It looks like only one of due_date_guar and due_date_max is updated in the illrequestattributes table? Both of them should be updated from the respective fields in the "renew" screen. Then illrequests.date_due should be updated with the value from either due_date_guar or due_date_max, depending on the due_date_period config variable.

Adding the datepicker to the date fields is a very nice touch, but it looks like it is missing some conversion between the date format in Koha and the internal YYYY-MM-DD format. I see "02/24/2022" in the form, and that is also stored in the database:

> select * from illrequestattributes where illrequest_id = 789 and type in ( 'due_date_guar','due_date_max' );
+---------------+---------------+------------+----------+
| illrequest_id | type          | value      | readonly |
+---------------+---------------+------------+----------+
|           789 | due_date_guar | 02/24/2022 |        1 |
|           789 | due_date_max  | 2022-12-24 |        1 |
+---------------+---------------+------------+----------+
taskula commented 2 years ago

Sorry, one more thing... It looks like only one of due_date_guar and due_date_max is updated in the illrequestattributes table? Both of them should be updated from the respective fields in the "renew" screen. Then illrequests.date_due should be updated with the value from either due_date_guar or due_date_max, depending on the due_date_period config variable.

Thanks for noticing this. I can reproduce it. I've created a new subroutine Base::_save_due_date() that now centralizes saving and formatting due dates for both receive() and renew() steps. So for a complete test, please also test receival. It now updates both fields in illrequestattributes.

Format "yyyy-mm-dd" (DateTime->ymd()) is now used for both illrequestattributes due_date_guar and max as well as illrequests.date_due.

MagnusEnger commented 2 years ago

One small thing missing when I test receiving now: It looks like illrequests.date_due is not set?

taskula commented 2 years ago

It looks like store() was missing.

MagnusEnger commented 2 years ago

All good now! Thanks for all your fixes!