Add Code Review groups client-side API for updating groups and setting enabled status #43467
Conversation
| @@ -11,7 +11,7 @@ export default function CodeReviewGroupsLoader({sectionId}) { | |||
| const api = new CodeReviewGroupsDataApi(sectionId); | |||
| api | |||
| .getCodeReviewGroups() | |||
| .then(groups => onLoadSuccess([convertGroupData(groups)])) | |||
| .then(groups => onLoadSuccess([groups])) | |||
There was a problem hiding this comment.
For my understanding, why do we need brackets around groups here? Isn't it already an array?
There was a problem hiding this comment.
Yep that's because onLoadSuccess needs to be called with an array of parameters. This array of parameters will be passed to the render function as a set of arguments. For this function, there's only one parameter (the array groups) so it's a single member in an array.
| @@ -8,6 +8,48 @@ export default class CodeReviewGroupsDataApi { | |||
| getCodeReviewGroups() { | |||
| return $.getJSON( | |||
There was a problem hiding this comment.
Not in this PR, but I think the more common pattern I see is to JSON.parse the response. Is there a difference? Not sure it matters, mostly curious
There was a problem hiding this comment.
Hmm, yeah I saw this in a few other places and figured it might be more streamlined? Looks like getJSON is just shorthand for $.ajax({dataType: 'json', url: ... }). But yeah, not sure what our preference is - I do also see plenty of $.ajax(...) as you mentioned.
| ] | ||
| # Parameters are represented as hashes | ||
| new_groups = { | ||
| '0' => {name: group_1_name, members: {'0' => {follower_id: followers[0].id}}}, |
There was a problem hiding this comment.
This feels unusual to me...I think using jQuery's post isn't super common for us either (only seeing a handful of uses in our code base) -- is there a reason to not use something like this? Personal preference?
There was a problem hiding this comment.
I believe this is due to how Rails's action controller parameters are represented. The data that is POST-ed is still a JSON object with arrays, i.e.
{
groups: [
{ id: 1, members: [...] },
{ id: 2, members: [...] },
...
]
}
But from what I was seeing, in Rails, the parameters were converted to hashes, with indices being treated as keys, i.e.
{
groups: {
0: { id: 1, members: { 0: {...}, 1: {...} },
1: { id: 2, members: {...} },
...
}
}
Let me double check if the jQuery method makes a difference though. AFAIK, $.post is just shorthand for $.ajax({ type: "POST", url: ... })
There was a problem hiding this comment.
Maybe something like this? I do see content type application/json often:
There was a problem hiding this comment.
Edit: didn't see your second comment before posting this - yep exactly, I think that's the culprit.
Actually looking online, you're right :) I think because contentType: application/json isn't being set, it's being converted to this weird hash format. Let me update this and confirm.
| it('makes a GET request and converts group data when calling getCodeReviewGroups', () => { | ||
| let convertedResponse; | ||
|
|
||
| getJSON.returns({ |
This adds the remaining client API calls (set code review groups, set enabled status) for code review groups plus some refactoring to put all the data conversion logic in once place. I also noticed while testing that rails turns JSON params with lists into hashes, so I modified the controller a bit accordingly.
Links
https://codedotorg.atlassian.net/browse/CSA-948
Testing story
Local (all the things) + unit.
Deployment strategy
Follow-up work
Privacy
Security
Caching
PR Checklist: