|
Description
Matt Blenkinsop
2023-11-15 12:20:20 UTC
Created attachment 158980 [details] [review] Bug 35341: Fix ciruclation rule dates being overwritten This patch correctly sets the values of the date fields so that they are not overwritten when only one is amended. It also amends the collumn indexes being used in the click event as new columns have been added without these being updated Test plan: 1) Navigate to Administration > Circulation and fine rules 2) Add a new circulation rule for a category of your choice 3) For the "All" rule, add the date 31/12/2023 to both the Hard due date and No automatic renewal after (hard limit) (NARAHL) columns and save 4) Now edit the rule that you created. 5) Change the two columns above to 01/12/2023 and click save 6) Edit the rule again and change the Hard due date to 15/12/2023. Click save 7) Note that the date for your rule in the NARAHL has changed to 31/12/2023 despite not being edited 8) Click edit again, and change the NARAHL date to 15/12/2023 and save. 9) This time the date in Hard due date will have changed to 31/12/2023 10) Apply patch 11) Edit the dates individually a few times and this time your rule should update correctly Hi Matt, I noticed we have quite a lot of "bugs" related to circulation rules right now. I didn't have time to dig deeper yet, but could this one relate to one of these as well? Bug 34151 - Hard due date being ignored in circulation rules in favor of loan period Bug 32262 - smart-rules: Some rules can be created but are not visible on interface Bug 34826 - Circulation rules edit row select elements don't always update when selecting new row for editing ? No it doesn't look like it - this is a separate issue spotted by one of our customers Bug 34826 is the most similar but I think that one relates more to bug 34269 Created attachment 159005 [details] [review] Bug 35341: Fix ciruclation rule dates being overwritten This patch correctly sets the values of the date fields so that they are not overwritten when only one is amended. It also amends the collumn indexes being used in the click event as new columns have been added without these being updated Test plan: 1) Navigate to Administration > Circulation and fine rules 2) Add a new circulation rule for a category of your choice 3) For the "All" rule, add the date 31/12/2023 to both the Hard due date and No automatic renewal after (hard limit) (NARAHL) columns and save 4) Now edit the rule that you created. 5) Change the two columns above to 01/12/2023 and click save 6) Edit the rule again and change the Hard due date to 15/12/2023. Click save 7) Note that the date for your rule in the NARAHL has changed to 31/12/2023 despite not being edited 8) Click edit again, and change the NARAHL date to 15/12/2023 and save. 9) This time the date in Hard due date will have changed to 31/12/2023 10) Apply patch 11) Edit the dates individually a few times and this time your rule should update correctly Signed-off-by: David Nind <david@davidnind.com> (In reply to Matt Blenkinsop from comment #3) > No it doesn't look like it - this is a separate issue spotted by one of our > customers > > Bug 34826 is the most similar but I think that one relates more to bug 34269 Thanks for checking! Looking here This really is a rabbit hole ;) This = smart rules.. I think that we are not going in the right direction here with the hidden backup inputs. We are trying to solve a flatpickr issue here. We should do that differently.
Seeing several related bugs in the smart rules. Just listing a few:
[1] This is incorrect. The i is the column starting from 0.
} else if ( i == 5 || i == 6 || i == 27 || i == 28 || i == 29 || current_input_id === "holds_pickup_period" ) {
// If the value is not an integer for
// - "Current checkouts allowed"
// - "Current on-site checkouts allowed"
// - "Holds allowed (total)"
// - "Holds allowed (daily)"
// - "Holds per record (count)"
// - "Holds pickup period (day)"
// The value is "Unlimited" (or an equivalent translated string)
// an it should be set to an empty string
Current checkouts allowed should be 4?
Because 6 is listed, it is now cleared. (Loan period should allow a zero?)
[2] There is a check on overwriting the edit rule. But shouldnt there be a check too about warning to overwrite a current rule when saving a blank rule ?
Out of scope.
[3] Flatpickr fields should be cleared in edit rule when saving/submitting.
[4] If you do not click in the flatpickr field, the date is not saved! (This is in my test patch.) The input is still empty. We should trigger flatpickr to fill it before.
Will submit a test patch which is not finished but gives an idea of the solution that I am thinking of.
Created attachment 159077 [details] [review] Bug 35341: Testing Created attachment 159591 [details] [review] Bug 35341: Fix ciruclation rule dates being overwritten This patch correctly sets the values of the date fields so that they are not overwritten when only one is amended. It also amends the collumn indexes being used in the click event as new columns have been added without these being updated Test plan: 1) Navigate to Administration > Circulation and fine rules 2) Add a new circulation rule for a category of your choice 3) For the "All" rule, add the date 31/12/2023 to both the Hard due date and No automatic renewal after (hard limit) (NARAHL) columns and save 4) Now edit the rule that you created. 5) Change the two columns above to 01/12/2023 and click save 6) Edit the rule again and change the Hard due date to 15/12/2023. Click save 7) Note that the date for your rule in the NARAHL has changed to 31/12/2023 despite not being edited 8) Click edit again, and change the NARAHL date to 15/12/2023 and save. 9) This time the date in Hard due date will have changed to 31/12/2023 10) Apply patch 11) Edit the dates individually a few times and this time your rule should update correctly Created attachment 159592 [details] [review] Bug 35341: Fix circulation rule dates being overwritten This patch correctly sets the values of the date fields so that they are not overwritten when only one is amended. It also amends the collumn indexes being used in the click event as new columns have been added without these being updated Test plan: 1) Navigate to Administration > Circulation and fine rules 2) Add a new circulation rule for a category of your choice 3) For the "All" rule, add the date 31/12/2023 to both the Hard due date and No automatic renewal after (hard limit) (NARAHL) columns and save 4) Now edit the rule that you created. 5) Change the two columns above to 01/12/2023 and click save 6) Edit the rule again and change the Hard due date to 15/12/2023. Click save 7) Note that the date for your rule in the NARAHL has changed to 31/12/2023 despite not being edited 8) Click edit again, and change the NARAHL date to 15/12/2023 and save. 9) This time the date in Hard due date will have changed to 31/12/2023 10) Apply patch 11) Edit the dates individually a few times and this time your rule should update correctly I've taken your test patch Marcel and made some changes so that it now only uses flatpickr rather than adding additional hidden inputs to hold the value. This feels like a major bug so I think we should get a fix in place for this ASAP. Let me know what you think and if you agree with the concept I will address some of the other issues you spotted in follow-up bugs (e.g. incorrect column indexes etc). (In reply to Matt Blenkinsop from comment #12) > I've taken your test patch Marcel and made some changes so that it now only > uses flatpickr rather than adding additional hidden inputs to hold the value. > > This feels like a major bug so I think we should get a fix in place for this > ASAP. Let me know what you think and if you agree with the concept I will > address some of the other issues you spotted in follow-up bugs (e.g. > incorrect column indexes etc). Okay. Will have a look later this week. QA: Looking here Still some issues here: Template process failed: undef error - The given date (0NaN-aN-aN) does not match the date format (iso) at /usr/share/koha/Koha/DateUtils.pm line 205. => Try another date format like dd-mm-yy => var formatted_date = flatpickr.formatDate(new Date(itm_text), "Y-m-d") does not look good. Still looking further Created attachment 159669 [details] [review] Bug 35341: Improve processing hard due date and auto renewal date After quite a struggle, came up with this approach using flatpickr's parseDate and setDate. Seems to be the best. The dateformat variable is set in calendar.inc. Test plan: Test edit, save, clear rules, focus on both dates. Toggle all I18N preference date formats. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Matt Blenkinsop from comment #12) > I've taken your test patch Marcel and made some changes so that it now only > uses flatpickr rather than adding additional hidden inputs to hold the value. > > This feels like a major bug so I think we should get a fix in place for this > ASAP. Let me know what you think and if you agree with the concept I will > address some of the other issues you spotted in follow-up bugs (e.g. > incorrect column indexes etc). Sorry, but it did not work yet. I obsoleted your patch now in favor of adding flatpickr methods on top of the first patch. Could you add your signoff, Matts? And perhaps let Martin do the final QA? Note that this patch makes sure that the dates are not kept when saving or clearing the rule in the edit row. Matt is on holiday at the minute, but I'm having a quick look. I'm afraid I've found some issues, though I'm not sure if they predate the patches or not. The core issue is fixed.. woop woop! However; 1. Each time one clicks 'Edit' to edit a line, the flatpickers each get an extra 'clear date' red X added. 2. It appears that there's a link between the two flatpickers where they each highlight the corresponding date picked in the other rule.. that link doesn't update when either date is updated during edit. OK, maybe I'm confused.. looks like it shows the currently selected date prior to change rather than the other pickers date. My mistake. First issue about repeated X is still valid however. Looking here again Hmm. This comes from flatpickr instantiation etc. Not my favorite waste of time:
Calendar.inc contains:
onReady: function( selectedDates, dateStr, instance ){
/* When flatpickr instance is created, automatically append a "clear date" link */
This onReady is called too often.
Flatpickr says that onReady gets triggered once the calendar is in a ready state.
What happens now, is that the edit of a circulation rule triggers this onReady again and keeps adding buttons.
Sounds easy to fix but the automagical duplication of flatpickr inputs somehow interferes.
In the current codebase the second input gets wrapped into the flatpickr_wrapper class.
But if I move the onReady code to document.ready level only, somehow this second input is no longer wrapped into that span? Although the control works, you will see it wrap after clearing a date.
Not sure how to continue here.
Jonathan: do you have a suggestion here?
I tried several things including a filter like:
- $(instance.input).find('~input.flatpickr:first')
+ $(instance.input)
+ .filter( function () { return $(this).siblings('span').length == 0 } )
+ .find('~input.flatpickr:first')
As a separate function:
+ function add_clear_button (input) {
+ /* Append a "clear date" link */
+ $(input).find('~input.flatpickr:first')
+ /* Add a wrapper element so that we can prevent the clear button from wrapping */
+ .wrap("<span class='flatpickr_wrapper'></span>")
+ .attr("autocomplete", "off")
+ .after( $("<a/>")
+ .attr("href","#")
+ .addClass("clear_date")
+ .on("click", function(e){
+ e.preventDefault();
+ $(input).flatpickr().clear();
+ })
+ .addClass("fa fa-fw fa-times")
+ .attr("aria-hidden", true)
+ .attr("aria-label", _("Clear date") )
+ ).keydown(function(e) {
+ var key = (event.keyCode ? event.keyCode : event.which);
+ if ( key == 40 ) {
+ $(input).flatpickr().set('allowInput',false);
+ }
+ });
+ }
Created attachment 160830 [details] [review] Bug 35341: Do not reinstantiate flatpickr We do not need to reinstantiate flatpickr, we need to retrieve the existing instance. Marcel, I think I have fixed the problem you were facing. However I am failing at hiding the error (not a JS Error object, but something appearing in the console) raised by is_valid_date (actually flatpickr.parseDate) when we have "None defined". I have tried to silent it (with an option) but errorHandler in flatpickr is not documented (and I don't find a way to pass it when using parseDate as a static method). The only possible solution I found is to temporary override console.warn, but this sounds terribly wrong. Ah great. I am looking further now. Thanks. diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
index e3edc701bc..f3c57ad25e 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
@@ -1505,11 +1505,13 @@
} else if ( i == 9 ) {
// specific processing for Hard due date
$(current_column).find("select").val(itm_code);
- if (is_valid_date(itm_text)) {
+ if (itm_text) {
input_values = itm_text.split(' ');
var fp = document.querySelector("#hardduedate")._flatpickr;
- var hardduedate = fp.parseDate( input_values[1], flatpickr_dateformat_string );
- if( hardduedate) fp.setDate( hardduedate, 1 );
+ if (is_valid_date(input_values[1])) {
+ var hardduedate = fp.parseDate( input_values[1], flatpickr_dateformat_string );
+ if( hardduedate) fp.setDate( hardduedate, 1 );
+ }
}
} else if ( i == 26 ) {
// specific processing for No automatic renewal after (hard limit)
Adding the above change since item_text does contain more than a date in the first case. It also includes the Before/On/After text part for the first date. (In reply to Jonathan Druart from comment #25) > Marcel, I think I have fixed the problem you were facing. However I am > failing at hiding the error (not a JS Error object, but something appearing > in the console) raised by is_valid_date (actually flatpickr.parseDate) when > we have "None defined". > > I have tried to silent it (with an option) but errorHandler in flatpickr is > not documented (and I don't find a way to pass it when using parseDate as a > static method). > > The only possible solution I found is to temporary override console.warn, > but this sounds terribly wrong. I was not able to recreate a JS error in the console. Created attachment 160844 [details] [review] Bug 35341: Improve processing hard due date and auto renewal date After quite a struggle, came up with this approach using flatpickr's parseDate and setDate. Seems to be the best. The dateformat variable is set in calendar.inc. Test plan: Test edit, save, clear rules, focus on both dates. Toggle all I18N preference date formats. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 160845 [details] [review] Bug 35341: Do not reinstantiate flatpickr We do not need to reinstantiate flatpickr, we need to retrieve the existing instance. Test plan: See previous patch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Martin or Matt, could one of you have a final look? Three members of QA team already did until now :) Wondering if we really need this btw, but it should not harm either:
diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/calendar.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/calendar.inc
index f027552053..076820873b 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/includes/calendar.inc
+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/calendar.inc
@@ -200,6 +200,7 @@
fp.set("maxDate", now.setMinutes(now.getMinutes() + 1));
}, 30000);
}
+ return fp;
}
$(document).ready(function(){
(In reply to Marcel de Rooy from comment #33) > Wondering if we really need this btw, but it should not harm either: > > diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/calendar.inc > b/koha-tmpl/intranet-tmpl/prog/en/includes/calendar.inc > index f027552053..076820873b 100644 > --- a/koha-tmpl/intranet-tmpl/prog/en/includes/calendar.inc > +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/calendar.inc > @@ -200,6 +200,7 @@ > fp.set("maxDate", now.setMinutes(now.getMinutes() + 1)); > }, 30000); > } > + return fp; > } > > $(document).ready(function(){ No we don't, I was trying something else. (In reply to Marcel de Rooy from comment #29) > (In reply to Jonathan Druart from comment #25) > > Marcel, I think I have fixed the problem you were facing. However I am > > failing at hiding the error (not a JS Error object, but something appearing > > in the console) raised by is_valid_date (actually flatpickr.parseDate) when > > we have "None defined". > > > > I have tried to silent it (with an option) but errorHandler in flatpickr is > > not documented (and I don't find a way to pass it when using parseDate as a > > static method). > > > > The only possible solution I found is to temporary override console.warn, > > but this sounds terribly wrong. > > I was not able to recreate a JS error in the console. With the sample data, click "Edit" => "Error: Invalid date provided: defined" (In reply to Jonathan Druart from comment #35) > (In reply to Marcel de Rooy from comment #29) > > (In reply to Jonathan Druart from comment #25) > > > Marcel, I think I have fixed the problem you were facing. However I am > > > failing at hiding the error (not a JS Error object, but something appearing > > > in the console) raised by is_valid_date (actually flatpickr.parseDate) when > > > we have "None defined". > > > > > > I have tried to silent it (with an option) but errorHandler in flatpickr is > > > not documented (and I don't find a way to pass it when using parseDate as a > > > static method). > > > > > > The only possible solution I found is to temporary override console.warn, > > > but this sounds terribly wrong. > > > > I was not able to recreate a JS error in the console. > > With the sample data, click "Edit" > => "Error: Invalid date provided: defined" Okay, will refresh db and try. Created attachment 160850 [details] [review] Bug 35341: Do not reinstantiate flatpickr We do not need to reinstantiate flatpickr, we need to retrieve the existing instance. Test plan: See previous patch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Jonathan Druart from comment #34) > (In reply to Marcel de Rooy from comment #33) > > Wondering if we really need this btw, but it should not harm either: > > No we don't, I was trying something else. Removed the line. (In reply to Jonathan Druart from comment #35) > (In reply to Marcel de Rooy from comment #29) > > I was not able to recreate a JS error in the console. > > With the sample data, click "Edit" > => "Error: Invalid date provided: defined" With fresh db still not able to reproduce. Did you try the current patches and did you refresh browser cache? Will rebuild my container completely. Back to Assigned. Needs more attention still :) Created attachment 160896 [details] [review] Bug 35341: Improve processing hard due date and auto renewal date After quite a struggle, came up with this approach using flatpickr's parseDate and setDate. Seems to be the best. The dateformat variable is set in calendar.inc. Test plan: Test edit, save, clear rules, focus on both dates. Toggle all I18N preference date formats. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Bug 35341: Do not reinstantiate flatpickr We do not need to reinstantiate flatpickr, we need to retrieve the existing instance. Test plan: See previous patch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> 12-01-2024 Squashed both patches. Removed the unused hidden inputs for hardduedatecompare and added a data attribute for the duedate. The check with is_valid_date seems no longer needed here. Normally date is empty or valid. A js warning from parseDate should be exceptional. Made code for both dates a bit more consistent. Back to Signed Off. Created attachment 160928 [details] [review] Bug 35341: Improve processing hard due date and auto renewal date After quite a struggle, came up with this approach using flatpickr's parseDate and setDate. Seems to be the best. The dateformat variable is set in calendar.inc. Test plan: Test edit, save, clear rules, focus on both dates. Toggle all I18N preference date formats. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Bug 35341: Do not reinstantiate flatpickr We do not need to reinstantiate flatpickr, we need to retrieve the existing instance. Test plan: See previous patch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> 12-01-2024 Squashed both patches. Removed the unused hidden inputs for hardduedatecompare and added a data attribute for the duedate. The check with is_valid_date seems no longer needed here. Normally date is empty or valid. A js warning from parseDate should be exceptional. Made code for both dates a bit more consistent. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Thanks for all your work here Marcel, It's all looking much better for me now.. so far I've not managed to break anything :) Passing QA Pushed for 24.05! Well done everyone, thank you! Pushed to 23.11.x for 23.11.02 Backported to 23.05.x for upcoming 23.05.09 |