Bug 37096

Summary: Required fields in budget creation are ignored if left blank
Product: Koha Reporter: Esther Melander <esther.melander>
Component: AcquisitionsAssignee: Sam Lau <samalau>
Status: Failed QA --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: minor    
Priority: P5 - low CC: m.de.rooy, roman.dolny, samalau
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 37096: Display error message for budget creation/modification if start/end date is left blank
Bug 37096: Display error message for budget creation/modification if start/end date is left blank

Description Esther Melander 2024-06-14 15:40:25 UTC
To Replicate

1. In Acquisitions, Budgets, add a new budget.
2. Fill in the Start date, Description, Total Amount, and make active. Leave the end date BLANK.
3. Save.

See that the user is returned to the budget listing page but the budget creation silently fails. The behavior is repeated if both the start date and end date are blank. A save failure error message does appear if the description is left blank.

I would expect a save fail error message if either the start or end date was blank.
Comment 1 Sam Lau 2024-07-30 16:51:11 UTC
Created attachment 169869 [details] [review]
Bug 37096: Display error message for budget creation/modification if start/end date is left blank

This patch adds an error message for a failed budget creation or modifcation due to the dates being empty.

To test:
1) Go to the acquisitions tab, then on the side under 'Administration' select the 'Budgets' tab.
2) Click 'New budget', enter only a description and press save.
3) Note that the budget was not created, but no messages were displayed.
4) Apply patch, restart_all
5) Once again select 'New budget' and enter only a description.
6) This time when submitting, you will see an error message that says the process failed.
7) Edit an exisitng budget and remove the start date from this budget.
8) Attempt to save and note that the error message displays properly.
Comment 2 Roman Dolny 2024-07-30 18:24:19 UTC
Created attachment 169873 [details] [review]
Bug 37096: Display error message for budget creation/modification if start/end date is left blank

This patch adds an error message for a failed budget creation or modifcation due to the dates being empty.

To test:
1) Go to the acquisitions tab, then on the side under 'Administration' select the 'Budgets' tab.
2) Click 'New budget', enter only a description and press save.
3) Note that the budget was not created, but no messages were displayed.
4) Apply patch, restart_all
5) Once again select 'New budget' and enter only a description.
6) This time when submitting, you will see an error message that says the process failed.
7) Edit an exisitng budget and remove the start date from this budget.
8) Attempt to save and note that the error message displays properly.

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 3 Roman Dolny 2024-07-30 18:24:53 UTC
This works as described, but shouldn't we prevent the form from being saved until all required fields are filled in?
Comment 4 Sam Lau 2024-07-30 18:43:17 UTC
(In reply to Roman Dolny from comment #3)
> This works as described, but shouldn't we prevent the form from being saved
> until all required fields are filled in?

Validating flatpickr values isn't the easiest thing to do. I couldn't find a way to make both of them required before submission while still utilizing jquery validation, so this solution was the next best thing for now
Comment 5 Marcel de Rooy 2024-11-22 09:04:39 UTC
(In reply to Roman Dolny from comment #3)
> This works as described, but shouldn't we prevent the form from being saved
> until all required fields are filled in?

Yeah, sounds good.
Comment 6 Marcel de Rooy 2024-11-22 09:05:19 UTC
Some minor QA comments:

my $action                  = $budget_period_id ne '' ? 'modify' : 'add';
This probably may trigger an uninitialized warning.
Shouldnt we fix by adding a fallback here:
my $budget_period_id     = $input->param('budget_period_id');

        if ( $budget_period_startdate eq '' || $budget_period_enddate eq '' ) {
Same here.
Add fallback to:
        my $budget_period_startdate = $input->param('budget_period_startdate');
        my $budget_period_enddate   = $input->param('budget_period_enddate');

EXIT_IF: {
[etc]
            $op = 'else';
            last EXIT_IF;
Using last (goto) is not the best programming style. And it seems easy to adjust the next if / else to prevent needing it.

+    [% IF failed_add_validate %]
I wonder if adding something with no start- or enddate here would be more clear?