use strings for level ids in levelbuilder

[cforkish]

this builds on #38331 to standardize all level ids to be strings in our javascript code. this PR specifically addresses level ids in levelbuilder.

[jmkulwik]

Looks good to me!

[davidsbailey]

Do you have a link to a passing drone run?

[cforkish]

@davidsbailey is there a way to trigger a drone build of an arbitrary branch? i was planning to wait and focus on drone issues once this gets merged into #38085 since this PR isn't based on staging, but if i could do a drone run on this branch that would be even better!

[davidsbailey]

@davidsbailey is there a way to trigger a drone build of an arbitrary branch? i was planning to wait and focus on drone issues once this gets merged into #38085 since this PR isn't based on staging, but if i could do a drone run on this branch that would be even better!

one trick to get drone to run would be to copy the branch, and then open a new PR for that branch against staging. the other would be to change the base of this PR to the staging branch, then push another commit to it.

[davidsbailey]

Unfortunately, in this situation, it is hard as a reviewer to sign off without seeing the test results. Passing tests implies a whole bunch of things have been figured out, and I plan to look for gaps in our test coverage when reviewing.

[davidsbailey]

i was planning to wait and focus on drone issues once this gets merged into #38085

I didn't realize that you were planning to merge this PR into #38085. I would definitely encourage you to merge the changes to CPLAT code into staging as part of a smaller PR if possible. Giant PRs make me nervous because they will be much more difficult to rollback if something goes wrong, and it shouldn't be hard to make incremental progress on this problem by having different components temporarily use different shapes.

[cforkish]

I would definitely encourage you to merge the changes to CPLAT code into staging as part of a smaller PR if possible.

right on. no reason not to make this its own PR rather than merge it into the code studio one. good call!

[cforkish]

@davidsbailey there's your successful drone run!

[cforkish]

oops i forgot to rebase so this branch only includes the levelbuilder stuff. will do so now.

[davidsbailey]

Thanks for the updates, Charlie! Unfortunately it looks like our end to end tests still pass even when JS errors are occurring. Here are some of the JS errors I am seeing when I load the script edit page locally:

There could be others when editing the lesson page (go to http://localhost-studio.code.org:3000/s/coursea-2021/edit and click ✏️ on one of the lessons). On the lesson edit page, the flow for adding a level to an activity section would also need to be tested manually:

In terms of code changes, you may need to change the code in these places to convert between integer and string:

• code-dot-org/apps/src/lib/levelbuilder/script-editor/scriptEditorRedux.js

  Line 279 in e6cb5c4

  .map(level => ({

• code-dot-org/apps/src/lib/levelbuilder/lesson-editor/activitiesEditorRedux.js

  Line 547 in edbd702

  activitySection.scriptLevels.forEach(scriptLevel => {

• code-dot-org/apps/src/lib/levelbuilder/lesson-editor/activitiesEditorRedux.js

  Line 490 in edbd702

  activitySection.scriptLevels.forEach(scriptLevel => {

Apologies that this means you have to manually test our part of the codebase. If you have ideas for how to get automated test coverage on the types of the data passed between server and client, I'm all ears.

[cforkish]

@davidsbailey it ended up being simpler (and imo a better solution) to convert ids to strings in the for_edit summarize functions. if this looks okay to you i plan to update the rest of the summarize functions in the code studio PR for this work.

[davidsbailey]

I'm not sure I understand this comment -- to me, these lines already look like code studio using strings for IDs. does "code studio" refer to the client code? If so, that would help to clarify.

[cforkish]

the update for all our frontend dashboard code to start using strings is in a separate PR that will be merged after this, so for now we only want ids to be strings in levelbuilder, which is why these to_s calls are in this if for_edit block. in my other PR i move them out of the if for_edit block so that all calls to this function will return strings. does that make sense?

[davidsbailey]

ok, if this comment will be going away soon then this sounds fine. :-) it sounds like you are referring to our frontend / client / js / apps code as "code studio". I had always thought of "code studio" as a synonym for dashboard or studio.code.org, frontend and backend. If I'm still misunderstanding please let me know, but I don't think it affects this review.

[cforkish]

yes, as soon as this PR gets merged i will be working on the followup PR. i referred to the other code as "code studio" because when i took this work over, Jessie told me she was done with all the code studio changes but hadn't handled levelbuilder yet, so i just rolled with that language. :)

[cforkish]

@davidsbailey since we originally were making level ids required as numbers, we decided to do the update to strings as a followup PR. that first PR using numbers was merged yesterday, so i had to add some temporary backward compatibility in the client code in this PR. just wanted to give you a heads up, and make sure you don't have any issues with my latest commit before i merge (once i get the build to pass). can i get a final thumbs up from you?

[davidsbailey]

@cforkish the workaround looks good! apologies, I realize that doing this piecemeal has added a bit of work.

[cforkish]

thanks @davidsbailey! yeah keeping these three PRs compatible with each other hasn't been easy, but i think it was the right approach.