Remove Role enum & add update role functionality #4051
Conversation
| String avatarUrl = json.optString("avatar_URL"); | ||
| // We don't support multiple roles, so the first role is picked just as it's in Calypso | ||
| Role role = Role.fromKey(json.optJSONArray("roles").optString(0)); | ||
| String role = json.optJSONArray("roles").optString(0); |
There was a problem hiding this comment.
I think I missed this like in the previous PR: the .optString() call here can throw a NPE so it's better to use json.getJSONArray (instead of json.optJSONArray). This way, the potential JSONException will get caught below.
| String role = json.getJSONArray("roles").optString(0); | ||
|
|
||
| return new Person(personID, localTableBlogId, username, firstName, lastName, displayName, avatarUrl, role); | ||
| return new Person(personID, siteID, localTableBlogId, username, firstName, lastName, displayName, avatarUrl, role); |
There was a problem hiding this comment.
Let's wrap this line to 120chars. Cheers!
While at it, there's a comment line a few lines above that needs wrapping as well.
|
Just finished my pass in reviewing this PR @oguzkocer , thanks for submitting it! Let me know if it needs another pass. Cheers! |
|
@hypest I've fixed the issues that came up. Should be ready for another look. Thanks! |
|
@oguzkocer , thanks for the changes and kudos for going the extra mile and change the variables along with the table column name for LGTM! |
Relevant #3901. This PR removes the Role enum and adds an array resource for the possible roles in WordPress.com. I've decided to make this change because we don't really benefit from having an enum as we are only showing the Role's label in UI and don't use it for anything else. We should use the
/v1.1/sites/$site/rolesendpoint to retrieve the possible roles, but I think this is better for the v1. I've already opened an issue for it #4050.I've also added the
updateRolehelper method inPeopleUtilsand I know it's not used here. I am working on the UI changes infeature/people-management-uiand I'll immediately use this method there. I thought it made more sense to have it in this PR then in the UI one. I've tested the method and confirmed that it's working, but I recommend checking the flow on the UI PR once it's done. Let's just make sure we handle the call correctly code-wise. Since this is not getting merged intodevelopI think it's a fine approach.I've also decided to save the
siteIDproperty forPersonmodel as it's needed to update the user's role. Please note that, thesite_idreturned from the/v1.1/sites/$site/usersendpoint doesn't return the current site's id, so I decided to save the localsiteIDwe use to make that call to save for the Person object.Please note there is a migration change and you might need to uninstall the app before installing this. I think it's best to avoid extra migrations if the code never makes it to
developTo test: There are no testing steps for this PR as they'll be available in the UI PR.
/cc @astralbodies
Needs review: @hypest