Load Teacher Tools Catalog from Docs, Store in State #9829

thsparks · 2024-01-23T20:42:35Z

This change includes the following:

Loading the catalog from docs files (shared and target-specific, with "live" and "test" files in each).
Storing catalog in state once loaded
Selecting catalog criteria items to add to your rubric
Storing selected criteria instances in state
Basic UI for selecting criteria & displaying selected criteria
A few new types needed to read/parse criteria json

Notably, it does not include:

Any linking between the selected criteria and what gets sent to the editor for validation
Generating validator plans from the selected criteria (would be a prerequisite to sending to the editor)
Any target-specific catalogs (those will need to be checked into their respective pxt-target repos)

A few notes:

Test catalog entries are only loaded if the URL contains dbg=1. We could also show this in localhost builds, but I decided not to since I thought that could lead to confusion if someone saw a new catalog entry in localhost and didn't realize it needed to be moved to be visible in live.
In order to generate GUIDS, I added the uuid package: https://www.npmjs.com/package/uuid

Unfortunately upload targets don't work well with docs, but here are some screenshots:

…ag for logging errors.

…s for them.

…riteria at once, and utilize modal actions instead of creating separate buttons in the modal body.

srietkerk

Have lots of questions and comments, but this is super exciting stuff. Great work!!

docs/teachertool/catalog-shared-test.json

pxtblocks/code-validation/types.ts

srietkerk · 2024-01-23T23:24:47Z

pxtblocks/code-validation/types.ts

+    }
+
+    // Possible values for CriteriaParameterPicker, these are different ways a user may enter parameter values.
+    export type CriteriaParameterPicker = "blocksPicker" | "numericInput";


Styling nit but also a question. I was thinking that it would be nice to have types defined before the interfaces, but maybe it makes more sense to have this close to CriteriaParameter since that's where it's used. Are there standards around this area?

docs/teachertool/catalog-shared-test.json

srietkerk · 2024-01-23T23:35:34Z

teachertool/package-lock.json

@@ -9,11 +9,13 @@
      "version": "0.1.0",
      "dependencies": {
        "@types/node": "^16.11.33",
+        "@types/uuid": "^9.0.7",


Did you look into if we have this somewhere common? I'm wondering if there is somewhere client-side where we make unique ids already and we can thus avoid adding this.

I looked and wasn't able to find one. I was also surprised that we hadn't already needed to do this elsewhere, but searching for guid/uuid didn't turn up much at all.

srietkerk · 2024-01-23T23:55:06Z

teachertool/src/components/CatalogModal.tsx

+
+const CatalogModal: React.FC<IProps> = ({}) => {
+    const { state: teacherTool, dispatch } = useContext(AppStateContext);
+    const [ checkedCriteriaIds, setCheckedCriteria ] = useState<string[]>([]);


Question: is the <string[]> needed when using useState? I don't think I usually include it when the types are more definitive (list, string, number, bool) and want to make sure that I'm following convention.

Would it be possible to make this into a set? I think we could make handleCheckboxChange simpler if so.

I add it when the type is not easily inferred from the default value. So it's not really needed for a number or a string, but in this case it's not clear what type is expected in the list, so I think it's helpful to include.

Set might work well. I'll take a look.

srietkerk · 2024-01-23T23:58:28Z

teachertool/src/components/CatalogModal.tsx

+        hideCatalogModal();
+
+        // Clear for next open.
+        setCheckedCriteria([]);


Is this something we want to do? I thought we wanted to keep criteria that have already been added to the rubric "checked" so we don't have duplicates.

By design, we want to enable duplicates of some criteria. We should probably hide criteria from this list if they've already been added and duplicates aren't allowed (I forgot to do that, but I can add it...). The checks are specifically for which new criteria we're adding in this "modal-session" (for lack of a better term).

Makes sense! I don't think the "don't allow duplicates" case is something that needs to be included in this PR.

srietkerk · 2024-01-24T00:12:32Z

teachertool/src/services/loggingService.ts

-export const logError = (name: string, details: string) => {
-    pxt.tickEvent("teachertool.error", { name: formatName(name), message: details });
-    console.error(formatMessageForConsole(`${name}: ${details}`));
+export const logError = (name: string, details: string, props: pxt.Map<string | number> = {}) => {


nit: rename props to data or something similar. When I think of props, I think of React props..

teachertool/src/transforms/addCriteriaToRubric.ts

srietkerk · 2024-01-24T00:29:57Z

teachertool/src/components/CatalogModal.tsx

+    const { state: teacherTool, dispatch } = useContext(AppStateContext);
+    const [ checkedCriteriaIds, setCheckedCriteria ] = useState<string[]>([]);
+
+    function handleCheckboxChange(criteria: pxt.blocks.CatalogCriteria, newValue: boolean) {


Something I was thinking about.. What if we added a selected field to the Catalog Criteria Schema? Since we parse the catalog from docs into state, we can manage whether or not something is checked from the catalog directly. The look-up for an object like that would be faster, right? And then when the checkbox is changed, we can just toggle that field. The done and close modal would have to be modified, though. Also, I'm not a huge fan of the implications that this has on state.. it feels kinda icky that a modal would have so much power over the state of the catalog, but I also feel like this handler should be simpler than having to do a look-up or filter.

We want the user to be able to have multiple instances of the same catalog criteria, so just having selected on catalog criteria doesn't quite work (if it did, there'd be no real need for criteria instances at all).

I'm not sure I quite understand the concern around the handler. Could you elaborate on that a bit?

Yeah, my idea would be that the selected state on the catalog would be reset every time the modal is closed so when the user re-opens the modal, they have a fresh state. This might be more work than it's worth.

In regards to the handler, I'm a bit worried about a list operation every time the box is checked or unchecked. To be fair, they shouldn't be very computationally taxing, but the list operations plus conditions when we want to toggle a condition for something seems like a lot of logic.

Per our discussion in person, I think something like that might work (we'd need a new component like a CriteriaCheckbox to store selected, since I don't think we want UI-related fields to leak out into non-UI-specific classes), but it would probably just move the complexity elsewhere rather than actually simplifying things, so I'm inclined to leave this as-is at the moment.

thsparks · 2024-01-24T00:43:00Z

teachertool/src/transforms/addCriteriaToRubric.ts

+    const { state: teacherTool, dispatch } = stateAndDispatch();
+
+    // Create instances for each of the catalog criteria.
+    const instances = catalogCriteriaIds.reduce((accumulator, catalogCriteriaId) => {


reduce

I should probably just use a for...of loop here instead of reduce. Would be cleaner & easier to read.

eanders-ms

Still reviewing, but wanted to send out my comments so far.

eanders-ms · 2024-01-24T00:23:58Z

teachertool/src/App.tsx

@@ -37,9 +45,11 @@ function App() {
            <HeaderBar />


Just a note that we need to put all this rendering behind the ready flag. Otherwise stateAndDispatch may not be ready in code triggered by the first UI render.

I'll leave this be since you've added one in #9830

eanders-ms · 2024-01-24T00:30:03Z

teachertool/src/components/CatalogModal.tsx

+                            <Checkbox
+                                id={`chk${criteria.id}`}
+                                className="catalog-item"
+                                label={criteria.template}


TODO: Figure out how to send this through localization.

eanders-ms · 2024-01-24T00:33:47Z

teachertool/src/transforms/loadCatalog.ts

+    criteria: pxt.blocks.CatalogCriteria[];
+}
+
+export async function loadCatalog() {


Suggested change

export async function loadCatalog() {

export async function loadCatalogAsync() {

Update filename to match.

eanders-ms · 2024-01-24T00:40:58Z

teachertool/src/transforms/addCriteriaToRubric.ts

+import * as Actions from "../state/actions";
+import { logDebug, logError } from "../services/loggingService";
+
+export async function addCriteriaToRubric(catalogCriteriaIds: string[]) {


Suggested change

export async function addCriteriaToRubric(catalogCriteriaIds: string[]) {

export async function addCriteriaToRubricAsync(catalogCriteriaIds: string[]) {

eanders-ms · 2024-01-24T00:41:27Z

teachertool/src/transforms/hideCatalogModal.ts

+import { stateAndDispatch } from "../state";
+import * as Actions from "../state/actions";
+
+export async function hideCatalogModal() {


Does this need to be async?

eanders-ms · 2024-01-24T01:11:12Z

teachertool/src/transforms/removeCriteriaFromRubric.ts

+import * as Actions from "../state/actions";
+import { logDebug } from "../services/loggingService";
+
+export async function removeCriteriaFromRubric(instance: pxt.blocks.CriteriaInstance) {


Does this need to be async?

eanders-ms · 2024-01-24T01:14:37Z

teachertool/src/utils/index.ts

@@ -1,5 +1,6 @@
 import { nanoid } from "nanoid";
 import { NotificationWithId } from "../types";
+import { stateAndDispatch } from "../state";


Let's keep app state out of the utils file. I suggest moving getCatalogCriteriaWithId to /src/state/helpers.ts. We'll probably accumulate more of these helpers, and I'd like to keep utils uncomplicated.

eanders-ms · 2024-01-24T01:17:08Z

teachertool/src/transforms/loadCatalog.ts

+            const catalogInfo = catalogInfoParsed as CatalogInfo;
+            fullCatalog = fullCatalog.concat(catalogInfo.criteria ?? []);
+        } catch (e) {
+            logError("parse_catalog_failed", e as string, {catalogFile});


It might be nice to organize these error codes in one place and refer to them here like Errors.parse_catalog_failed. This would simplify looking up error codes when building a Kusto query.

eanders-ms · 2024-01-24T01:19:32Z

pxtblocks/code-validation/types.ts

+    }
+
+    // Possible values for CriteriaParameterPicker, these are different ways a user may enter parameter values.
+    export type CriteriaParameterPicker = "blocksPicker" | "numericInput";


"numericInput"

I don't think we need a picker for numbers. We can make the param value directly editable.

eanders-ms · 2024-01-24T01:21:32Z

pxtblocks/code-validation/types.ts

+    export interface CriteriaParameter {
+        name: string;
+        type: string;
+        picker: CriteriaParameterPicker;


Now I'm thinking we don't need a picker field at all. We can use the type field to infer the picker. type: "block_id" for the block picker.

thsparks · 2024-01-24T18:45:57Z

teachertool/src/transforms/loadCatalog.ts

+
+export async function loadCatalog() {
+    const { dispatch } = stateAndDispatch();
+    const catalogFiles = pxt.options.debug ? prodFiles.concat(testFiles) : prodFiles;


pxt.options.debug

Use a different flag or beta?

thsparks · 2024-01-24T19:02:53Z

teachertool/src/App.tsx

+            NotificationService.initialize();
+
+            // Load criteria catalog
+            loadCatalog();


loadCatalog

Additional flag for when all this is done?

Separate PR :)

thsparks · 2024-01-24T19:16:24Z

teachertool/src/transforms/loadCatalog.ts

+        let catalogContent = "";
+        try {
+            const catalogResponse = await fetch(catalogFile);
+            catalogContent = await catalogResponse.text();


text

json accessor?

thsparks · 2024-01-24T19:19:04Z

teachertool/src/components/CatalogModal.tsx

+        },
+    ]
+
+    return teacherTool.modal ? (


teacherTool.modal

teacherTool.modal == "catalog-modal"

thsparks · 2024-01-24T19:22:51Z

teachertool/src/components/CatalogModal.tsx

+
+    return teacherTool.modal ? (
+        <Modal className="catalog-modal" title={lf("Select the criteria you'd like to include")} onClose={closeModal} actions={modalActions}>
+            <div className="catalog-container" title={lf("Select the criteria you'd like to include")}>


[](http://example.com/codeflow?start=12&length=91)

redundant now?

thsparks · 2024-01-24T19:52:17Z

pxtblocks/code-validation/types.ts

@@ -0,0 +1,35 @@
+namespace pxt.blocks {
+    // A criteria defined in the catalog of all possible criteria for the user to choose from when creating a rubric.
+    export interface CatalogCriteria {


CatalogCriteria

move criteria types to teacher tool

thsparks · 2024-01-24T19:59:37Z

teachertool/src/state/reducer.ts

+                catalog: action.catalog,
+            };
+        }
+        case "ADD_CRITERIA_INSTANCES": {


ADD_CRITERIA_INSTANCES

Fully set, not append

thsparks · 2024-01-24T20:01:46Z

teachertool/src/components/ActiveRubricDisplay.tsx

+                const catalogCriteria = getCatalogCriteriaWithId(criteriaInstance.catalogCriteriaId);
+
+                return criteriaInstance?.catalogCriteriaId && (
+                        <div className="criteria-instance-display" id={`criteriaInstance${criteriaInstance.instanceId}`}>


div

key here

thsparks · 2024-01-24T20:04:13Z

teachertool/src/transforms/hideCatalogModal.ts

+import { stateAndDispatch } from "../state";
+import * as Actions from "../state/actions";
+
+export async function hideCatalogModal() {


hideCatalogModal

hideModal(ModalType)

…dempotent "set" action (apparently required by react).

kimprice

From the walkthrough you did yesterday, I have no concerns about this PR. LGTM! :)

… on the existing debug flag which could have unintended consequences

…ks/add_catalog

thsparks · 2024-01-25T20:19:20Z

teachertool/src/App.tsx

@@ -28,6 +34,10 @@ function App() {
                const cfg = await downloadTargetConfigAsync();
                dispatch(Actions.setTargetConfig(cfg || {}));
                pxt.BrowserUtils.initTheme();
+
+                // Load criteria catalog
+                await loadCatalogAsync();


@eanders-ms I wasn't totally sure how to plug into this initialization code. Is this suitable?

Yep, looks good! If we end up with a lot of async tasks here we should try to parallelize some of them. Nothing to worry about yet.

eanders-ms

Looks good!

eanders-ms · 2024-01-25T20:47:13Z

teachertool/src/types/errorCode.ts

@@ -0,0 +1,8 @@
+export enum ErrorCode {


Oh nice. We might want to strongly type logError to this. logError(errorCode: ErrorCode,

thsparks added 17 commits January 19, 2024 16:17

Update debug share link

724eeb9

Do not send a tick event for logging info and add optional property b…

40510b3

…ag for logging errors.

Initial change to support catalog in docs

432d79b

Move catalog criteria and instanced criteria into state, add new type…

67e7ffa

…s for them.

Add telemetry for adding a criteria

dff315f

Remove template, since it might be translated

1ddc90e

Clean up

2841ad9

Enable adding, removing, and displaying criteria instances in a rubric.

6001d4a

Basic styling changes

510bfc0

Put catalog selection into a modal

6307c80

Transforms for showing and hiding the catalog

5af0b63

Move rubric relaetd types into a separate file

b7eb28d

Change start to flex-start

654e400

Use check boxes instead of buttons, allow adding multiple instanced c…

7e50749

…riteria at once, and utilize modal actions instead of creating separate buttons in the modal body.

Remove sample test rule

07a7666

Rename showCatalog to showCatalogModal

0fbd1e4

Use helper for looking up catalog criteria from id

2b6462c

thsparks requested a review from a team January 23, 2024 20:42

srietkerk reviewed Jan 24, 2024

View reviewed changes

thsparks commented Jan 24, 2024

View reviewed changes

eanders-ms reviewed Jan 24, 2024

View reviewed changes

thsparks added 4 commits January 24, 2024 10:12

Remove picker from catalog criteria parameters

f729383

reorder imports

c61278a

App UI behind ready flag

cfb7aaf

Adjust null checks

38ba924

thsparks commented Jan 24, 2024

View reviewed changes

thsparks added 17 commits January 24, 2024 14:35

Use set instead of array for selected criteria in the catalog modal

b6a6d8c

microbit testing link

c3b0ae4

Move criteria types into teacher tool

aa62b37

Use nanoid, which we already have, rather than importing uuid.

07e0494

Add keys for components added while traversing a list

aade43b

Rename some functions to reflect their use more accurately

0e976f2

Semicolon

7b36a6c

Fix modal check

90bb803

Remove redundant div and css

fb72a38

Get a callstack when logging an error

ce3de89

Use for...of instead of reduce, change add criteria instances to an i…

47a9092

…dempotent "set" action (apparently required by react).

Remove unneeded async and rename function when it is needed

9a4eac3

Make hide modal function generic.

a0309ef

load catalog touch-ups

0c59596

Move getCatalogWithCriteriaId to a helpers file in state

bcf607d

Create a centralized location for error names.

44937e2

Remove ready check. Eric is adding one in a separate change.

7f7551d

kimprice approved these changes Jan 25, 2024

View reviewed changes

thsparks added 3 commits January 25, 2024 09:58

ErrorName -> ErrorCode

9c0fd12

Add testCatalog flag for including test catalogs, rather than relying…

d0155b5

… on the existing debug flag which could have unintended consequences

Merge branch 'master' of https://github.com/microsoft/pxt into thspar…

ebb7d09

…ks/add_catalog

thsparks commented Jan 25, 2024

View reviewed changes

eanders-ms approved these changes Jan 25, 2024

View reviewed changes

eanders-ms reviewed Jan 25, 2024

View reviewed changes

thsparks merged commit cd7ed80 into master Jan 25, 2024
6 checks passed

thsparks deleted the thsparks/add_catalog branch January 25, 2024 21:50

	export async function loadCatalog() {
	export async function loadCatalogAsync() {

	export async function addCriteriaToRubric(catalogCriteriaIds: string[]) {
	export async function addCriteriaToRubricAsync(catalogCriteriaIds: string[]) {

Load Teacher Tools Catalog from Docs, Store in State #9829

Load Teacher Tools Catalog from Docs, Store in State #9829

Conversation

thsparks commented Jan 23, 2024

srietkerk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srietkerk Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eanders-ms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eanders-ms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srietkerk Jan 24, 2024 •

edited

Loading