Code Review: move data API into class

[bencodeorg]

Moves standalone functions used to make requests related to code review into a class, which allows us to share re-used data (script ID, level ID, channel ID) across multiple requests.

Testing story

Tested manually after moving each function into the class that I was able to appropriately make the appropriate request. We also have unit testing in a lot of these cases.

[jmkulwik]

Nice work! Using the class is an elegant solution.

[sanchitmalhotra126]

Nice! This looks a lot cleaner. Like I mentioned on the other PR, it would be nice if we could set the token in its own step somehow so it isn't coupled with fetching comments specifically.

That being said, we also discussed possibly adding a route to the backend that would bundle all the initial code review data into one call (comments, review status, enabled status, etc) - in which case I think tacking on the token to that payload makes sense since it's meant to be an initialization request. In any case, we can deal with that later down the line - this refactor is great for now.

Also, now that this is in a class, can we start adding some tests? Could be as a follow-up as it looks like this may still get refactored a bit more, but we should add them at some point.

[bencodeorg]

That being said, we also discussed possibly adding a route to the backend that would bundle all the initial code review data into one call (comments, review status, enabled status, etc) - in which case I think tacking on the token to that payload makes sense since it's meant to be an initialization request. In any case, we can deal with that later down the line - this refactor is great for now.

I feel like having a single method in the JS API that would make all three calls and assemble initial state would be nice, but I don't necessarily see the benefit of adding a single Rails route to do all of the initial assembly. Feels more fragile to bundle all of them vs. making each call separately, so if one failed we could, say, render existing comments but not show the "ready for review" checkbox?

Also, now that this is in a class, can we start adding some tests? Could be as a follow-up as it looks like this may still get refactored a bit more, but we should add them at some point.

New to including data API classes in our Javascript code -- what would tests look like here? We'd still stub the actual requests, right? Just confirming that a stubbed call can be made once a dataApi object has been created?

[jmkulwik]

RE Tests: Here's a sample of similar tests https://github.com/code-dot-org/code-dot-org/blob/staging/apps/test/unit/code-studio/components/libraries/LibraryClientApiTest.js

[jmkulwik]

I feel like having a single method in the JS API that would make all three calls and assemble initial state would be nice, but I don't necessarily see the benefit of adding a single Rails route to do all of the initial assembly. Feels more fragile to bundle all of them vs. making each call separately, so if one failed we could, say, render existing comments but not show the "ready for review" checkbox?

Hmm I take back my earlier suggestion that we do this all as a single Rails route. I was thinking the requests were dependent on each other & sequential rather than independent & parallel.

That being said, I'm not sure that the experience is better if we display only the successful pieces. For example, what would the experience be if the checkbox showed up, but we didn't display the comments due to a failed request? It feels a little odd that we display a partial version of the feature. I think as a user I'd prefer to see either everything or see nothing but an error.

I don't have a firmly formed opinion on either of these and am curious to hear other peoples' thoughts too.

[sanchitmalhotra126]

Re: tests - yep, thanks @jmkulwik that's a good example. I imagine the tests will be fairly simple for now but at least setting them up will allow us to test more functionality later. And it'll allow us to verify some basic cases (ex. making sure every request checks if the token exists).

[sanchitmalhotra126]

And yes, agreed that a single rails route may not be the best fit here since these are pretty independent calls. Regarding loading some of the UI if only some of the calls are successful, I'd lean towards only loading the component if all the calls succeed, and hiding everything if one of the calls fail. The Promise.all in componentDidMount does a little bit of this by only showing the UI once all the calls have finished, so I think it makes sense that we can extend this behavior and only show the UI if all the calls have succeeded.