Catches JS errors in calculations on the front end and alerts users. #66

LauraSteiner · 2017-09-01T01:35:16Z

@jwahlin @stephywells First draft of catching JS calculation errors on the front end and alert owners/users.

To be safe, I hide the submit/next button when there's an error. I also put a red line around the form. I'm happy to handle this another way, if you guys have a different preference.

This is phase 1. Phase 2 will involve checking calculations in the admin for unmatched parens (and other errors, if we think of them) and alerting users.

jwahlin · 2017-09-01T15:55:25Z

Hi Laura!

It looks like the approach for this is a bit dependent on what the team decides.

Once that is decided, I can better review this pull request. At the moment, I'm seeing a couple of minor code styling items that could be adjusted:

In our JavaScript, we use camel case. That is the norm for JavaScript according to the research I've done on it.
There is some inconsistent spacing in the PR. That could be cleaned up. We include spaces within function calls in PHP, but not JavaScript. Each language has its own code styling. A couple examples of places where I'm seeing inconsistent spacing:

function create_calculations_error_message(err, field_key, thisFullCalc ){ (should not have space after thisFullCalc)
if (action ==="frm_forms_preview"){ (should be space after ===)

…rens in admin.

LauraSteiner · 2017-09-08T02:53:08Z

@jwahlin This is ready for your review so we can start a discussion. I'd especially like your input on what to put in error messages.

stephywells

This is looking great! I have a couple suggestions here. Great work!

stephywells · 2018-01-11T21:16:50Z

js/formidable.js

+			}

-			if ( typeof total === 'undefined' || isNaN(total) ) {
+			if (typeof total === 'undefined' || isNaN(total)) {


Would you mind putting the spacing back in this line?

stephywells · 2018-01-11T21:17:04Z

js/formidable.js


 			// Set decimal points
-			if ( isNumeric( dec ) ) {
+			if (isNumeric(dec)) {


Can we get the spacing back in this line too?

stephywells · 2018-01-11T21:26:22Z

js/formidable_admin.js

+	 * @returns {RegExp}
+	 */
+	function getNonNumericShortcodes() {
+		return /\[(date|time|email|ip\])/;


Our age calculation actually uses the time shortcode. Since both date and time can use formats that make them numeric, it may be better to allow them. Moving the ] after the ) in this regex should do the trick.

stephywells · 2018-01-11T21:35:57Z

js/formidable.js

+		alertMessage += 'Oops! There\'s an error in the calculation in field ' + field_key + ':\n\n';
+		alertMessage += thisFullCalc + '\n\n';
+		if (err.message) {
+			alertMessage += 'The error message is: ' + err.message + '\n\n';


Maybe we can remove 'The error message is:' part? That will simplify the localization if there's only one string or two that we need to add.

stephywells · 2018-01-11T21:36:52Z

js/formidable.js

+		if (err.message) {
+			alertMessage += 'The error message is: ' + err.message + '\n\n';
+		}
+		alertMessage += 'No worries!  These are generally easy to fix.';


Maybe we can remove this line too?

stephywells · 2018-01-11T21:38:07Z

classes/controllers/FrmFormsController.php

-        }
-    }
+		}
+		if ( current_user_can( 'activate_plugins' ) ) {


Let's make sure they also have permission to edit forms with current_user_can( 'frm_edit_forms' )

stephywells · 2018-01-11T21:39:19Z

js/formidable.js

+			return;
+		}
+
+		alertMessage += 'Oops! There\'s an error in the calculation in field ' + field_key + ':\n\n';


We'll need to move any text strings to the localization. This is included in FrmAppHelper::localize_script()

stephywells · 2018-01-11T21:42:00Z

js/formidable_admin.js

+		if (stack.length > 0 || unmatchedClosing.length > 0) {
+			msg = '***This calculation has at least one unmatched ( ) { } [ ].***\n\n';
+			/*
+			//a more specific message option that has opening and closing grouped, with only the first unmatched


Does this commented section need to stick around?

stephywells · 2018-01-11T21:57:49Z

js/formidable_admin.js

+	 * @returns {RegExp}
+	 */
+	function getNonFormShortcodes() {
+		return /\[(key|id\]|created|updated)/;


This regex needs a little modification to make sure it only applies to the shortcodes we are checking for. Right now, it flags [keyoffield]. Should we use created-at, created-by, updated-at, updated-by?

…other fixes.

LauraSteiner · 2018-01-27T06:29:04Z

Thank you for your comments, which were very helpful, as always! I'm glad to know about storing message strings in wp_localize_script so they can be translated.

I've added more shortcodes to the regex checking for non-form shortcodes. As it's written, most don't require a closing bracket to match. I'd appreciate your review of this (and everything else, of course) to make sure the net I'm throwing isn't too wide.

stephywells

It looks like there is one more change needed on this. Thanks for your patience!

stephywells · 2018-03-29T18:00:21Z

js/formidable.js

+
+		var alertMessage = '';
+
+		if ( ( !jQuery( 'form' ).hasClass( 'frm-admin-viewing' ) ) ) {


This line looks like it would check for the class on the first form on the page. If the first form is a login form, this will always return false. Right?

So smart of you to think of this! Login forms didn't cross my mind. The jQuery docs say that hasClass will return true if any of the matched elements have the specified class. So this happens to work properly as it is, I believe.

stephywells · 2018-03-31T14:26:03Z

js/formidable_admin.js

+		var warningMessage = checkMatchingParens( calculation );
+		warningMessage += checkShortcodes( calculation, this );
+
+		if ( warningMessage != '' ) {


Do you have JS hint enabled in PHP storm? JS hint prefers !== instead of != whenever possible.

Great hint. (pun!) Thanks.

LauraSteiner · 2018-04-12T01:50:27Z

Thank you very much for doing this review! I need to figure out a better system for following GitHub updates. This somehow slipped past my radar, and I didn't see it until today.

The JS has been split since I wrote this. I'm sure there's a cool GitHub way to incorporate everything, but I just created a new branch and added the changes by hand. I'll submit for a pull request shortly.

LauraSteiner requested review from jwahlin and stephywells September 1, 2017 01:35

LauraSteiner force-pushed the issue/1524 branch from 93f8a22 to c6f2e7e Compare September 1, 2017 01:40

Catches JS errors in calculations on the front end and alerts users.

ab895ae

LauraSteiner force-pushed the issue/1524 branch from c6f2e7e to ab895ae Compare September 1, 2017 01:52

Alerts admins to calculations errors on front-end and to unmatched pa…

0c77d93

…rens in admin.

Add check of calculations for inappropriate shortcodes.

3482108

stephywells requested changes Jan 11, 2018

View reviewed changes

stephywells and others added 3 commits January 19, 2018 09:18

Merge remote-tracking branch 'origin/master' into issue/1524

f37caec

Moves error messages for user calculations to wp_localize_script and …

2509308

…other fixes.

Moves error messages for user calculations to wp_localize_script and …

48f307a

…other fixes.

LauraSteiner removed the request for review from jwahlin January 31, 2018 03:51

stephywells requested changes Mar 31, 2018

View reviewed changes

stephywells closed this Apr 12, 2018

stephywells deleted the issue/1524 branch February 11, 2019 14:45


		var alertMessage = '';

		if ( ( !jQuery( 'form' ).hasClass( 'frm-admin-viewing' ) ) ) {

Catches JS errors in calculations on the front end and alerts users. #66

Catches JS errors in calculations on the front end and alerts users. #66

Uh oh!

Conversation

LauraSteiner commented Sep 1, 2017

Uh oh!

jwahlin commented Sep 1, 2017

Uh oh!

LauraSteiner commented Sep 8, 2017

Uh oh!

stephywells left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LauraSteiner commented Jan 27, 2018

Uh oh!

stephywells left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LauraSteiner commented Apr 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants