Fix error handling for getColumn when table does not exist

[cnbrenci]

There was a recent regression in getColumn where there's an uncaught exception when the table does not exist, which results in the student's program halting execution, and no error appearing that points to the problem line in their code. This makes it difficult for students to diagnose the issue if they forget to import the table they're referencing, and is a common occurrence with remixing projects since data tables do not get copied over into the remixed project (by design, but it's still a common gotcha).

I think we should get this in ASAP to mitigate the impact that the regression is causing AP CSP students. I've scoped it down as much as possible to avoid any side-effects, and added test coverage for the code path.

Warning!!

The AP CSP Create Performance Task is in progress. The most critical dates are from April 3 - April 30, 2024. Please consider any risk introduced by this PR that could affect our students taking AP CSP. Code.org students taking AP CSP primarily use App Lab for their Create Task, however a small percent use Game Lab. Carefully consider whether your change has any risk of alterering, changing, or breaking anything in these two labs. Even small changes, such as a different button color, are considered significant during this time period. Reach out to the Student Learning team or Curriculum team for more details.

Links

We've seen a few zendesk tickets lately where users were complaining about their OnEvent callbacks not working, where the culprit seemed to be that they had a getColumn call stopping execution of the script.

slack discussions with links

Testing story

I added some test coverage that was previously missing and would have caught this regression, and tested the success case and error cases for nonexistent table and nonexistent column.

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

[cnbrenci]

null check here avoids exception on null.forEach and passes null back to handleGetColumn here: https://github.com/code-dot-org/code-dot-org/blob/cassi/applab-getcol-onfailure-fix/apps/src/applab/commands.js#L1877-L1878

[thomasoniii]

I think this can just be a map.

const columnValues = records.map(row => row[columnName])

[thomasoniii]

Oh, also just as another note in case anyone looks to this code again...

even if it's not switched to the map, it should still be const columnValues = []. It doesn't need to be a let.

const only prevents you from re-assigning the variable itself (e.g., columnValues = 'foo' later on wouldn't be allowed), but you can still manipulate the object itself. JavaScript is weird. 🤷

[cnbrenci]

I really appreciate this comment and will keep it in mind going forward. Because this is in the critical path for the AP CSP Create task though, I want to keep the change as scoped as possible and only touch the "records is null" fork of the code path. So I'll leave it as is for this PR. I would follow-up and change it after Create task next month, but since this file will be deleted in a couple months anyway, I'm going to skip the overhead. BUT I will remember this fact about const, and how much cleaner map is than the forEach push song and dance! Thank you!!

[thomasoniii]

Again, just a non-mandatory suggestion, but string concatenation and especially with embedded newlines is a little twitchy and easy to have errors (e.g., by forgetting the newline).

I'd suggest a set of lists and a join on "\n"

const csvData = [
  'id,name,age,male',
  '4,alice,7,false',
  '5,bob,8,true',
  '6,charlie,9,true'].join("\n");

Reads a little cleaner to me. We could also take this to an extra level and join the subarrays on commas, but that feels like overkill unless we needed to be dynamic about the record delimeter.

Also, as a side note - backtick quotes do allow embedded newlines, but since this is formatted data I'd lean against using them in this case.

[cnbrenci]

Noted, and there are a lot of things I could clean up in this file but am avoiding that rabbit hole since this code will be going away with the rest of firebase. Thank you!!

[thomasoniii]

two small suggestions about changing how things are written, but nothing required (though the map would be nice to do so we can get rid of the external mutable var)

otherwise, lgtm 🚀

[jmkulwik]

Nice job on the quick fix! And on the added tests too! This will be a big help to our students working on the create task!