Bug 16457 - Remove the use of "onclick" from the patron entry form
Summary: Remove the use of "onclick" from the patron entry form
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Owen Leonard
QA Contact: Katrin Fischer
URL: /cgi-bin/koha/members/memberentry.pl
Keywords:
Depends on: 13041 16386 16450
Blocks: 17858
  Show dependency treegraph
 
Reported: 2016-05-05 18:53 UTC by Owen Leonard
Modified: 2020-04-03 22:35 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 16457: Remove the use of "onclick" from the patron entry form (13.26 KB, patch)
2020-03-18 17:19 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16457: Remove the use of "onclick" from the patron entry form (13.57 KB, patch)
2020-03-22 20:04 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 16457: (follow-up) Enable click event on dynamically-created elements (1.84 KB, patch)
2020-03-24 12:20 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16457: Remove the use of "onclick" from the patron entry form (13.63 KB, patch)
2020-03-25 20:42 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 16457: (follow-up) Enable click event on dynamically-created elements (1.89 KB, patch)
2020-03-25 20:42 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-05-05 18:53:18 UTC
There are several instances of "onclick" in memberentrygen.tt. It should be rewritten so that JS functionality can be separate from the markup.
Comment 1 Owen Leonard 2020-03-18 17:19:05 UTC
Created attachment 100971 [details] [review]
Bug 16457: Remove the use of "onclick" from the patron entry form

This patch removes several event attributes from the patron entry form
and replaces them with event handlers in the <script> block.

Some event attributes were removed altogether because they weren't
functional: Onchange events on jQueryUI datepickers don't work, and they
would have been redundant even if they did work.

To test you should have some patron attributes configured
(Administration -> Patron attribute types). At least one attribute
should be configured to apply to one particular patron category.

Apply the patch and begin the process of adding a new patron.

1. Try to add a patron with details matching a patron which already
   exists. You should get a confirmation message on screen which asks if
   the patron is a duplicte. Test the link which shows the existing
   record. It should trigger a popup window with details about the
   patron.

2. Test the process of setting a patron's date of birth. Selecting a
   date should trigger the display of the patron's age immedately below
   the field.

3. Test that you cannot set a patron's expiration to a date which
   precedes the date in the registration date field.

4. Test that you can add a manual restriction's expiration date, and
   that the "Clear date" link works correctly.

5. Test that the "clear" and "new" controls links work with additional
   attributes and identifiers.

6. Test that changing the patron's category correctly affects the
   display of available attributes and identifiers: The attribute you
   have associated with a particular patron category should only show if
   that category is selected under "Category."
Comment 2 Bernardo Gonzalez Kriegel 2020-03-22 20:04:27 UTC
Created attachment 101391 [details] [review]
Bug 16457: Remove the use of "onclick" from the patron entry form

This patch removes several event attributes from the patron entry form
and replaces them with event handlers in the <script> block.

Some event attributes were removed altogether because they weren't
functional: Onchange events on jQueryUI datepickers don't work, and they
would have been redundant even if they did work.

To test you should have some patron attributes configured
(Administration -> Patron attribute types). At least one attribute
should be configured to apply to one particular patron category.

Apply the patch and begin the process of adding a new patron.

1. Try to add a patron with details matching a patron which already
   exists. You should get a confirmation message on screen which asks if
   the patron is a duplicte. Test the link which shows the existing
   record. It should trigger a popup window with details about the
   patron.

2. Test the process of setting a patron's date of birth. Selecting a
   date should trigger the display of the patron's age immedately below
   the field.

3. Test that you cannot set a patron's expiration to a date which
   precedes the date in the registration date field.

4. Test that you can add a manual restriction's expiration date, and
   that the "Clear date" link works correctly.

5. Test that the "clear" and "new" controls links work with additional
   attributes and identifiers.

6. Test that changing the patron's category correctly affects the
   display of available attributes and identifiers: The attribute you
   have associated with a particular patron category should only show if
   that category is selected under "Category."

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Easy to follow test plan, all works, but on point 5. I found one 'clear'
but no 'new' control link, it's my only comment.
No errors.
Comment 3 Katrin Fischer 2020-03-24 08:00:58 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #2)
> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
> Easy to follow test plan, all works, but on point 5. I found one 'clear'
> but no 'new' control link, it's my only comment.
> No errors.

The 'new' button will be visible for repeatable attributes. Thanks to the note I tested this and found that when you click on 'new' for a second time to add a third field you are thrown to the top of the screen.

It depends on which 'new' link you use.

Owen, can you please check?
Comment 4 Owen Leonard 2020-03-24 12:20:38 UTC
Created attachment 101554 [details] [review]
Bug 16457: (follow-up) Enable click event on dynamically-created elements

This patch moves the click handler for patron attribute actions ("new",
"clear") from the links themselves to the container. This allows jQuery
to correctly target dynamically-created copies of the links.

To test you should have extended patron attributes enabled and you
should have some repeatable attributes defined.

 - Create or edit a patron record
 - In the "Additional attributes and identifiers" section, test the
   "Clear" and "New" links. Confirm that the controls work with both the
   original instance of the field and those created by clicking "New."
Comment 5 Katrin Fischer 2020-03-25 20:42:48 UTC
Created attachment 101770 [details] [review]
Bug 16457: Remove the use of "onclick" from the patron entry form

This patch removes several event attributes from the patron entry form
and replaces them with event handlers in the <script> block.

Some event attributes were removed altogether because they weren't
functional: Onchange events on jQueryUI datepickers don't work, and they
would have been redundant even if they did work.

To test you should have some patron attributes configured
(Administration -> Patron attribute types). At least one attribute
should be configured to apply to one particular patron category.

Apply the patch and begin the process of adding a new patron.

1. Try to add a patron with details matching a patron which already
   exists. You should get a confirmation message on screen which asks if
   the patron is a duplicte. Test the link which shows the existing
   record. It should trigger a popup window with details about the
   patron.

2. Test the process of setting a patron's date of birth. Selecting a
   date should trigger the display of the patron's age immedately below
   the field.

3. Test that you cannot set a patron's expiration to a date which
   precedes the date in the registration date field.

4. Test that you can add a manual restriction's expiration date, and
   that the "Clear date" link works correctly.

5. Test that the "clear" and "new" controls links work with additional
   attributes and identifiers.

6. Test that changing the patron's category correctly affects the
   display of available attributes and identifiers: The attribute you
   have associated with a particular patron category should only show if
   that category is selected under "Category."

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Easy to follow test plan, all works, but on point 5. I found one 'clear'
but no 'new' control link, it's my only comment.
No errors.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 6 Katrin Fischer 2020-03-25 20:42:53 UTC
Created attachment 101771 [details] [review]
Bug 16457: (follow-up) Enable click event on dynamically-created elements

This patch moves the click handler for patron attribute actions ("new",
"clear") from the links themselves to the container. This allows jQuery
to correctly target dynamically-created copies of the links.

To test you should have extended patron attributes enabled and you
should have some repeatable attributes defined.

 - Create or edit a patron record
 - In the "Additional attributes and identifiers" section, test the
   "Clear" and "New" links. Confirm that the controls work with both the
   original instance of the field and those created by clicking "New."

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 7 Martin Renvoize 2020-03-26 12:02:54 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 8 Martin Renvoize 2020-03-26 12:04:00 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 9 Joy Nelson 2020-04-03 22:35:23 UTC
enhancement not backported to 19.11.x