Bug 16754 - Use validation plugin in budgets, planning, and contracts
Summary: Use validation plugin in budgets, planning, and contracts
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Owen Leonard
QA Contact: Testopia
URL: /cgi-bin/koha/admin/aqbudgetperiods.pl
Keywords:
Depends on:
Blocks: 26496
  Show dependency treegraph
 
Reported: 2016-06-16 13:22 UTC by Owen Leonard
Modified: 2020-09-21 01:53 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 16754 - Use validation plugin when adding or editing budgets (9.03 KB, patch)
2016-06-16 13:40 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16754 - Use validation plugin in budgets, planning, and contracts (22.14 KB, patch)
2016-06-17 11:57 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16754 - Use validation plugin in budgets, planning, and contracts (22.24 KB, patch)
2016-08-05 17:07 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 16754: Use validation plugin in budgets, planning, and contracts (22.89 KB, patch)
2018-10-03 14:53 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16754: Use validation plugin in budgets, planning, and contracts (22.94 KB, patch)
2018-10-04 08:52 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 16754: Use validation plugin in budgets, planning, and contracts (23.00 KB, patch)
2018-10-06 20:45 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2016-06-16 13:22:09 UTC
There is a lot of custom form validation JavaScript in the budget editing template which can be handled instead by the jQuery validation plugin.
Comment 1 Owen Leonard 2016-06-16 13:40:22 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2016-06-16 13:47:32 UTC
I see that the date comparison operation is handled better in other templates. I'm going to revise this patch to try to incorporate a working version.
Comment 3 Owen Leonard 2016-06-17 11:57:04 UTC Comment hidden (obsolete)
Comment 4 Frédéric Demians 2016-08-01 17:29:03 UTC
There is something wrong (strange) in budget creation:

- Create a new budget

- Click on Save

- A warning is displayed after Start date input box: This field is required.
  And the date picker is displayed

- Pick a date. => The date is copied to the input box, but (1) it is red, and
  (2) the message 'This field is required' is still displayed.
Comment 5 Owen Leonard 2016-08-05 13:47:04 UTC
(In reply to Frédéric Demians from comment #4)

> - Pick a date. => The date is copied to the input box, but (1) it is red, and
>   (2) the message 'This field is required' is still displayed.

This is a problem not with this patch but with the way the validation plugin works together with the datepicker widget. Now that I know of the problem I can find it elsewhere too, for instance aqcontract.pl.

I can try to find a solution, but I would like to do so as a separate bug if that's okay with everyone.
Comment 6 Frédéric Demians 2016-08-05 17:07:18 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2016-08-12 11:05:49 UTC
I'm not sure how this is valid but, if you enter start date == end date, the validator tell you that it is not allowed.
It's a change with before this patch. We could imagine a budget for only 1 day, which was allowed before, but not anymore.
Feel free to contradict :)
Comment 8 Owen Leonard 2016-08-26 14:07:28 UTC
(In reply to Jonathan Druart from comment #7)

> We could imagine a budget for only 1
> day, which was allowed before, but not anymore.

That seems very unlikely. It wouldn't be difficult to add a new validation setting to allow the two dates to match, but I'm not sure it's worth it just for this form and without an indication that it's an important feature. I think this form should match others in Koha where the end date in a date span is required to be after the start.
Comment 9 Owen Leonard 2018-10-03 14:53:11 UTC
Created attachment 79896 [details] [review]
Bug 16754: Use validation plugin in budgets, planning, and contracts

This patch removes the custom javascript form validation from three
templates: Budgets, budget planning, and contracts. Validation via the
validation plugin is done instead.

This patch also removes use of event attributes like onclick and
onchange. Also changed: I added the "Required" text next to required
fields as necessary.

To test, apply the patch and go to Administration -> Budgets.

- Test the budget creation process for adding, editing, and duplicating
  budgets. In all cases:
  - The start date and end date should be required
  - You should be warned if you enter an end date which falls before the
    start date.
  - The description should be required
  - The budget amount should be required to be a number

To test the budget planning page:

Note that much of the JS removed from this template was unused. It
looked for inputs by the name 'est_total' which don't exist.

- When viewing the table of funds for a budget, select a choice from the
  'planning' toolbar menu.
  - Verify that after entering a numeric value in any cell, the
    'remaning' value at the end of the row is correctly updated.
  - Verify that after entering non-numeric data an error is displayed.
  - Verify that clicking the 'auto-fill' button at the end of the row
    works to fill any empty or zero-value fields with the remaning funds
    available.

To test the vendor contracts page:

- Go to Acquisitions -> Vendor -> Contracts
- Add or edit a contract
  - Confirm that name, start date, and end date are required.
  - Confirm that you are warned if you enter an end date which falls
    before the start date.
Comment 10 Michal Denar 2018-10-04 08:52:02 UTC
Created attachment 79940 [details] [review]
Bug 16754: Use validation plugin in budgets, planning, and contracts

This patch removes the custom javascript form validation from three
templates: Budgets, budget planning, and contracts. Validation via the
validation plugin is done instead.

This patch also removes use of event attributes like onclick and
onchange. Also changed: I added the "Required" text next to required
fields as necessary.

To test, apply the patch and go to Administration -> Budgets.

- Test the budget creation process for adding, editing, and duplicating
  budgets. In all cases:
  - The start date and end date should be required
  - You should be warned if you enter an end date which falls before the
    start date.
  - The description should be required
  - The budget amount should be required to be a number

To test the budget planning page:

Note that much of the JS removed from this template was unused. It
looked for inputs by the name 'est_total' which don't exist.

- When viewing the table of funds for a budget, select a choice from the
  'planning' toolbar menu.
  - Verify that after entering a numeric value in any cell, the
    'remaning' value at the end of the row is correctly updated.
  - Verify that after entering non-numeric data an error is displayed.
  - Verify that clicking the 'auto-fill' button at the end of the row
    works to fill any empty or zero-value fields with the remaning funds
    available.

To test the vendor contracts page:

- Go to Acquisitions -> Vendor -> Contracts
- Add or edit a contract
  - Confirm that name, start date, and end date are required.
  - Confirm that you are warned if you enter an end date which falls
    before the start date.

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 11 Katrin Fischer 2018-10-06 20:45:25 UTC
Created attachment 80169 [details] [review]
Bug 16754: Use validation plugin in budgets, planning, and contracts

This patch removes the custom javascript form validation from three
templates: Budgets, budget planning, and contracts. Validation via the
validation plugin is done instead.

This patch also removes use of event attributes like onclick and
onchange. Also changed: I added the "Required" text next to required
fields as necessary.

To test, apply the patch and go to Administration -> Budgets.

- Test the budget creation process for adding, editing, and duplicating
  budgets. In all cases:
  - The start date and end date should be required
  - You should be warned if you enter an end date which falls before the
    start date.
  - The description should be required
  - The budget amount should be required to be a number

To test the budget planning page:

Note that much of the JS removed from this template was unused. It
looked for inputs by the name 'est_total' which don't exist.

- When viewing the table of funds for a budget, select a choice from the
  'planning' toolbar menu.
  - Verify that after entering a numeric value in any cell, the
    'remaning' value at the end of the row is correctly updated.
  - Verify that after entering non-numeric data an error is displayed.
  - Verify that clicking the 'auto-fill' button at the end of the row
    works to fill any empty or zero-value fields with the remaning funds
    available.

To test the vendor contracts page:

- Go to Acquisitions -> Vendor -> Contracts
- Add or edit a contract
  - Confirm that name, start date, and end date are required.
  - Confirm that you are warned if you enter an end date which falls
    before the start date.

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Nick Clemens (kidclamp) 2018-10-17 19:21:14 UTC
Awesome work all!

Pushed to master for 18.11
Comment 13 Martin Renvoize (ashimema) 2018-10-26 12:19:49 UTC
Nice bit of code tidying.

Pushed to 18.05.x for 18.05.06
Comment 14 Fridolin Somers 2018-11-09 14:38:02 UTC
I prefer not to push to oldstable 17.11.x to avoid behavior and display changes
Comment 15 David Cook 2020-09-21 00:33:21 UTC
It looks like this change actually broke aqplan.pl because it accidentally removed the Javascript that was used to add the "op" parameter, and the test plan didn't include testing saving budget plans. Oops!