Create helper to build public widget options for Matcher #2156
Conversation
…options And add type definition of the public widget options
…c widget option functions
…rt function to filter out rubric data from widget options for the Matcher widget
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (1f1abe7) and published it to npm. You Example: yarn add @khanacademy/perseus@PR2156If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2156 |
|
Size Change: +951 B (+0.06%) Total Size: 1.48 MB
ℹ️ View Unchanged
|
…unction And update tests
| // @ts-expect-error - TS4104 - The type 'readonly (string | number)[][]' is 'readonly' and cannot be assigned to the mutable type '(string | number)[][]'. | ||
| indexedItems = CoreUtil.shuffle( | ||
| indexedItems, | ||
| // @ts-expect-error - TS2345 - Argument of type 'number | null | undefined' is not assignable to parameter of type 'number | RNG'. |
There was a problem hiding this comment.
This error message was already present in the original expect error line, but I've had to split it across multiple lines here due to formatting.
There was a problem hiding this comment.
Cool! The code looks good in general. I don't fully understand why we need to use the seeded random shuffle when getting the public options, though. It seems like sorting the "right" and "left" arrays would conceal the answer just as well, and would be simpler. EDIT: just saw Jeremy's comment. However, I am concerned that because we don't have the problemNum when shuffling serverside, the serverside shuffle is in general going to result in a different order from the clientside shuffle, so we can't rely on them being the same. Shuffling in both places creates a misleading appearance of consistency between the client and server. Overall, I still think sorting is a simpler (and equally correct) solution.
| left = CoreUtil.shuffle(props.left, rng, /* ensurePermuted */ true); | ||
| } | ||
|
|
||
| const right = CoreUtil.shuffle(props.right, rng, /* ensurePermuted */ true); |
There was a problem hiding this comment.
This ensurePermuted = true actually leaks information about the correct answer. We are revealing that the correct answer is not the permutation sent to the client. A cheater could answer the question with better than chance accuracy by choosing some other permutation at random.
Fun fact: a variant of this type of information leak allowed the Allies to break the Enigma cipher in World War II!
A major weakness of the system, however, was that no letter could be enciphered to itself.
There was a problem hiding this comment.
Hmm, I see that this line was copied from the component. I think we should keep it as-is to avoid changing the UX.
Removes reference to problemNum as the seed
…pects New shuffles result from the new hashed seed. Add expects to make it clear when and which columns should not be equal.
| /* Note(tamara): Brought over from the perseus package packages/perseus/src/util.ts file. | ||
| May be useful to bring other perseus package utilities here. */ |
There was a problem hiding this comment.
The functions here are still used in other widgets as well, specifically categorizer, radio, and sorter, so I figured I should leave them in this file for now.
| expect(publicWidgetOptions.right).not.toEqual(options.right); | ||
| expect(publicWidgetOptions.left).not.toEqual(options.left); |
There was a problem hiding this comment.
I added these expect lines to emphasize what we are looking for in this test. I think these check what we need to check regarding making sure the public widget options don't have the same order as the original ones, but please let me know if there's another test I can add for better coverage!
| orderMatters: boolean; | ||
| }; | ||
|
|
||
| // Note: Previously called cyrb128 |
There was a problem hiding this comment.
@benchristel Should I link to that stack overflow page in the code? Not sure about attribution within code 🤔
| left = CoreUtil.shuffle(data.left, seed, /* ensurePermuted */ true); | ||
| } | ||
|
|
||
| const right = CoreUtil.shuffle(data.right, seed, /* ensurePermuted */ true); |
There was a problem hiding this comment.
@benchristel Is there still a concern with these ensurePermuted arguments leaking data? Is that something we can fix?
Summary:
Adds a function that takes a Matcher widget's full widget options and removes correct order information. Some additional shuffling logic has been added to allow shuffling to occur server-side.
It also adds this function to the widget's widget export and adds a test confirming the function works as expected.
Issue: LEMS-2761
Test plan: