Skip to content

Commit

Permalink
Teacher Tool: More Telemetry (#10196)
Browse files Browse the repository at this point in the history
This change adds a few telemetry events and some code changes to support them:

1. Changing the eval result (including a field indicating whether or not it's manual and whether the previous result was manual, which necessitated adding resultIsManual to the result type. I think this could be helpful in the future too, if we decide not to overwrite manual results when doing bulk evaluate)
2. Changing eval notes (debounced)
3. Run single eval (including hash of the checklist which should help with understanding evals / checklist and usage of pre-made checklists)
4. Run bulk eval
5. Importing a checklist (whether there's an invalid file, a successful import, or closed without doing anything)
6. Loading in new projects
7. Block picker opened & block selected
8. Adjusted logging of opening pre-built checklists so we can include checklist hash (and error reporting if a pre-built checklist is invalid)


There's also a one-line bug fix so we don't show the "Replace existing checklist" warning when the user clicks new checklist for the first time (only consider having an "existing checklist" if there is criteria, regardless of name).
  • Loading branch information
thsparks authored Sep 20, 2024
1 parent 6cbce6f commit 1e22ec7
Show file tree
Hide file tree
Showing 24 changed files with 167 additions and 50 deletions.
2 changes: 2 additions & 0 deletions teachertool/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { SignedOutPanel } from "./components/SignedOutPanel";
import * as authClient from "./services/authClient";
import { ErrorCode } from "./types/errorCode";
import { loadProjectMetadataAsync } from "./transforms/loadProjectMetadataAsync";
import { Ticks } from "./constants";

export const App = () => {
const { state, dispatch } = useContext(AppStateContext);
Expand Down Expand Up @@ -47,6 +48,7 @@ export const App = () => {
const decoded = decodeURIComponent(projectParam);
const shareId = pxt.Cloud.parseScriptId(decoded);
if (!!shareId) {
pxt.tickEvent(Ticks.LoadProjectFromUrl);
await loadProjectMetadataAsync(decoded, shareId);
}
}
Expand Down
3 changes: 2 additions & 1 deletion teachertool/src/components/BlockPickerModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { getReadableBlockString } from "../utils";
import { setParameterValue } from "../transforms/setParameterValue";
import { ErrorCode } from "../types/errorCode";
import { logError } from "../services/loggingService";
import { Strings } from "../constants";
import { Strings, Ticks } from "../constants";
import { BlockPickerOptions } from "../types/modalOptions";
import css from "./styling/BlockPickerModal.module.scss";

Expand Down Expand Up @@ -51,6 +51,7 @@ const BlockPickerCategory: React.FC<BlockPickerCategoryProps> = ({ category, onB
const [expanded, setExpanded] = useState(false);

function blockSelected(block: pxt.editor.ToolboxBlockDefinition) {
pxt.tickEvent(Ticks.BlockPickerBlockSelected, { blockId: block.blockId ?? "" });
onBlockSelected?.(block);
}

Expand Down
10 changes: 2 additions & 8 deletions teachertool/src/components/CatalogOverlay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,7 @@ const CatalogItemLabel: React.FC<CatalogItemLabelProps> = ({ catalogCriteria, is
<div className={css["catalog-item-label"]}>
<div className={css["action-indicators"]}>
{isMaxed ? (
<i
className="fas fa-check"
title={Strings.MaxReached}
/>
<i className="fas fa-check" title={Strings.MaxReached} />
) : (
<>
<i
Expand All @@ -59,10 +56,7 @@ const CatalogItemLabel: React.FC<CatalogItemLabelProps> = ({ catalogCriteria, is
title={lf("Added!")}
/>
<i
className={classList(
"fas fa-plus",
recentlyAdded ? css["hide-indicator"] : undefined
)}
className={classList("fas fa-plus", recentlyAdded ? css["hide-indicator"] : undefined)}
title={Strings.AddToChecklist}
/>
</>
Expand Down
2 changes: 1 addition & 1 deletion teachertool/src/components/CriteriaEvalResultDropdown.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export const CriteriaEvalResultDropdown: React.FC<CriteriaEvalResultProps> = ({
selectedId={selectedResult}
className={classList("rounded", selectedResult)}
items={dropdownItems}
onItemSelected={id => setEvalResultOutcome(criteriaId, itemIdToCriteriaResult[id])}
onItemSelected={id => setEvalResultOutcome(criteriaId, itemIdToCriteriaResult[id], true)}
/>
);
};
3 changes: 2 additions & 1 deletion teachertool/src/components/CriteriaInstanceDisplay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ 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 { Strings, Ticks } from "../constants";
import { showModal } from "../transforms/showModal";
import { BlockPickerOptions } from "../types/modalOptions";
import { validateParameterValue } from "../utils/validateParameterValue";
Expand Down Expand Up @@ -91,6 +91,7 @@ interface BlockData {
const BlockInputSegment: React.FC<BlockInputSegmentProps> = ({ instance, param }) => {
const { state: teacherTool } = useContext(AppStateContext);
function handleClick() {
pxt.tickEvent(Ticks.BlockPickerOpened, { criteriaCatalogId: instance.catalogCriteriaId });
showModal({
modal: "block-picker",
criteriaInstanceId: instance.instanceId,
Expand Down
15 changes: 12 additions & 3 deletions teachertool/src/components/CriteriaResultEntry.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { removeCriteriaFromChecklist } from "../transforms/removeCriteriaFromChe
import { Button } from "react-common/components/controls/Button";
import { setEvalResult } from "../transforms/setEvalResult";
import { showToast } from "../transforms/showToast";
import { makeToast } from "../utils";
import { getChecklistHash, getObfuscatedProjectId, makeToast } from "../utils";

interface CriteriaResultNotesProps {
criteriaId: string;
Expand Down Expand Up @@ -69,7 +69,11 @@ const CriteriaResultError: React.FC<CriteriaResultErrorProps> = ({ criteriaInsta
leftIcon="fas fa-times-circle"
title={Strings.Dismiss}
onClick={() =>
setEvalResult(criteriaInstanceId, { result: EvaluationStatus.NotStarted, error: undefined })
setEvalResult(criteriaInstanceId, {
result: EvaluationStatus.NotStarted,
resultIsManual: false,
error: undefined,
})
}
/>
</div>
Expand All @@ -80,7 +84,12 @@ const CriteriaResultToolbarTray: React.FC<{ criteriaId: string }> = ({ criteriaI
const { state: teacherTool } = useContext(AppStateContext);

async function handleEvaluateClickedAsync() {
pxt.tickEvent(Ticks.Evaluate);
const criteriaInstance = getCriteriaInstanceWithId(teacherTool, criteriaId);
pxt.tickEvent(Ticks.SingleEvaluate, {
catalogCriteriaId: criteriaInstance?.catalogCriteriaId ?? "",
checklistHash: getChecklistHash(teacherTool.checklist),
projectId: getObfuscatedProjectId(teacherTool.projectMetadata?.id),
});
const success = await runSingleEvaluateAsync(criteriaId, true);

if (success) {
Expand Down
12 changes: 10 additions & 2 deletions teachertool/src/components/EvalResultDisplay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { useReactToPrint } from "react-to-print";
import { AppStateContext } from "../state/appStateContext";
import { CriteriaResultEntry } from "./CriteriaResultEntry";
import { QRCodeSVG } from "qrcode.react";
import { getProjectLink } from "../utils";
import { getChecklistHash, getObfuscatedProjectId, getProjectLink } from "../utils";
import { classList } from "react-common/components/util";
import { AddCriteriaButton } from "./AddCriteriaButton";
import { DebouncedInput } from "./DebouncedInput";
Expand All @@ -30,7 +30,15 @@ const ResultsHeader: React.FC<ResultsHeaderProps> = ({ printRef }) => {
const [checklistNameInputRef, setChecklistNameInputRef] = useState<HTMLInputElement>();

const handleEvaluateClickedAsync = async () => {
pxt.tickEvent(Ticks.Evaluate);
pxt.tickEvent(Ticks.BulkEvaluate, {
fromUserInteraction: true + "",
runOnLoad: false + "",
criteriaCount: checklist.criteria.length,
catalogCriteriaIds: JSON.stringify(checklist.criteria.map(c => c.catalogCriteriaId)),
checklistHash: getChecklistHash(checklist),
projectId: getObfuscatedProjectId(projectMetadata?.id),
});

await runEvaluateAsync(true);
};

Expand Down
3 changes: 1 addition & 2 deletions teachertool/src/components/HomeScreen.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ interface ChecklistResourceCardProps {

const ChecklistResourceCard: React.FC<ChecklistResourceCardProps> = ({ cardTitle, imageUrl, checklistUrl }) => {
const onCardClickedAsync = async () => {
pxt.tickEvent(Ticks.LoadChecklist, { checklistUrl });
await loadChecklistAsync(checklistUrl);
};
return (
Expand Down Expand Up @@ -148,7 +147,7 @@ const GetStarted: React.FC = () => {
};

const onImportChecklistClicked = () => {
pxt.tickEvent(Ticks.ImportChecklist);
pxt.tickEvent(Ticks.ImportChecklistOpen);
showModal({ modal: "import-checklist" } as ImportChecklistOptions);
};

Expand Down
15 changes: 11 additions & 4 deletions teachertool/src/components/ImportChecklistModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { Modal } from "react-common/components/controls/Modal";
import { hideModal } from "../transforms/hideModal";
import { getChecklistFromFileAsync } from "../transforms/getChecklistFromFileAsync";
import { DragAndDropFileSurface } from "./DragAndDropFileSurface";
import { Strings } from "../constants";
import { Strings, Ticks } from "../constants";
import css from "./styling/ImportChecklistModal.module.scss";
import { replaceActiveChecklistAsync } from "../transforms/replaceActiveChecklistAsync";

Expand All @@ -14,24 +14,31 @@ export const ImportChecklistModal: React.FC<IProps> = () => {
const { state: teacherTool } = useContext(AppStateContext);
const [errorMessage, setErrorMessage] = useState<string | undefined>(undefined);

function closeModal() {
function closeModal(hasImported: boolean) {
pxt.tickEvent(Ticks.ImportChecklistClose, { hasImported: hasImported + "" });
setErrorMessage(undefined);
hideModal();
}

async function handleFileDroppedAsync(file: File) {
const parsedChecklist = await getChecklistFromFileAsync(file, false /* allow partial */);
if (!parsedChecklist) {
pxt.tickEvent(Ticks.ImportChecklistInvalidFile);
setErrorMessage(Strings.InvalidChecklistFile);
} else {
pxt.tickEvent(Ticks.ImportChecklistSuccess);
setErrorMessage(undefined);
closeModal();
closeModal(true);
replaceActiveChecklistAsync(parsedChecklist);
}
}

return teacherTool.modalOptions?.modal === "import-checklist" ? (
<Modal title={Strings.ImportChecklist} onClose={closeModal} className={css["import-checklist-modal"]}>
<Modal
title={Strings.ImportChecklist}
onClose={() => closeModal(false)}
className={css["import-checklist-modal"]}
>
<div className={css["import-checklist"]}>
<DragAndDropFileSurface onFileDroppedAsync={handleFileDroppedAsync} errorMessage={errorMessage} />
</div>
Expand Down
12 changes: 11 additions & 1 deletion teachertool/src/components/ShareLinkInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ import { AppStateContext } from "../state/appStateContext";
import { classList } from "react-common/components/util";
import { Input } from "react-common/components/controls/Input";
import { loadProjectMetadataAsync } from "../transforms/loadProjectMetadataAsync";
import { Strings, Ticks } from "../constants";
import { getChecklistHash, makeToast } from "../utils";
import { showToast } from "../transforms/showToast";

interface IProps {}

Expand All @@ -26,7 +29,14 @@ export const ShareLinkInput: React.FC<IProps> = () => {

const onEnterKey = useCallback(() => {
const shareId = pxt.Cloud.parseScriptId(text);
if (!!shareId && !(shareId === projectMetadata?.shortid || shareId === projectMetadata?.persistId)) {
if (!shareId) {
pxt.tickEvent(Ticks.LoadProjectInvalid);
showToast(makeToast("error", Strings.InvalidShareLink));
return;
}

if (shareId !== projectMetadata?.shortid && shareId !== projectMetadata?.persistId) {
pxt.tickEvent(Ticks.LoadProjectFromInput, { checklistHash: getChecklistHash(teacherTool.checklist) });
loadProjectMetadataAsync(text, shareId);
}
}, [text, projectMetadata?.shortid, projectMetadata?.persistId]);
Expand Down
18 changes: 15 additions & 3 deletions teachertool/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ export namespace Strings {
export const BelowMin = lf("Below minimum value");
export const ExceedsMax = lf("Exceeds maximum value");
export const InvalidValue = lf("Invalid value");
export const InvalidShareLink = lf("Invalid share link");
}

export namespace Ticks {
Expand All @@ -67,10 +68,14 @@ export namespace Ticks {
export const OrgLink = "teachertool.orglink";
export const Error = "teachertool.error";
export const NewChecklist = "teachertool.newchecklist";
export const ImportChecklist = "teachertool.importchecklist";
export const ImportChecklistOpen = "teachertool.importchecklist.open";
export const ImportChecklistInvalidFile = "teachertool.importchecklist.invalidfile";
export const ImportChecklistSuccess = "teachertool.importchecklist.success";
export const ImportChecklistClose = "teachertool.importchecklist.close";
export const ExportChecklist = "teachertool.exportchecklist";
export const LoadChecklist = "teachertool.loadchecklist";
export const Evaluate = "teachertool.evaluate";
export const LoadChecklistFromUrl = "teachertool.loadchecklistfromurl";
export const SingleEvaluate = "teachertool.singleevaluate";
export const BulkEvaluate = "teachertool.bulkevaluate";
export const RunOnLoad = "teachertool.runonload";
export const AddCriteria = "teachertool.addcriteria";
export const RemoveCriteria = "teachertool.removecriteria";
Expand All @@ -83,6 +88,13 @@ export namespace Ticks {
export const UnhandledEvalError = "teachertool.evaluateerror";
export const FeedbackForm = "teachertool.feedbackform";
export const ParamErrorMissingMessage = "teachertool.paramerrormissingmessage";
export const SetEvalResultOutcome = "teachertool.setevalresultoutcome";
export const SetEvalResultNotes = "teachertool.setevalresultnotes";
export const LoadProjectFromInput = "teachertool.loadproject.frominput";
export const LoadProjectFromUrl = "teachertool.loadproject.fromurl";
export const LoadProjectInvalid = "teachertool.loadproject.invalid";
export const BlockPickerBlockSelected = "teachertool.blockpicker.blockselected";
export const BlockPickerOpened = "teachertool.blockpicker.opened";
}

namespace Misc {
Expand Down
11 changes: 11 additions & 0 deletions teachertool/src/services/makecodeEditorService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import { EditorDriver } from "pxtservices/editorDriver";
import { loadToolboxCategoriesAsync } from "../transforms/loadToolboxCategoriesAsync";
import { stateAndDispatch } from "../state";
import { runEvaluateAsync } from "../transforms/runEvaluateAsync";
import { Ticks } from "../constants";
import { getChecklistHash, getObfuscatedProjectId } from "../utils";

let driver: EditorDriver | undefined;
let highContrast: boolean = false;
Expand Down Expand Up @@ -36,6 +38,15 @@ export function setEditorRef(ref: HTMLIFrameElement | undefined, forceReload: ()
const { runOnLoad, projectMetadata } = state;

if (runOnLoad && !!projectMetadata) {
pxt.tickEvent(Ticks.BulkEvaluate, {
fromUserInteraction: true + "",
runOnLoad: true + "",
criteriaCount: state.checklist.criteria.length,
catalogCriteriaIds: JSON.stringify(state.checklist.criteria.map(c => c.catalogCriteriaId)),
checklistHash: getChecklistHash(state.checklist),
projectId: getObfuscatedProjectId(state.projectMetadata?.id),
});

runEvaluateAsync(true); // cause a switch to checklist tab on run
}

Expand Down
2 changes: 1 addition & 1 deletion teachertool/src/state/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export function isProjectLoaded(state: AppState): boolean {
}

export function isChecklistLoaded(state: AppState): boolean {
return !!(state.checklist.criteria.length || state.checklist.name);
return !!state.checklist.criteria.length;
}

export function getSafeProjectName(state: AppState): string | undefined {
Expand Down
17 changes: 11 additions & 6 deletions teachertool/src/transforms/loadChecklistAsync.ts
Original file line number Diff line number Diff line change
@@ -1,25 +1,30 @@
import { Strings } from "../constants";
import { Strings, Ticks } from "../constants";
import { fetchJsonDocAsync } from "../services/backendRequests";
import { logError } from "../services/loggingService";
import { verifyChecklistIntegrity } from "../state/helpers";
import { Checklist } from "../types/checklist";
import { makeToast } from "../utils";
import { ErrorCode } from "../types/errorCode";
import { getChecklistHash, makeToast } from "../utils";
import { replaceActiveChecklistAsync } from "./replaceActiveChecklistAsync";
import { showToast } from "./showToast";

export async function loadChecklistAsync(checklistUrl: string) {
const json = await fetchJsonDocAsync<Checklist | undefined>(checklistUrl);
const checklist = await fetchJsonDocAsync<Checklist | undefined>(checklistUrl);

if (!json) {
if (!checklist) {
showToast(makeToast("error", Strings.ErrorLoadingChecklistMsg));
return;
}

const { valid } = verifyChecklistIntegrity(json);
const { valid } = verifyChecklistIntegrity(checklist);

if (!valid) {
logError(ErrorCode.invalidPremadeChecklist, { checklistUrl });
showToast(makeToast("error", Strings.ErrorLoadingChecklistMsg));
return;
} else {
pxt.tickEvent(Ticks.LoadChecklistFromUrl, { checklistUrl, checklistHash: getChecklistHash(checklist) });
}

await replaceActiveChecklistAsync(json);
await replaceActiveChecklistAsync(checklist);
}
8 changes: 7 additions & 1 deletion teachertool/src/transforms/mergeEvalResult.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,12 @@ import { setEvalResult } from "./setEvalResult";
import { setUserFeedback } from "./setUserFeedback";

// This will set the outcome and notes for a given criteria instance id, but if the provided value is undefined, it will not change that value.
export function mergeEvalResult(criteriaInstanceId: string, outcome?: EvaluationStatus, notes?: string) {
export function mergeEvalResult(
criteriaInstanceId: string,
isManual: boolean,
outcome?: EvaluationStatus,
notes?: string
) {
const { state: teacherTool, dispatch } = stateAndDispatch();

const newCriteriaEvalResult = { ...teacherTool.evalResults[criteriaInstanceId] };
Expand All @@ -14,6 +19,7 @@ export function mergeEvalResult(criteriaInstanceId: string, outcome?: Evaluation

if (outcome !== undefined) {
newCriteriaEvalResult.result = outcome;
newCriteriaEvalResult.resultIsManual = isManual;
}
if (notes !== undefined) {
if (newCriteriaEvalResult.notes !== notes) {
Expand Down
6 changes: 4 additions & 2 deletions teachertool/src/transforms/runSingleEvaluateAsync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ export async function runSingleEvaluateAsync(criteriaInstanceId: string, fromUse
return resolve(true);
}

setEvalResultOutcome(criteriaInstance.instanceId, EvaluationStatus.InProgress);
setEvalResultOutcome(criteriaInstance.instanceId, EvaluationStatus.InProgress, false);

const loadedValidatorPlans = teacherTool.validatorPlans;
if (!loadedValidatorPlans) {
Expand Down Expand Up @@ -154,11 +154,12 @@ export async function runSingleEvaluateAsync(criteriaInstanceId: string, fromUse
? EvaluationStatus.Pass
: EvaluationStatus.Fail;

mergeEvalResult(criteriaInstance.instanceId, result, planResult.notes);
mergeEvalResult(criteriaInstance.instanceId, false, result, planResult.notes);
return resolve(true); // evaluation completed successfully, so return true (regardless of pass/fail)
} else {
setEvalResult(criteriaInstance.instanceId, {
result: EvaluationStatus.NotStarted,
resultIsManual: false,
error: planResult?.executionErrorMsg ?? Strings.UnexpectedError,
});
setUserFeedback(criteriaInstanceId, undefined);
Expand All @@ -174,6 +175,7 @@ export async function runSingleEvaluateAsync(criteriaInstanceId: string, fromUse
setUserFeedback(criteriaInstanceId, undefined);
setEvalResult(criteriaInstance.instanceId, {
result: EvaluationStatus.NotStarted,
resultIsManual: false,
error: Strings.UnexpectedError,
});
return resolve(false);
Expand Down
Loading

0 comments on commit 1e22ec7

Please sign in to comment.