[Applab] Clear Firebase data on puzzle reset

[ajpal]

Description

Currently, the "start over" button in app lab is a little bit of a lie, because assets and data are not versioned. There are ongoing questions about what versioning would look like for these, but for now, we'd at least like to move in the direction of the "start over" button bringing you back to a clean slate. This PR handles clearing the firebase channel when the puzzle resets.

Note: Intermediate versions are unaffected by this change. This is only for clicking the "Start Over" button.

Scenarios tested:
Widget: s/csd1-pilot/stage/3/puzzle/2

• partially complete the puzzle, reload, your found words are still there
• partially complete the puzzle, click start over, you go back to a blank slate
• fully complete the puzzle, click finish, go back, puzzle is still solved

Level with json-specified data: s/csppostap-2019/stage/10/puzzle/2

• edit the data in the table, reload, your edits are still there
• delete myTable, reload. myTable reappears. This matches current behavior. I could see us wanting this behavior to change, but for now, since this matches current behavior, I think it's okay.
• add a new table or k/v pair, reload, your data is still there
• edit some data, revert to a previous version (but not the first), your data is still there
• edit some data, click "Start Over", myTable is there with the level-specified data, and no other tables or k/v pairs are there.

Freeplay level: /projects/applab/new

• add some data, reload, your data is there
• add some data (v1), reload, edit the data (v2), reload, edit the data again (v3). Revert to v2, the data is still in the state it was in v3.
• Now click start over, the data tab is empty.

cc @davidsbailey for Firebase rules change to enable deleting a channel from Firebase.

Reviewer Checklist:

• Tests provide adequate coverage
• 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

[davidsbailey]

Hi Anjali, thanks for looping me in. I do have a concern about the approach from a security perspective, but it should be pretty straight forward to work around.

One other top-level comment is that this could really use an end-to-end test, which for us would be a UI test.

There are many ways you could do this, but because it is a pain to write code using UI test steps, one strategy we've used in the past is just to create an applab level which already has the code or design mode content so that all our UI test needs to do is click buttons to run it. Here is one example:

code-dot-org/dashboard/test/ui/features/applab/data_blocks.feature

Lines 5 to 8 in 47cd42a

	Scenario: Evaluate Data Blocks
	# This level evaluates the create/read/update/deleteRecord and set/getKeyValue blocks
	# when run, and prints success if the data storage APIs are working properly.
	Given I am on "http://studio.code.org/s/allthethings/stage/18/puzzle/8?noautoplay=true"

In this case, you could perhaps write a level with initial code that lets you create / print / delete firebase data by clicking the buttons, and then have your UI test click runButton, click those buttons, clear the puzzle, etc.

maybe you know this already, but one way to add a level is to run your local server in levelbuilder mode, copy an existing level, use the levelbuilder UI to edit the level, then add it to the script at /s/allthethings/edit.

Please let me know if I can help more with this.

[davidsbailey]

this line is concerning because it breaks the strategy outlined above for ensuring there are no unsecured nodes in our firebase:

// 1. a node with a write() rule must not have an ancestor with a write() rule.

Rather than changing firebase rules to allow this or rethinking this entire strategy, I think it will be easier to revert this change to the rules and just delete the nodes you need to:

/v3/channels/{channelId}/current_tables
/v3/channels/{channelId}/metadata
/v3/channels/{channelId}/storage

It seems ok not to erase /v3/channels/{channelId}/counters, which might be difficult given the rules on those.

In the future, if you ever want to delete an entire channel such as when we delete a user's account, you could do that from the server, which these security rules won't apply to.

[ajpal]

Unfortunately I think we need to erase /v3/channels/{channelId}/counters. That node is used as the source of truth for the list of tables in the project.
Without deleting /counters, if I make a project with a table mytable, then start over, I will still see mytable in the list

[davidsbailey]

can you use /v3/channels/{channelId}/current_tables as the source of truth for what tables exist instead?

it seems fine to erase counters if you need to -- I just wasn't quite sure if the existing security rules would let you.

If doing this on the client gets to be too complicated, one alternate approach is to send a request to the server and then make a REST API request to delete the entire channel via firebase_helper.rb.

[davidsbailey]

I see that you manually verified that initial table data is getting repopulated after you start over. How is that still working, given that you're removing this call to populateTable?

[ajpal]

Data is still getting repopulated because of this: https://github.com/code-dot-org/code-dot-org/blob/staging/apps/src/applab/applab.js#L612

Under the old model, data didn't get deleted when you start over, but for levels with json-specified tables, we wanted to make sure the user was back to the starting state of the level, which is why we needed the call to populateTable with overwrite=true. Now, though, we clean up the data when you start over the level, so when we call populateTable with overwrite=false on page load, you get a fresh copy of the table.

[davidsbailey]

Ahh, nice catch and nice simplification! In that case, I would suggest taking this opportunity to remove the now-unused overwrite option too.

[davidsbailey]

nice work! looks like it was not as hard as I suggested to add code via UI test steps. :-) I am wondering though, won't a #keyValueLabel containing success already be in the DOM even without this the "add code" line, since the start code will be run again? or does the "add code" line overwrite that code? Just looking for a sanity check that this test will actually fail if the key-value pairs are not reset.

[davidsbailey]

Great work @ajpal ! Thanks for the follow-up. OK to merge after confirming that the UI test will fail if the key value pairs are not reset.

[jmkulwik]

This is awesome. Huge thanks for making this change :)

[ajpal]

This UI test is passing for me locally, but consistently failing on drone. Removing it for now so I can get this change and #31857 in by the end of this week. I'm working on fixing the UI test in another branch now.