Teacher Tool: Max Count for Catalog Criteria (#9986)

This change allows us to specify a maximum count for catalog criteria, which defines how many instances a user is allowed to have for a given catalog criteria. It also removes the attempted auto-detection of whether or not we should allow multiple instances (which was based the presence of parameters), since that didn't work well for criteria like "Must have [count] comments". If the field is not set, we assume no max.

I decided to go with a max count rather than a simple `allowMultiple` boolean, because I thought it might be useful to have some criteria where multiple instances are allowed, but not an unlimited number of them. (For example, I've limited the copilot questions to a max of 10, since they're expensive.)

I've also removed "All function definitions have comments" per feedback in microsoft/pxt-microbit#5597

common-docs/teachertool/catalog-shared.json

@@ -1,32 +1,28 @@
 {
     "criteria": [
-        {
-            "id": "8F97C9A6-CF16-48B4-A84F-3105C24B20DE",
-            "use": "functions_have_comments",
-            "template": "All function definitions have comments",
-            "docPath": "/teachertool",
-            "description": "All user-defined functions in the program have comments attached to them."
-        },
         {
             "id": "D21D76A2-D9FD-4F9B-B0AC-973CB870EA78",
             "use": "variable_set",
             "template": "At least one custom variable is set",
             "docPath": "/teachertool",
-            "description": "At least one user-defined variable is set to a value."
+            "description": "At least one user-defined variable is set to a value.",
+            "maxCount": 1
         },
         {
             "id": "0173898D-8A48-4266-AAB9-CE934471A734",
             "use": "variable_accessed",
             "template": "At least one variable is accessed",
             "docPath": "/teachertool",
-            "description": "At least one variable's value is read."
+            "description": "At least one variable's value is read.",
+            "maxCount": 1
         },
         {
             "id": "7AE7EA2A-3AC8-42DC-89DB-65E3AE157156",
             "use": "block_comment_used",
             "template": "At least ${count} comments",
             "description": "The project contains at least the specified number of comments.",
             "docPath": "/teachertool",
+            "maxCount": 1,
             "params": [
                 {
                     "name": "count",
@@ -62,6 +58,7 @@
             "template": "At least ${count} loops used",
             "docPath": "/teachertool",
             "description": "The program uses at least this many loops of any kind (for, repeat, while, or for-of).",
+            "maxCount": 1,
             "params": [
                 {
                     "name": "count",
@@ -77,6 +74,7 @@
             "template": "At least ${count} custom functions exist and get called",
             "docPath": "/teachertool",
             "description": "At least this many user-defined functions are created and called.",
+            "maxCount": 1,
             "params": [
                 {
                     "name": "count",

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

@@ -6,6 +6,7 @@
             "template": "Ask Copilot: ${question}",
             "description": "Experimental: AI outputs may not be accurate. Use with caution and always review responses.",
             "docPath": "/teachertool",
+            "maxCount": 10,
             "params": [
                 {
                     "name": "question",

teachertool/src/components/CatalogOverlay.tsx

@@ -32,22 +32,17 @@ const CatalogHeader: React.FC<CatalogHeaderProps> = ({ onClose }) => {
 
 interface CatalogItemLabelProps {
     catalogCriteria: CatalogCriteria;
-    allowsMultiple: boolean;
-    existingInstanceCount: number;
+    isMaxed: boolean;
     recentlyAdded: boolean;
 }
-const CatalogItemLabel: React.FC<CatalogItemLabelProps> = ({
-    catalogCriteria,
-    allowsMultiple,
-    existingInstanceCount,
-    recentlyAdded,
-}) => {
-    const canAddMore = allowsMultiple || existingInstanceCount === 0;
-    const showRecentlyAddedIndicator = recentlyAdded && canAddMore;
+const CatalogItemLabel: React.FC<CatalogItemLabelProps> = ({ catalogCriteria, isMaxed, recentlyAdded }) => {
+    const showRecentlyAddedIndicator = recentlyAdded && !isMaxed;
     return (
         <div className={css["catalog-item-label"]}>
             <div className={css["action-indicators"]}>
-                {canAddMore ? (
+                {isMaxed ? (
+                    <span>{Strings.Max}</span>
+                ) : (
                     <>
                         <i
                             className={classList(
@@ -65,8 +60,6 @@ const CatalogItemLabel: React.FC<CatalogItemLabelProps> = ({
                             title={Strings.AddToChecklist}
                         />
                     </>
-                ) : (
-                    <span className={css["max-label"]}>{Strings.Max}</span>
                 )}
             </div>
             <ReadOnlyCriteriaDisplay catalogCriteria={catalogCriteria} showDescription={true} />
@@ -116,10 +109,10 @@ const CatalogList: React.FC = () => {
     return (
         <div className={css["catalog-list"]}>
             {criteria.map(c => {
-                const allowsMultiple = c.params !== undefined && c.params.length !== 0; // TODO add a json flag for this (MaxCount or AllowMultiple)
                 const existingInstanceCount = teacherTool.rubric.criteria.filter(
                     i => i.catalogCriteriaId === c.id
                 ).length;
+                const isMaxed = c.maxCount !== undefined && existingInstanceCount >= c.maxCount;
                 return (
                     c.template && (
                         <Button
@@ -130,13 +123,12 @@ const CatalogList: React.FC = () => {
                             label={
                                 <CatalogItemLabel
                                     catalogCriteria={c}
-                                    allowsMultiple={allowsMultiple}
-                                    existingInstanceCount={existingInstanceCount}
+                                    isMaxed={isMaxed}
                                     recentlyAdded={recentlyAddedIds[c.id] !== undefined}
                                 />
                             }
                             onClick={() => onItemClicked(c)}
-                            disabled={!allowsMultiple && existingInstanceCount > 0}
+                            disabled={isMaxed}
                         />
                     )
                 );

teachertool/src/components/MakecodeFrame.tsx

@@ -11,8 +11,7 @@ interface IProps {}
 
 export const MakeCodeFrame: React.FC<IProps> = () => {
     const { state: teacherTool } = useContext(AppStateContext);
-    const [ frameId ] = useState(pxt.Util.guidGen());
-
+    const [frameId] = useState(pxt.Util.guidGen());
 
     function createIFrameUrl(shareId: string): string {
         const editorUrl: string = pxt.BrowserUtils.isLocalHost()

teachertool/src/state/helpers.ts

@@ -78,12 +78,3 @@ export function getSafeRubricName(state: AppState): string | undefined {
 export function getCatalogCriteria(state: AppState): CatalogCriteria[] {
     return state.catalog?.filter(c => !c.hideInCatalog) ?? [];
 }
-
-export function criteriaIsSelectable(state: AppState, catalogCriteria: CatalogCriteria): boolean {
-    // Return a criteria as selectable if it has parameters (so it can be used multiple times in a rubric)
-    // or if it has not yet been used in the active rubric.
-    return (
-        (catalogCriteria.params && catalogCriteria.params.length > 0) ||
-        !state.rubric.criteria.some(c => c.catalogCriteriaId === catalogCriteria.id)
-    );
-}

teachertool/src/types/criteria.ts

@@ -7,6 +7,7 @@ export interface CatalogCriteria {
     docPath: string | undefined; // Path to documentation
     params: CriteriaParameter[] | undefined; // Any parameters that affect the criteria
     hideInCatalog?: boolean; // Whether the criteria should be hidden in the user-facing catalog
+    maxCount?: number; // The maximum number of instances allowed for this criteria within a single checklist. Unlimited if undefined.
 }
 
 // An instance of a criteria in a rubric.