Merge branch 'master' into eanders-ms/pxt-bedrock

common-docs/teachertool/test/catalog-shared.json

@@ -13,6 +13,7 @@
                 {
                     "name": "question",
                     "type": "longString",
+                    "maxCharacters": 1000,
                     "paths": ["checks[0].question"]
                 },
                 {

localtypings/pxtarget.d.ts

@@ -1037,6 +1037,7 @@ declare namespace ts.pxtc {
         pyName?: string;
         pyQName?: string;
         snippetAddsDefinitions?: boolean;
+        isStatic?: boolean;
     }
 
     interface ApisInfo {

pxtcompiler/emitter/annotate.ts

@@ -116,7 +116,9 @@ namespace ts.pxtc {
                     case SK.SetAccessor:
                     case SK.MethodDeclaration:
                     case SK.MethodSignature:
-                        isMethod = true
+                        if (!isStatic(decl)) {
+                            isMethod = true
+                        }
                         break;
                     default:
                         break;

pxtcompiler/emitter/service.ts

@@ -204,6 +204,7 @@ namespace ts.pxtc {
                 pkg,
                 pkgs,
                 extendsTypes,
+                isStatic: decl.modifiers?.some(m => m.kind === SyntaxKind.StaticKeyword),
                 retType:
                     stmt.kind == SyntaxKind.Constructor ? "void" :
                         kind == SymbolKind.Module ? "" :

pxtlib/blocks.ts

@@ -126,7 +126,7 @@ namespace pxt.blocks {
             handlerArgs: []
         };
 
-        let instance = (fn.kind == ts.pxtc.SymbolKind.Method || fn.kind == ts.pxtc.SymbolKind.Property) && !fn.attributes.defaultInstance;
+        let instance = (fn.kind == ts.pxtc.SymbolKind.Method || fn.kind == ts.pxtc.SymbolKind.Property) && !fn.attributes.defaultInstance && !fn.isStatic;
         if (typeof fn.isInstance === "boolean" && !fn.attributes?.defaultInstance) instance = fn.isInstance;
         const hasBlockDef = !!fn.attributes._def;
         const defParameters = hasBlockDef ? fn.attributes._def.parameters.slice(0) : undefined;

teachertool/src/components/CriteriaInstanceDisplay.tsx

@@ -1,17 +1,18 @@
 import css from "./styling/CriteriaInstanceDisplay.module.scss";
-import { getCatalogCriteriaWithId } from "../state/helpers";
+import { getCatalogCriteriaWithId, getParameterDefinition } from "../state/helpers";
 import { CriteriaInstance, CriteriaParameterValue } from "../types/criteria";
 import { logDebug } from "../services/loggingService";
 import { setParameterValue } from "../transforms/setParameterValue";
 import { classList } from "react-common/components/util";
 import { getReadableBlockString, splitCriteriaTemplate } from "../utils";
-import { useContext, useMemo, useState } from "react";
+import { useContext, useEffect, useMemo, useState } from "react";
 import { Input } from "react-common/components/controls/Input";
 import { Button } from "react-common/components/controls/Button";
 import { AppStateContext } from "../state/appStateContext";
 import { Strings } from "../constants";
 import { showModal } from "../transforms/showModal";
 import { BlockPickerOptions } from "../types/modalOptions";
+import { validateParameterValue } from "../utils/validateParameterValue";
 
 interface InlineInputSegmentProps {
     initialValue: string;
@@ -27,24 +28,46 @@ const InlineInputSegment: React.FC<InlineInputSegmentProps> = ({
     shouldExpand,
     numeric,
 }) => {
-    const [isEmpty, setIsEmpty] = useState(!initialValue);
+    const [errorMessage, setErrorMessage] = useState(initialValue ? "" : Strings.ValueRequired);
+    const paramDefinition = useMemo(() => getParameterDefinition(instance.catalogCriteriaId, param.name), [param]);
+
+    useEffect(() => {
+        if (!paramDefinition) {
+            return;
+        }
+
+        // We still allow some invalid values to be set on the parameter so the user can see what they typed
+        // and the associated error.
+        // Without this, we risk erroring too soon (i.e. typing in first digit of number with min > 10),
+        // losing the user's input (which could be long), or desynchronizing the UI from the state.
+        // It will still be blocked via a separate check when the user tries to evaluate the criteria.
+        const paramValidation = validateParameterValue(paramDefinition, initialValue);
+        if (!paramValidation.valid) {
+            setErrorMessage(paramValidation.message ?? Strings.InvalidValue);
+        } else {
+            setErrorMessage("");
+        }
+    }, [initialValue]);
 
     function onChange(newValue: string) {
-        setIsEmpty(!newValue);
+        if (!newValue) {
+            setErrorMessage(Strings.ValueRequired);
+        }
+
         setParameterValue(instance.instanceId, param.name, newValue);
     }
 
-    const tooltip = isEmpty ? `${param.name}: ${Strings.ValueRequired}` : param.name;
+    const tooltip = errorMessage ? `${param.name}: ${errorMessage}` : param.name;
     return (
         <div title={tooltip} className={css["inline-input-wrapper"]}>
             <Input
                 className={classList(
                     css["inline-input"],
                     numeric ? css["number-input"] : css["string-input"],
                     shouldExpand ? css["long"] : undefined,
-                    isEmpty ? css["error"] : undefined
+                    errorMessage ? css["error"] : undefined
                 )}
-                icon={isEmpty ? "fas fa-exclamation-triangle" : undefined}
+                icon={errorMessage ? "fas fa-exclamation-triangle" : undefined}
                 initialValue={initialValue}
                 onChange={onChange}
                 preserveValueOnBlur={true}

teachertool/src/constants.ts

@@ -53,6 +53,11 @@ export namespace Strings {
     export const UnableToEvaluatePartial = lf("Unable to evaluate some criteria");
     export const GiveFeedback = lf("Give Feedback");
     export const MaxReached = lf("Maximum count reached for this item");
+    export const ExceedsMaxLength = lf("Exceeds maximum length");
+    export const MustBeANumber = lf("Must be a number");
+    export const BelowMin = lf("Below minimum value");
+    export const ExceedsMax = lf("Exceeds maximum value");
+    export const InvalidValue = lf("Invalid value");
 }
 
 export namespace Ticks {
@@ -77,6 +82,7 @@ export namespace Ticks {
     export const Print = "teachertool.print";
     export const UnhandledEvalError = "teachertool.evaluateerror";
     export const FeedbackForm = "teachertool.feedbackform";
+    export const ParamErrorMissingMessage = "teachertool.paramerrormissingmessage";
 }
 
 namespace Misc {

teachertool/src/state/helpers.ts

@@ -5,6 +5,7 @@ import { Checklist } from "../types/checklist";
 import { stateAndDispatch } from "./appStateContext";
 import { AppState } from "./state";
 import { Strings } from "../constants";
+import { CriteriaParameter } from "../types/criteriaParameters";
 
 export function getCatalogCriteriaWithId(id: string): CatalogCriteria | undefined {
     const { state } = stateAndDispatch();
@@ -15,6 +16,11 @@ export function getCriteriaInstanceWithId(state: AppState, id: string): Criteria
     return state.checklist.criteria.find(c => c.instanceId === id);
 }
 
+export function getParameterDefinition(catalogCriteriaId: string, paramName: string): CriteriaParameter | undefined {
+    const catalogCriteria = getCatalogCriteriaWithId(catalogCriteriaId);
+    return catalogCriteria?.params?.find(p => p.name === paramName);
+}
+
 export function getParameterValue(state: AppState, instanceId: string, paramName: string): string | undefined {
     const instance = getCriteriaInstanceWithId(state, instanceId);
     return instance?.params?.find(p => p.name === paramName)?.value;

teachertool/src/transforms/loadCatalogAsync.ts

@@ -8,7 +8,6 @@ const prodFiles = [
     "/teachertool/catalog-shared.json", // shared across all targets
     "/teachertool/catalog.json", // target-specific catalog
 ];
-
 export async function loadCatalogAsync() {
     const { dispatch } = stateAndDispatch();
     const fullCatalog = await loadTestableCollectionFromDocsAsync<CatalogCriteria>(prodFiles, "criteria");

teachertool/src/transforms/runSingleEvaluateAsync.ts

@@ -13,6 +13,8 @@ import { mergeEvalResult } from "./mergeEvalResult";
 import { setEvalResult } from "./setEvalResult";
 import { setUserFeedback } from "./setUserFeedback";
 import { Strings, Ticks } from "../constants";
+import { SystemParameter } from "../types/criteriaParameters";
+import { validateParameterValue } from "../utils/validateParameterValue";
 
 function generateValidatorPlan(
     criteriaInstance: CriteriaInstance,
@@ -53,8 +55,11 @@ function generateValidatorPlan(
             return undefined;
         }
 
-        if (catalogParam.type === "system" && catalogParam.key) {
-            param.value = getSystemParameter(catalogParam.key, teacherTool);
+        if (catalogParam.type === "system") {
+            const systemParam = catalogParam as SystemParameter;
+            if (systemParam.key) {
+                param.value = getSystemParameter(systemParam.key, teacherTool);
+            }
             if (!param.value) {
                 param.value = catalogParam.default;
             }
@@ -71,6 +76,17 @@ function generateValidatorPlan(
             return undefined;
         }
 
+        const validationResult = validateParameterValue(catalogParam, param.value);
+        if (!validationResult.valid) {
+            if (showErrors) {
+                logError(ErrorCode.invalidParameterValue, validationResult.message, {
+                    catalogId: criteriaInstance.catalogCriteriaId,
+                    paramName: param.name,
+                });
+            }
+            return undefined;
+        }
+
         for (const path of catalogParam.paths) {
             jp.apply(plan, path, () => param.value);
         }

teachertool/src/types/criteria.ts

@@ -1,4 +1,5 @@
 import { UserFeedback } from ".";
+import { CriteriaParameter } from "./criteriaParameters";
 
 // A criteria defined in the catalog of all possible criteria for the user to choose from when creating a checklist.
 export interface CatalogCriteria {
@@ -22,16 +23,6 @@ export interface CriteriaInstance {
     userFeedback?: UserFeedback;
 }
 
-// Represents a parameter definition in a catalog criteria.
-export type CriteriaParameterType = "string" | "longString" | "number" | "block" | "system";
-export interface CriteriaParameter {
-    name: string;
-    type: CriteriaParameterType;
-    default: string | undefined;
-    key: string | undefined;
-    paths: string[]; // The json path(s) to update with the parameter value in the catalog criteria.
-}
-
 // Represents a parameter value in a criteria instance.
 export interface CriteriaParameterValue {
     name: string;

teachertool/src/types/criteriaParameters.ts

@@ -0,0 +1,47 @@
+export type CriteriaParameterType = "string" | "longString" | "number" | "block" | "system";
+
+// Represents a parameter definition in a catalog criteria.
+export type CriteriaParameterBase = {
+    name: string;
+    type: CriteriaParameterType;
+    default: string | undefined;
+    paths: string[]; // The json path(s) to update with the parameter value in the catalog criteria.
+}
+
+export type StringParameterBase = CriteriaParameterBase & {
+    maxCharacters?: number;
+}
+
+export type StringParameter = StringParameterBase & {
+    type: "string";
+}
+
+export type LongStringParameter = StringParameterBase & {
+    type: "longString";
+}
+
+export type NumberParameter = CriteriaParameterBase & {
+    type: "number";
+    min?: number;
+    max?: number;
+}
+
+export type BlockParameter = CriteriaParameterBase & {
+    type: "block";
+}
+
+/**
+ * System parameters are fields that can change for a criteria but which are not set directly by the user.
+ * For example, the project id could be a parameter, but we fill it automatically at eval-time based on the loaded project.
+ */
+export type SystemParameter = CriteriaParameterBase & {
+    type: "system";
+    key?: string;
+}
+
+export type CriteriaParameter = StringParameter | LongStringParameter | NumberParameter | BlockParameter | SystemParameter;
+
+export interface CriteriaParameterValidationResult {
+    valid: boolean;
+    message?: string;
+}

teachertool/src/types/errorCode.ts

@@ -32,4 +32,6 @@ export enum ErrorCode {
     signInFailed = "signInFailed",
     loginApiError = "loginApiError",
     authCheckFailed = "authCheckFailed",
+    unrecognizedParameterType = "unrecognizedParameterType",
+    invalidParameterValue = "invalidParameterValue",
 }

teachertool/src/utils/validateParameterValue.ts

@@ -0,0 +1,67 @@
+import { Strings } from "../constants";
+import {
+    CriteriaParameter,
+    CriteriaParameterValidationResult,
+    LongStringParameter,
+    NumberParameter,
+    StringParameter,
+    StringParameterBase,
+} from "../types/criteriaParameters";
+
+export function validateParameterValue(param: CriteriaParameter, value: string): CriteriaParameterValidationResult {
+    switch (param.type) {
+        case "string":
+            return validateStringParameter(param, value);
+        case "longString":
+            return validateLongStringParameter(param, value);
+        case "number":
+            return validateNumberParameter(param, value);
+        case "block":
+        // Fall through to default case.
+        case "system":
+        // Fall through to default case.
+        default:
+            return { valid: true } as CriteriaParameterValidationResult;
+    }
+}
+
+function validateStringParameterBase(param: StringParameterBase, value: string): CriteriaParameterValidationResult {
+    if (!value) return { valid: true }; // Unset is okay for initial value
+
+    if (param.maxCharacters && value.length > param.maxCharacters) {
+        return { valid: false, message: Strings.ExceedsMaxLength };
+    }
+    return { valid: true };
+}
+
+function validateStringParameter(param: StringParameter, value: string): CriteriaParameterValidationResult {
+    return validateStringParameterBase(param, value);
+}
+
+function validateLongStringParameter(param: LongStringParameter, value: string): CriteriaParameterValidationResult {
+    return validateStringParameterBase(param, value);
+}
+
+function validateNumberParameter(param: NumberParameter, value: string): CriteriaParameterValidationResult {
+    // Ensure the value is numeric and within the specified range.
+    const num = Number(value);
+    if (isNaN(num)) {
+        return {
+            valid: false,
+            message: Strings.MustBeANumber,
+        };
+    }
+    if (param.min !== undefined && num < param.min) {
+        return {
+            valid: false,
+            message: Strings.BelowMin,
+        };
+    }
+    if (param.max !== undefined && num > param.max) {
+        return {
+            valid: false,
+            message: Strings.ExceedsMax,
+        };
+    }
+    return { valid: true };
+}