Changes for superset bulk user create in Uganda #1
Conversation
src/index.ts
Outdated
|
|
||
| const DASHBOARD_VIEWER_ROLE_ID = 9; | ||
| const DASHBOARD_VIEWER_ROLE_ID = 7; |
There was a problem hiding this comment.
We have to get this ID from superset dynamically
KicaRonaldOkello
force-pushed
the
superset-bulk-user-creation-uganda
branch
from
3dfc136
to
a702cff
Compare
| SUPERSET_PASSWORD = "admin" | ||
| SUPERSET_BASE_URL = "http://localhost:8088" | ||
| SUPERSET_API_PATH = "/api/v1" | ||
| CHA_TABLES = [1,2,3] |
There was a problem hiding this comment.
We have to load the datasets dynamically and also be able to update the rowlevel security with the newly added datasets
There was a problem hiding this comment.
This can safely be ignored.
There was a problem hiding this comment.
Looks great overall. I'm keen about running tests of the readAndParse function where some of the method calls fail. Are the errors being handled gracefully or do they run optimistically, expecting the previous method to have completed successfully?
We could explore chaining then method calls and handling the errors in one place.
| SUPERSET_PASSWORD = "admin" | ||
| SUPERSET_BASE_URL = "http://localhost:8088" | ||
| SUPERSET_API_PATH = "/api/v1" | ||
| CHA_TABLES = [1,2,3] |
There was a problem hiding this comment.
This can safely be ignored.
src/utils/rowlevelsecurity.ts
Outdated
|
|
||
| export const getAvailableRowlevelSecurityFromSuperset = async ( | ||
| authorizationHeaders: any, | ||
| ): Promise<any> => { |
There was a problem hiding this comment.
Since we're using Typescript, let's utilize it's full capability. Define the return type to make your code more predictable.
src/utils/role.ts
Outdated
| `/security/roles/`, | ||
| JSON.stringify(role), | ||
| ); | ||
| return { id: createdRole.id, name: createdRole.result.name }; |
There was a problem hiding this comment.
Used defined objects instead of destructuring. It'll be easier to identify errors. Also this method assumes a success scenario. The postRequest function checks to see if the request fails but doesn't handle the error.
src/utils/permissions.ts
Outdated
|
|
||
| if (!dasboardViewerRole) { | ||
| // Number got directly from Superset, could change | ||
| dasboardViewerRole = { id: 7, name: DASHBOARD_VIEWER }; |
There was a problem hiding this comment.
Let's handle the scenario of when the user does not exist and return that as an output or error to the user. This can have unintended consequences if ones don't know that this was hardcoded here.
src/index.ts
Outdated
| console.log(`Processed ${users.length} successfully`); | ||
|
|
||
| users.forEach(async (user) => { | ||
| let userRole: { id: number; name: string }; |
There was a problem hiding this comment.
The interface SupersetRole is already defined. Let's use that to improve readability.
src/index.ts
Outdated
|
|
||
| const generatedRole = generateRole(user.role, user.place); | ||
|
|
||
| const roleExists = rolesAvailableOnSuperset.find( |
There was a problem hiding this comment.
The const name roleExists implies a boolean. Perhaps rename it to something more descriptive.
|
Please consider extracting the implementation of the |
This PR makes changes for superset bulk user creation for MoH Uganda dashboards