Bug 9507 - Some code used to prevent form from being unexpectedly submitted is duplicated
Summary: Some code used to prevent form from being unexpectedly submitted is duplicated
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 9268
Blocks: 10310
  Show dependency treegraph
 
Reported: 2013-01-30 14:19 UTC by Jonathan Druart
Modified: 2019-03-06 16:24 UTC (History)
6 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:


Attachments
Bug 9507: prevent submit: factorize some code in a js file (5.40 KB, patch)
2013-01-30 14:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9507: prevent submit: factorize some code in a js file (5.45 KB, patch)
2013-02-01 16:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9507: prevent submit: refactor some code in a js file (5.45 KB, patch)
2013-05-22 10:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9507: prevent submit: refactor some code in a js file (5.90 KB, patch)
2013-05-22 12:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9507: prevent submit: refactor some code in a js file (5.90 KB, patch)
2013-05-22 12:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[3.8.x] Bug 9507: prevent submit: refactor some code in a js file (4.61 KB, patch)
2013-06-07 08:55 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-01-30 14:19:25 UTC

    
Comment 1 Jonathan Druart 2013-01-30 14:22:38 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-02-01 16:48:57 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2013-05-21 16:16:51 UTC
Just a question:

+function IEEventHandler_KeyDown() {
+    if (event.keyCode == 13 && event.srcElement.type != 'textarea' && event.srcElement.type != 'submit')
+        return false;
+    return true;
+}

But am I missing a parameter here?
Comment 4 Jonathan Druart 2013-05-22 07:44:40 UTC
(In reply to comment #3)
> Just a question:
> 
> +function IEEventHandler_KeyDown() {
> +    if (event.keyCode == 13 && event.srcElement.type != 'textarea' &&
> event.srcElement.type != 'submit')
> +        return false;
> +    return true;
> +}
> 
> But am I missing a parameter here?

Humm, I just factorize the code, so it is not a regression. I don't have IE for testing it.
In fact event is a global object, it is not necessary to pass it in parameters.
This code should work I think.
Comment 5 Marcel de Rooy 2013-05-22 10:17:36 UTC
(In reply to comment #4)
> I don't have IE for testing it.
:) I would not recommend IE too for the staff client. But if you change code related to IE, it still would be an idea to test it somehow? 

> In fact event is a global object, it is not necessary to pass it in
> parameters.
Yes, you are right (of course). As a purist, you could perhaps say that it should be even better to refer to window.event. 
With Google, I came across this example for a cross-browser solution:

element.onclick = doSomething;
function doSomething(e) {
	if (!e) var e = window.event;
	// e gives access to the event in all browsers
}
Comment 6 Jonathan Druart 2013-05-22 10:29:11 UTC
(In reply to comment #5)
> With Google, I came across this example for a cross-browser solution:
> 
> element.onclick = doSomething;
> function doSomething(e) {
> 	if (!e) var e = window.event;
> 	// e gives access to the event in all browsers
> }

Yep I think the code can be improved, but the goal of this patch is only to factorize the code.
When this patch will be pushed, it will be easy to improve it: there will be only 1 place to change :)
Don't you agree with me?
Comment 7 Marcel de Rooy 2013-05-22 10:44:02 UTC
(In reply to comment #6)
> Yep I think the code can be improved, but the goal of this patch is only to
> factorize the code.
> Don't you agree with me?
Yes, I agree. Will try to qa this today.
BTW As a non-native English speaker, I think you mean to refactor the code. I thought factorizing was 12=3*2*2 :-)
Comment 8 Jonathan Druart 2013-05-22 10:53:27 UTC
(In reply to comment #7)
> BTW As a non-native English speaker, I think you mean to refactor the code.
> I thought factorizing was 12=3*2*2 :-)

Ooops :-/ Thanks!
Comment 9 Jonathan Druart 2013-05-22 10:53:38 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2013-05-22 12:02:41 UTC Comment hidden (obsolete)
Comment 11 Marcel de Rooy 2013-05-22 12:04:37 UTC
Created attachment 18310 [details] [review]
Bug 9507: prevent submit: refactor some code in a js file

This patch refactors some code in a js file.

Test plan:
On acqui/neworderempty.pl, acqui/orderreceive.pl and
serials/serials-edit.tt try to scan a barcode (or press enter) on the
form and check that it is not sent.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Have to note that the code for IE9 does not work. Before and after this patch.
If we refactor code, it would have been nice to resolve this too.
But I do not oppose pushing this patch.
The test uses 'an ancient Netscape property' window.Event (uppercase!) to make
the distinction between browers and event models. Some more documentation here
would be welcome too.
Comment 12 Marcel de Rooy 2013-05-22 12:07:04 UTC
QA Comment:
See signoff.
Tested under FF. Works well.
Tested under IE9. Does not work before or after.
No complaints from qa-tools.
Passing QA but with some reluctancy as noted.

With appreciation for the intention to improve code BTW!
Comment 13 Galen Charlton 2013-05-22 14:54:38 UTC
Pushed to master.  Thanks, Jonathan!

I too encourage a quick follow-up to improve the Javascript, now that it lives only in one place.
Comment 14 Tomás Cohen Arazi 2013-06-03 14:17:47 UTC
This patch has been pushed to 3.12.x, will be in 3.12.1.

I was reluctant to cherry picking refactoring patches, but it is needed by 10310 which is a good bugfix.
Comment 15 Bernardo Gonzalez Kriegel 2013-06-06 01:14:14 UTC
Pushed to 3.10.x, will be in 3.10.7
Comment 16 Chris Hall 2013-06-06 09:55:15 UTC
I tried applying this patch to 3.8.x and it came across mostly clean, the only concern I had was

 <script type="text/javascript" src="[% themelang %]/js/cataloging.js"></script>
+<script type="text/javascript" src="[% themelang %]/js/prevent_submit.js"></script>

3.8.x doesn't have cataloging.js, will prevent_submit.js work fine without it?
I didn't want to make a guess.

This patch is required for 10310 so I would quite like it for 3.8.x
Thank you for your attention.
Comment 17 Jonathan Druart 2013-06-07 08:55:59 UTC
Created attachment 18726 [details] [review]
[3.8.x] Bug 9507: prevent submit: refactor some code in a js file

This patch refactors some code in a js file.

Test plan:
On acqui/neworderempty.pl, acqui/orderreceive.pl and
serials/serials-edit.tt try to scan a barcode (or press enter) on the
form and check that it is not sent.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Have to note that the code for IE9 does not work. Before and after this patch.
If we refactor code, it would have been nice to resolve this too.
But I do not oppose pushing this patch.
The test uses 'an ancient Netscape property' window.Event (uppercase!) to make
the distinction between browers and event models. Some more documentation here
would be welcome too.
Comment 18 Jonathan Druart 2013-06-07 08:57:23 UTC
(In reply to comment #16)
Hi Chris, this last patch applies on 3.8.x branch.
js/cataloguing.js is not needed for this patch.
Comment 19 Chris Hall 2013-06-12 08:09:01 UTC
(In reply to comment #18)
Thank you very much for that Jonathan :)

Followed the test plan and it worked perfectly, you just have to make sure to select the form before hitting enter as the search box has focus (this is not a critique of your patch but a note to others testing it).

Pushed to 3.8.x, will be in 3.8.14.