#554 Fixed the viewer crashing when trying to parse erroneous/invalid…

… variables. Overhauled variable creation/validation in the visual editor to supply reasonable defaults when creating new variables or changing variables from one type to another, and added relevant logic to display messages when there are errors present and highlight relevant inputs. Moved some CSS rules into new files closer to the components they're styling.

packages/app/obojobo-document-engine/src/scripts/oboeditor/components/variables/variable-block.js

@@ -1,3 +1,5 @@
+import './variable-block.scss'
+
 import React from 'react'
 import {
 	STATIC_VALUE,
@@ -124,6 +126,11 @@ const VariableBlock = props => {
 					}
 				})()}
 			</small>
+			{ variable.errors ?
+			<small className='error-count'>
+				<span>{Object.keys(variable.errors).length} issues</span>
+			</small>
+			: null }
 		</button>
 	)
 }

packages/app/obojobo-document-engine/src/scripts/oboeditor/components/variables/variable-block.scss

@@ -0,0 +1,39 @@
+@import '~styles/includes';
+
+$variable-selected: #285aa4;
+
+.single-variable {
+    border: none;
+    background: transparent;
+    font-family: 'Libre Franklin', Arial, sans-serif;
+    width: 100%;
+    cursor: pointer;
+    margin: 0;
+    color: inherit;
+    font-size: 1em;
+    background-color: $color-bg;
+    border-bottom: solid 1px rgba(0, 0, 0, 0.1);
+    border-right: solid 1px rgba(0, 0, 0, 0.1);
+    padding: 0.5em;
+    text-align: left;
+    padding-left: 1em;
+
+    h4 {
+        padding: 0;
+        margin: 0;
+    }
+
+    small {
+        font-size: 0.75em;
+
+        &.error-count {
+            display: block;
+            color: $color-dangerous;
+        }
+    }
+}
+
+.variable-is-selected {
+    border-right: none;
+    color: $variable-selected;
+}

packages/app/obojobo-document-engine/src/scripts/oboeditor/components/variables/variable-list-modal.js

@@ -8,6 +8,7 @@ import NewVariable from './new-variable/new-variable'
 import VariableBlock from './variable-block'
 import { RANDOM_NUMBER, RANDOM_LIST } from './constants'
 import { getParsedRange } from '../../../common/util/range-parsing'
+import { changeVariableToType, validateVariableValue, validateMultipleVariables } from './variable-util'
 
 const { Button } = Common.components
 const { SimpleDialog } = Common.components.modal
@@ -164,27 +165,41 @@ const VariableListModal = props => {
 
 	const [currSelect, setCurrSelect] = useState(0)
 	const [creatingVariable, setCreatingVariable] = useState(false)
-	const [variables, setVariables] = useState(rangesToIndividualValues(props.content.variables))
+	const [variables, setVariables] = useState(validateMultipleVariables(rangesToIndividualValues(props.content.variables)))
 
 	const onClickVariable = index => {
 		setCreatingVariable(false)
 		setCurrSelect(index)
 		tabRef.current.focus() // Tab focus on the first element
 	}
 
+	// manage variable property changes and validation, also add/remove variable errors
 	const onChange = event => {
 		const name = event.target.name
 		const value = event.target.type === 'checkbox' ? event.target.checked : event.target.value
 
 		const updatedVariables = [...variables]
 		updatedVariables[currSelect][name] = value
 
-		// Delete all properties if type is change
+		const variableErrors = updatedVariables[currSelect].errors ?? {}
+
+		// if the variable type is changed, keep any relevant attributes and remove any others
+		// this should also reset issues based on the new type
 		if (name === 'type') {
-			for (const attr in updatedVariables[currSelect]) {
-				if (attr !== 'name' && attr !== 'type') {
-					delete updatedVariables[currSelect][attr]
-				}
+			changeVariableToType(updatedVariables[currSelect], value)
+		} else {
+			// indicate any errors with the changed property - or remove any that existed if it's valid
+			const error = validateVariableValue(name, value)
+			if (error) {
+				variableErrors[name] = true
+			} else {
+				delete variableErrors[name]
+			}
+
+			if (Object.keys(variableErrors).length) {
+				updatedVariables[currSelect].errors = variableErrors
+			} else {
+				delete updatedVariables[currSelect].errors
 			}
 		}
 
@@ -203,11 +218,8 @@ const VariableListModal = props => {
 		}
 
 		const newVariable = { type, name: 'var' + (index === 1 ? '' : index) }
-		if (type === RANDOM_NUMBER || type === RANDOM_LIST) {
-			newVariable['decimalPlacesMin'] = '0'
-			newVariable['decimalPlacesMax'] = '0'
-		}
-		const updatedVariables = [...variables, newVariable]
+
+		const updatedVariables = [...variables, changeVariableToType(newVariable, type)]
 		setVariables(updatedVariables)
 		setCurrSelect(variables.length)
 		setCreatingVariable(false)
@@ -264,44 +276,51 @@ const VariableListModal = props => {
 			onConfirm={handleOnConfirm}
 			focusOnFirstElement={focusOnFirstElement}
 		>
-			<div className="variable-list-modal">
-				<nav className="variable-list" role="navigation">
-					{variables.map((variable, index) => (
-						<VariableBlock
-							key={variable.name}
-							variable={{ ...variable }}
-							currSelect={currSelect}
-							isSelected={index === currSelect}
-							creatingVariable={creatingVariable}
-							firstRef={index === 0 ? firstRef : null}
-							onClickVariable={onClickVariable}
-							index={index}
-							onClick={() => onClickVariable(index)}
-						/>
-					))}
+			<div className='variable-list-modal-parent'>
+				<span className='error-warning'>
+					<strong>Warning:</strong> Variables with errors will not be subsituted correctly in the viewer.
+					<br/>
+					Please resolve all highlighted variable errors.
+				</span>
+				<div className="variable-list-modal">
+					<nav className="variable-list" role="navigation">
+						{variables.map((variable, index) => (
+							<VariableBlock
+								key={variable.name}
+								variable={{ ...variable }}
+								currSelect={currSelect}
+								isSelected={index === currSelect}
+								creatingVariable={creatingVariable}
+								firstRef={index === 0 ? firstRef : null}
+								onClickVariable={onClickVariable}
+								index={index}
+								onClick={() => onClickVariable(index)}
+							/>
+						))}
+
+						{creatingVariable ? (
+							<div className="variable-holder">
+								<p>New Variable...</p>
+							</div>
+						) : (
+							<Button className="create-variable-button" onClick={() => setCreatingVariable(true)}>
+								+ Create Variable
+							</Button>
+						)}
+					</nav>
 
 					{creatingVariable ? (
-						<div className="variable-holder">
-							<p>New Variable...</p>
-						</div>
+						<NewVariable addVariable={addVariable} />
 					) : (
-						<Button className="create-variable-button" onClick={() => setCreatingVariable(true)}>
-							+ Create Variable
-						</Button>
+						<VariableProperty
+							variable={variables[currSelect]}
+							onChange={onChange}
+							duplicateVariable={duplicateVariable}
+							deleteVariable={deleteVariable}
+							tabRef={tabRef}
+						/>
 					)}
-				</nav>
-
-				{creatingVariable ? (
-					<NewVariable addVariable={addVariable} />
-				) : (
-					<VariableProperty
-						variable={variables[currSelect]}
-						onChange={onChange}
-						duplicateVariable={duplicateVariable}
-						deleteVariable={deleteVariable}
-						tabRef={tabRef}
-					/>
-				)}
+				</div>
 			</div>
 		</SimpleDialog>
 	)

packages/app/obojobo-document-engine/src/scripts/oboeditor/components/variables/variable-list-modal.scss

@@ -1,5 +1,17 @@
 @import '~styles/includes';
 
+.variable-list-modal-parent {
+	.error-warning {
+		display: none;
+		font-size: 0.75em;
+		margin: 0 auto 0.25em;
+	}
+
+	&:has(.error-count) .error-warning {
+		display: block;
+	}
+}
+
 .variable-list-modal {
 	font-size: 0.85em;
 	background-color: $color-bg2;
@@ -10,37 +22,9 @@
 	display: flex;
 
 	.variable-list {
-		$variable-selected: #285aa4;
-
 		overflow-y: scroll;
 		width: 12em;
 
-		.single-variable {
-			border: none;
-			background: transparent;
-			font-family: 'Libre Franklin', Arial, sans-serif;
-			width: 100%;
-			cursor: pointer;
-			margin: 0;
-			color: inherit;
-			font-size: 1em;
-			background-color: $color-bg;
-			border-bottom: solid 1px rgba(0, 0, 0, 0.1);
-			border-right: solid 1px rgba(0, 0, 0, 0.1);
-			padding: 0.5em;
-			text-align: left;
-			padding-left: 1em;
-
-			h4 {
-				padding: 0;
-				margin: 0;
-			}
-
-			small {
-				font-size: 0.75em;
-			}
-		}
-
 		.variable-holder {
 			background-color: $color-bg;
 			border-bottom: solid 1px rgba(0, 0, 0, 0.1);
@@ -52,11 +36,6 @@
 			border-right: none;
 		}
 
-		.variable-is-selected {
-			border-right: none;
-			color: $variable-selected;
-		}
-
 		.create-variable-button {
 			margin: 0.5rem;
 

packages/app/obojobo-document-engine/src/scripts/oboeditor/components/variables/variable-property/variable-property.js

@@ -20,7 +20,7 @@ const VariableProperty = props => {
 
 	return (
 		<div className="variable-property">
-			<label className="group-item">
+			<label className={`group-item ${variable.errors && variable.errors.name ? 'has-error' : ''}`}>
 				<label>
 					<strong>Name:</strong>
 				</label>
@@ -33,7 +33,7 @@ const VariableProperty = props => {
 				/>
 
 				<MoreInfoButton ariaLabel="Click to explain variable's name rules">
-					<p>Alphanumeric plus underscore only</p>
+					<p>Alphanumeric plus underscore only, can not start with a number</p>
 				</MoreInfoButton>
 
 				<Button

packages/app/obojobo-document-engine/src/scripts/oboeditor/components/variables/variable-property/variable-property.scss

@@ -17,6 +17,10 @@
 		border: solid 1px rgba(0, 0, 0, 0.1);
 		box-sizing: border-box;
 		flex: 1;
+
+		&.has-error {
+			border: solid 1px $color-dangerous;
+		}
 	}
 
 	.variable-property--checkbox-item {
@@ -44,6 +48,10 @@
 			calc(100% - 2.5em) 0.5em;
 		background-size: 0.4em 0.4em, 0.4em 0.4em, 0.1em 1.5em;
 		background-repeat: no-repeat;
+
+		&.has-error {
+			border: solid 1px $color-dangerous;
+		}
 	}
 
 	.group-item {
@@ -52,6 +60,10 @@
 		padding: 0;
 		margin: 0.8em 0;
 
+		&.has-error input {
+			border: solid 1px $color-dangerous;
+		}
+
 		label {
 			margin: 0;
 			width: 4em;