From 8ae9fd20cba42e515a14e8375feddc6d6a1d7181 Mon Sep 17 00:00:00 2001
From: Thomas Sparks <69657545+thsparks@users.noreply.github.com>
Date: Thu, 25 Apr 2024 16:41:06 -0700
Subject: [PATCH] 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 https://github.com/microsoft/pxt-microbit/issues/5597
---
 common-docs/teachertool/catalog-shared.json   | 16 +++++-------
 .../teachertool/test/catalog-shared.json      |  1 +
 teachertool/src/components/CatalogOverlay.tsx | 26 +++++++------------
 teachertool/src/components/MakecodeFrame.tsx  |  3 +--
 teachertool/src/state/helpers.ts              |  9 -------
 teachertool/src/types/criteria.ts             |  1 +
 6 files changed, 19 insertions(+), 37 deletions(-)

diff --git a/common-docs/teachertool/catalog-shared.json b/common-docs/teachertool/catalog-shared.json
index 2768e7c84224..0b33bb96a1d9 100644
--- a/common-docs/teachertool/catalog-shared.json
+++ b/common-docs/teachertool/catalog-shared.json
@@ -1,25 +1,20 @@
 {
     "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",
@@ -27,6 +22,7 @@
             "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",
diff --git a/common-docs/teachertool/test/catalog-shared.json b/common-docs/teachertool/test/catalog-shared.json
index b9e60b6d4d88..73e98c28ce44 100644
--- a/common-docs/teachertool/test/catalog-shared.json
+++ b/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",
diff --git a/teachertool/src/components/CatalogOverlay.tsx b/teachertool/src/components/CatalogOverlay.tsx
index 42ddb777be61..d3eeb93afc24 100644
--- a/teachertool/src/components/CatalogOverlay.tsx
+++ b/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}
                         />
                     )
                 );
diff --git a/teachertool/src/components/MakecodeFrame.tsx b/teachertool/src/components/MakecodeFrame.tsx
index e74a08c54e31..6b37faffe6ed 100644
--- a/teachertool/src/components/MakecodeFrame.tsx
+++ b/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()
diff --git a/teachertool/src/state/helpers.ts b/teachertool/src/state/helpers.ts
index 04d96f8e5766..c69062de4f5f 100644
--- a/teachertool/src/state/helpers.ts
+++ b/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)
-    );
-}
diff --git a/teachertool/src/types/criteria.ts b/teachertool/src/types/criteria.ts
index a44b339a0e35..5c07b2b3204a 100644
--- a/teachertool/src/types/criteria.ts
+++ b/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.