|
Description
Katrin Fischer
2021-08-03 13:16:13 UTC
Hi Fabricio, any progress on this? Hi Katrin I'm pleased to inform you that the development is almost complete. I just need to finish linking the OnSiteCheckoutAutoCheck preference to make the development more comprehensive. It will be finished this week. Best regards. Fabricio. Thanks for the update! Created attachment 152992 [details] [review] Bug 28805: Add on-site checkout checkbox in batch checkout. Add the 'on-site checkout' checkbox to the 'Batch checkout'. To verify the functionality, follow these steps: 1) Before the modification is added, enable the 'OnSiteCheckouts' and 'BatchCheckout' system preferences. 2) In the 'BatchCheckoutsValidCategories' preference, select all categories. 3) Find a user in the system. For example, Henry Acevedo. 4) In the 'Check out' section, you can see that the 'Checkout settings' option displays the 'On-site Checkout' checkbox when clicked. 5) Check the 'On-site checkout' option. This will display the current date with a runtime deadline of 23:59. 6) Enter a barcode (for example 39999000011418). 7) Click the 'Check out' button. This will show that the loan was made on-site. 8) Check the 'Batch checkout' section. This section does not hace the 'On-site checkout' checkbox. When loading the patch with the addition of the 'on-site checkout' checkbox in 'Batch checkout' starting from step 3), you can already find the checkbox in the 'Batch checkout' section. To use it, you can try the barcodes 39999000006179 and 39999000008494 together. Then continue from step 5). When viewing the loans, you will see that both items (39999000006179 and 39999000008494) will appear with the label 'On-site checkout'. If this patch is deactivated, the functionality will remain the same as it was before this change. Sponsored-by: Banco Central de la República Argentina. Created attachment 153105 [details] [review] Bug 28805: Add on-site checkout checkbox in batch checkout Add the 'on-site checkout' checkbox to the 'Batch checkout'. To verify the functionality, follow these steps: 1) Before the modification is added, enable the 'OnSiteCheckouts' and 'BatchCheckout' system preferences. 2) In the 'BatchCheckoutsValidCategories' preference, select all categories. 3) Find a user in the system. For example, Henry Acevedo. 4) In the 'Check out' section, you can see that the 'Checkout settings' option displays the 'On-site Checkout' checkbox when clicked. 5) Check the 'On-site checkout' option. This will display the current date with a runtime deadline of 23:59. 6) Enter a barcode (for example 39999000011418). 7) Click the 'Check out' button. This will show that the loan was made on-site. 8) Check the 'Batch checkout' section. This section does not hace the 'On-site checkout' checkbox. When loading the patch with the addition of the 'on-site checkout' checkbox in 'Batch checkout' starting from step 3), you can already find the checkbox in the 'Batch checkout' section. To use it, you can try the barcodes 39999000006179 and 39999000008494 together. Then continue from step 5). When viewing the loans, you will see that both items (39999000006179 and 39999000008494) will appear with the label 'On-site checkout'. If this patch is deactivated, the functionality will remain the same as it was before this change. Sponsored-by: Banco Central de la República Argentina Hello Sam. Thank you very much for taking the time to make the correction. I apologize for the mistake made in the previously uploaded patch. Tomás helped me fix it. Could you please review it again?. Thanks! Created attachment 153185 [details] [review] Bug 28805: Add on-site checkout checkbox in batch checkout Add the 'on-site checkout' checkbox to the 'Batch checkout'. To verify the functionality, follow these steps: 1) Before the modification is added, enable the 'OnSiteCheckouts' and 'BatchCheckout' system preferences. 2) In the 'BatchCheckoutsValidCategories' preference, select all categories. 3) Find a user in the system. For example, Henry Acevedo. 4) In the 'Check out' section, you can see that the 'Checkout settings' option displays the 'On-site Checkout' checkbox when clicked. 5) Check the 'On-site checkout' option. This will display the current date with a runtime deadline of 23:59. 6) Enter a barcode (for example 39999000011418). 7) Click the 'Check out' button. This will show that the loan was made on-site. 8) Check the 'Batch checkout' section. This section does not hace the 'On-site checkout' checkbox. When loading the patch with the addition of the 'on-site checkout' checkbox in 'Batch checkout' starting from step 3), you can already find the checkbox in the 'Batch checkout' section. To use it, you can try the barcodes 39999000006179 and 39999000008494 together. Then continue from step 5). When viewing the loans, you will see that both items (39999000006179 and 39999000008494) will appear with the label 'On-site checkout'. If this patch is deactivated, the functionality will remain the same as it was before this change. Sponsored-by: Banco Central de la República Argentina Signed-off-by: Sam Lau <samalau@gmail.com> (In reply to Fabricio Molina from comment #6) > Hello Sam. Thank you very much for taking the time to make the correction. I > apologize for the mistake made in the previously uploaded patch. Tomás > helped me fix it. Could you please review it again?. Thanks! Fabricio, no need to apologize! The bug is now signed off :) 1. If you turn off SpecifyDueDate, the checkbox is no longer displayed
2. Do you really need that?
+ [% Asset.js("js/members-menu.js") | $raw %]
+ [% Asset.js("js/pages/circulation.js") | $raw %]
Comment on attachment 153185 [details] [review] Bug 28805: Add on-site checkout checkbox in batch checkout Review of attachment 153185 [details] [review]: ----------------------------------------------------------------- Hi Fabricio, thanks for working on this! Joubu already made 2 good points, I have a bit of an explanation and one more: ::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation_batch_checkouts.tt @@ +99,4 @@ > </li> > </ol> > </fieldset> > + [% IF Koha.Preference('SpecifyDueDate') && Koha.Preference('OnSiteCheckouts') %] To explain a bit more what Jonathan said: the specify due date feature and the onsite-checkout only partially depend on each other. So this should probably be split into separate conditions or be ||. * If you have both, you are able to change the pre-selected due date for the on-site checkout. * If you have only OnSiteCheckouts, the date is automatically set to today 23:59:00 in the background. * Only SpecifyDueDate works like now. You can compare how it should work with the same options on the checkouts tab. @@ +104,5 @@ > + <legend>Due date:</legend> > + <label for="duedatespec">Hard due date: [% INCLUDE 'date-format.inc' %]:</label> > + <input type="text" size="20" id="duedatespec" name="duedatespec" value="[% duedatespec | html %]" class="flatpickr" data-flatpickr-enable-time="true"/> > + <div id="onsite_checkout-select" class="circ-setting"> > + <div class="circ-setting"> You double-up the circ-setting classes here in 2 different div - this makes the font size smaller than for the other options. Maybe one or both should be removed. Created attachment 154346 [details] [review] Bug 28805: Add on-site checkout checkbox in batch checkout Add the 'on-site checkout' checkbox to the 'Batch checkout'. To verify the functionality, follow these steps: 1) Before the modification is added, enable the 'OnSiteCheckouts' and 'BatchCheckout' system preferences. 2) In the 'BatchCheckoutsValidCategories' preference, select all categories. 3) Find a user in the system. For example, Henry Acevedo. 4) In the 'Check out' section, you can see that the 'Checkout settings' option displays the 'On-site Checkout' checkbox when clicked. 5) Check the 'On-site checkout' option. This will display the current date with a runtime deadline of 23:59. 6) Enter a barcode (for example 39999000011418). 7) Click the 'Check out' button. This will show that the loan was made on-site. 8) Check the 'Batch checkout' section. This section does not hace the 'On-site checkout' checkbox. When loading the patch with the addition of the 'on-site checkout' checkbox in 'Batch checkout' starting from step 3), you can already find the checkbox in the 'Batch checkout' section. To use it, you can try the barcodes 39999000006179 and 39999000008494 together. Then continue from step 5). When viewing the loans, you will see that both items (39999000006179 and 39999000008494) will appear with the label 'On-site checkout'. If this patch is deactivated, the functionality will remain the same as it was before this change. Sponsored-by: Banco Central de la República Argentina Signed-off-by: Sam Lau <samalau@gmail.com> Hi Thanks for your feedback and suggestions. In this fix, the "OnSiteCheckouts" preference can be used without depending on the "SpecifyDueData" preference. Regarding the functionality of being able to modify the preselected due date, I used the same behavior that on-site checkout has for individual loans (the preselected date can also be modified there). I look forward to any feedback. Created attachment 156182 [details] [review] Bug 28805: Add on-site checkout checkbox in batch checkout Add the 'on-site checkout' checkbox to the 'Batch checkout'. To verify the functionality, follow these steps: 1) Before the modification is added, enable the 'OnSiteCheckouts' and 'BatchCheckout' system preferences. 2) In the 'BatchCheckoutsValidCategories' preference, select all categories. 3) Find a user in the system. For example, Henry Acevedo. 4) In the 'Check out' section, you can see that the 'Checkout settings' option displays the 'On-site Checkout' checkbox when clicked. 5) Check the 'On-site checkout' option. This will display the current date with a runtime deadline of 23:59. 6) Enter a barcode (for example 39999000011418). 7) Click the 'Check out' button. This will show that the loan was made on-site. 8) Check the 'Batch checkout' section. This section does not hace the 'On-site checkout' checkbox. When loading the patch with the addition of the 'on-site checkout' checkbox in 'Batch checkout' starting from step 3), you can already find the checkbox in the 'Batch checkout' section. To use it, you can try the barcodes 39999000006179 and 39999000008494 together. Then continue from step 5). When viewing the loans, you will see that both items (39999000006179 and 39999000008494) will appear with the label 'On-site checkout'. If this patch is deactivated, the functionality will remain the same as it was before this change. Sponsored-by: Banco Central de la República Argentina Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 156183 [details] [review] Bug 28805: (QA follow-up) Restore removed asset Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 156184 [details] [review] Bug 28805: (QA follow-up) Pass onsite variable to confirmation screen Signed-off-by: Nick Clemens <nick@bywatersolutions.com> I still got a warning about tidiness, however, the lines I added are tidied and nothing around them is not If SpecifyDueDate and OnSiteCheckouts are both disabled, you see "Due date" at the bottom of the screen. Created attachment 158108 [details] [review] Bug 28805: (follow-up) Fix template logic error This patch fixes a problem in the template logic that made the 'Due date' legend display when `OnSiteCheckouts` and `SpecifyDueDate` are not set, as highlighted by QA. Sponsored-by: Banco Central de la República Argentina (In reply to Jonathan Druart from comment #17) > If SpecifyDueDate and OnSiteCheckouts are both disabled, you see "Due date" > at the bottom of the screen. Sorry for the delay. The last patch fixes the issue. Created attachment 158279 [details] [review] Bug 28805: (follow-up) Fix template logic error This patch fixes a problem in the template logic that made the 'Due date' legend display when `OnSiteCheckouts` and `SpecifyDueDate` are not set, as highlighted by QA. Sponsored-by: Banco Central de la República Argentina Signed-off-by: Nick Clemens <nick@bywatersolutions.com> QA tool is not happy, but the template pattern works and is used elsewhere - I had to tidy the entire issuing block to remove the tidy error, did not seem worth it Pushed to master for 23.11. Nice work everyone, thanks! It looks like this makes the tests failing:
prove -v xt/author/valid-templates.t
# Failed test '/kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation_batch_checkouts.tt'
# at xt/author/valid-templates.t line 141.
# file error - parse error - /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation_batch_checkouts.tt line 103: unexpected token ( Koha.Preference('OnSiteCheckouts'))
# [% IF Koha.Preference('SpecifyDueDate') || Koha.Preference('OnSiteCheckouts') %]
# Looks like you failed 1 test of 822.
Created attachment 158386 [details] [review] Bug 28805: (follow-up) Fix non-printable char that breaks tests (In reply to Katrin Fischer from comment #24) > Created attachment 158386 [details] [review] [review] > Bug 28805: (follow-up) Fix non-printable char that breaks tests Nice job! I don't know how you spotted that C2A0 no-break space, but that was masterful [U+1F44D] (In reply to David Cook from comment #25) > (In reply to Katrin Fischer from comment #24) > > Created attachment 158386 [details] [review] [review] [review] > > Bug 28805: (follow-up) Fix non-printable char that breaks tests > > Nice job! I don't know how you spotted that C2A0 no-break space, but that > was masterful [U+1F44D] Thanks David :) It took a bit to figure that one out. As there was no obvious issue with the template logic, I looked at the patches here in bugzilla again and it showed in the browser. Guess more lucky than masterful ;) I have the following in my .vimrc
set list listchars=tab:>-,trail:.,extends:>,nbsp:·
And (9e675d2ac43 is right before the last patch has been pushed):
% git show 9e675d2ac43:koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation_batch_checkouts.tt|vim -
Shows me the non-printable char:
103 [% IF Koha.Preference('SpecifyDueDate') ||·Koha.Preference('OnSiteCheckouts') %]
Enhancement not pushed to 23.05.x |