diff --git a/common-docs/teachertool/test/catalog-shared.json b/common-docs/teachertool/test/catalog-shared.json index baab5985cae8..f8dbe6d15c17 100644 --- a/common-docs/teachertool/test/catalog-shared.json +++ b/common-docs/teachertool/test/catalog-shared.json @@ -13,6 +13,7 @@ { "name": "question", "type": "longString", + "maxCharacters": 1000, "paths": ["checks[0].question"] }, { diff --git a/localtypings/pxtarget.d.ts b/localtypings/pxtarget.d.ts index 86f28d2dbadf..fe423d2477d7 100644 --- a/localtypings/pxtarget.d.ts +++ b/localtypings/pxtarget.d.ts @@ -1037,6 +1037,7 @@ declare namespace ts.pxtc { pyName?: string; pyQName?: string; snippetAddsDefinitions?: boolean; + isStatic?: boolean; } interface ApisInfo { diff --git a/pxtcompiler/emitter/annotate.ts b/pxtcompiler/emitter/annotate.ts index 7634dec6f2fa..72721cb2347c 100644 --- a/pxtcompiler/emitter/annotate.ts +++ b/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; diff --git a/pxtcompiler/emitter/service.ts b/pxtcompiler/emitter/service.ts index 3475ab6021f2..28f101c16c3e 100644 --- a/pxtcompiler/emitter/service.ts +++ b/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 ? "" : diff --git a/pxtlib/blocks.ts b/pxtlib/blocks.ts index c80299ccb649..0c4b4ccf360b 100644 --- a/pxtlib/blocks.ts +++ b/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; diff --git a/teachertool/src/components/CriteriaInstanceDisplay.tsx b/teachertool/src/components/CriteriaInstanceDisplay.tsx index 50e8f30089e3..0cf977b44726 100644 --- a/teachertool/src/components/CriteriaInstanceDisplay.tsx +++ b/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,14 +28,36 @@ const InlineInputSegment: React.FC = ({ 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 (
= ({ 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} diff --git a/teachertool/src/constants.ts b/teachertool/src/constants.ts index ee07e962d639..262dc5831d5c 100644 --- a/teachertool/src/constants.ts +++ b/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 { diff --git a/teachertool/src/state/helpers.ts b/teachertool/src/state/helpers.ts index 61e14759e06e..cd62e7650409 100644 --- a/teachertool/src/state/helpers.ts +++ b/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; diff --git a/teachertool/src/transforms/loadCatalogAsync.ts b/teachertool/src/transforms/loadCatalogAsync.ts index 607f73e72452..594c44446a0b 100644 --- a/teachertool/src/transforms/loadCatalogAsync.ts +++ b/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(prodFiles, "criteria"); diff --git a/teachertool/src/transforms/runSingleEvaluateAsync.ts b/teachertool/src/transforms/runSingleEvaluateAsync.ts index 06847cd4daf3..755692830993 100644 --- a/teachertool/src/transforms/runSingleEvaluateAsync.ts +++ b/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); } diff --git a/teachertool/src/types/criteria.ts b/teachertool/src/types/criteria.ts index 80c3422faa9f..2d8ab865d52e 100644 --- a/teachertool/src/types/criteria.ts +++ b/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; diff --git a/teachertool/src/types/criteriaParameters.ts b/teachertool/src/types/criteriaParameters.ts new file mode 100644 index 000000000000..4c78e05096cd --- /dev/null +++ b/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; +} diff --git a/teachertool/src/types/errorCode.ts b/teachertool/src/types/errorCode.ts index f3222df84a2d..8f0043082d83 100644 --- a/teachertool/src/types/errorCode.ts +++ b/teachertool/src/types/errorCode.ts @@ -32,4 +32,6 @@ export enum ErrorCode { signInFailed = "signInFailed", loginApiError = "loginApiError", authCheckFailed = "authCheckFailed", + unrecognizedParameterType = "unrecognizedParameterType", + invalidParameterValue = "invalidParameterValue", } diff --git a/teachertool/src/utils/validateParameterValue.ts b/teachertool/src/utils/validateParameterValue.ts new file mode 100644 index 000000000000..2f47cd9048f3 --- /dev/null +++ b/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 }; +}