[Google Blockly] Unregister context menu items

[mikeharv]

The workspace context menu option "Clean up blocks" calls a method that currently has a bug in which immovable blocks are ignored:

Initial block layout and context menu options	Result when selecting "clean up blocks"

https://codedotorg.slack.com/archives/C03DBDN67B7/p1678288305437829
google/blockly#6889

The Blockly team has agreed that this is a bug they are interested in fixing. This bug affects nearly all Google Blockly levels because of our convention to make when run immovable. Fortunately, we have never broadcast that this option exists; in fact it was only recently discovered by the curriculum team. In the meantime, we can remove the context menu option.

While looking into this, I discovered three other context menu options that I believe should also be unregistered. The options to expand and collapse blocks "work", but not a behavior that we've ever surfaced to users before. Similarly, the option to delete all blocks from the workspace is probably more aggressive than we want. (Users are already trained to click "Start Over" if they want to get back to an initial state for their code.) This last option also produces a confirmation modal dialog that we haven't customized.

Collapsed blocks:

Delete all:

Links

Slack thread: https://codedotorg.slack.com/archives/C03DBDN67B7/p1678288305437829
Blockly bug report: google/blockly#6889

Follow-up work

Communicate with Blockly about the status of this report and consider reregistering the menu option once fixed.

PR Checklist:

• Tests provide adequate coverage
• Privacy and Security impacts have been assessed
• Code is well-commented
• New features are translatable or updates will not break translations
• Relevant documentation has been added or updated
• User impact is well-understood and desirable
• Pull Request is labeled appropriately
• Follow-up work items (including potential tech debt) are tracked and linked

[bencodeorg]

LGTM -- nit on PR description, could you use different titles in the "Before" / "After" screenshots you included that clarify that those screenshots aren't before/after implementing this PR (as I understand it)?

[bencodeorg]

Nit: might make sense organizationally to put these in contextMenu.js and add a function like unregisterContextMenuItems()?

[mikeharv]

Good suggestion. Done!

[bencodeorg]

Is this here to catch if a context menu option isn't in the list? Could we just use optional chaining?

[mikeharv]

It's actually to make sure the wrapper's unit test keeps passing.
I think that we're not really cleaning up its state properly since the same instance of GoogleBlockly is used and fed into initializeBlocklyWrapper multiple times. With the try/catch, the test doesn't crash if we've already unregistered things.
So I guess this is admittedly more of a bandaid than reworking the entire test file. That being said, if core Blockly did ever change their default registry out from under us, this code wouldn't break. I'd suggest we leave it regardless of how soon we refactor the test.

Drone log, if you're interested: https://drone.cdn-code.org/code-dot-org/code-dot-org/26802/1/4

FAILED TESTS:
--
5508 | Google Blockly Wrapper
5509 | ✖ "before each" hook for "getGenerator returns the JS Generator"
5510 | HeadlessChrome 0.0.0 (Linux 0.0.0)
5511 | Error: Menu item with ID "cleanWorkspace" not found.
5512 | at ContextMenuRegistry$module$build$src$core$contextmenu_registry.unregister (webpack://blockly-mooc/./node_modules/blockly/blockly_compressed.js?:11098:41)
5513 | at initializeBlocklyWrapper (webpack://blockly-mooc/./src/blockly/googleBlocklyWrapper.js?:297:56)
5514 | at Context.eval (webpack://blockly-mooc/./test/unit/blockly/googleBlocklyWrapperTest.js?:21:52)

[bencodeorg]

Ah ok, a couple thoughts then -- the last line of this section is confusing then, seems like that's what it's intended to do?

code-dot-org/apps/test/unit/blockly/googleBlocklyWrapperTest.js

Lines 14 to 19 in f3b6111

	afterEach(() => {
	// Reset Blockly for other tests.
	Blockly = cdoBlockly; // eslint-disable-line no-global-assign
	// Reset context menu for other tests.
	GoogleBlockly.ContextMenuRegistry.registry.reset();
	});

If you can't figure out why that isn't doing what we'd expect/don't want to figure it out now, could you catch a more specific error (maybe not, looks generic from the log you shared)? At the very least, a comment noting why we're catching all errors here that more explicitly notes that it is for tests?

[mikeharv]

So all the code in that init method is run multiple times on the same GoogleBlockly instance without that instance ever being reset. Resetting the context menu here had been working because previously we were only registering new options to the context menu, not unregistering existing ones from it. I can look at this again a little later on. Thanks!