-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
enroll non-UMich users (iss. #221, #169, #252, #291) #247
Conversation
Very simplistic approach for now. Should be refactored to reduce duplication of code before merging upstream.
…rse-manager-next into 221-enroll-non-umich
Also, temporarily accept results as JSON string. Method based on one for enrolling existing users
Temporarily use JSON results as return value.
…rse-manager-next into 221-enroll-non-umich
…rse-manager-next into 221-enroll-non-umich
`adminApiToken` may end up replacing `adminApiClientId` and `adminApiSecret` altogether.
const invitationConfig = configService.get('invitation', { infer: true }) | ||
const apiURL = invitationConfig.apiURL | ||
this.url = apiURL + cirrusAPIVersion + CirrusAPIEndPoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codacy says this is an issue, but I don't think it is. I believe Codacy doesn't have access to typing from NestJS, so it can't determine the type of apiURL
. Anywho, you can ignore this issue in Codacy. Click on the settings icon dropdown on the issue in the Codacy UI, then click "Ignore Issue".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the advice on ignoring. I didn't try that, but I had tried to change the code pattern config. That didn't work because Codacy said it was using the config file from our repo. I didn't think it'd be wise for me to change that config just to placate Codacy. At least, not without discussing it with you and the team first. That might mask real errors, I'd guess.
|
||
return response.data | ||
} catch (error) { | ||
if (axios.isAxiosError(error) && error.response !== undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar deal here. Codacy doesn't have access to the axios
typing, so I think Codacy believes error
and response
are any
. You can safely ignore the issue in Codacy.
INVITATION_API_URL=https://apps.cirrusidentity.com/console | ||
INVITATION_API_KEY= | ||
INVITATION_API_SECRET= | ||
INVITATION_API_ENTITY_ID= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These may benefit from short comment descriptions.
correct case from @pushyami's code Co-authored-by: Sam Sciolla <35741256+ssciolla@users.noreply.github.com>
Checking for specific error indicators among the long error body returned by Canvas.
As @pushyamig requested, I've done some testing. I've run several, at least ten or more, requests containing 200 semi-randomly generated users. Each time…
These batches of 200 take a range of 8–13 seconds to complete. Batches with more than 200 users gets an HTTP 400 error with the message…
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Releasing comments now. There is a compilation error; @lsloan, you need to update references to cirrusPIEndpoint
. Not seeing any (other) super serious issues. I'll finish the review with testing now.
21. Copy the static API key created in Canvas. The ID is the long number shown | ||
in the "Details" column of the "Developer Keys" page. It usually begins | ||
with "1770~…". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still accurate? I would think you'd do this in Settings, not anything related to Developer Keys.
7. Add to the `CANVAS_ADMIN_API_TOKEN` key the API token copied earlier | ||
(in step 20). | ||
8. Configure invitation API settings. The invitation API used by CCM is | ||
currently based on Cirrus Identity services. Provide values for each |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe for now just link "Cirrus Identity service" to the Cirrus site.
): Promise<Array<{ result: CanvasUserLoginEmail | APIErrorData | false, email: string }>> { | ||
const start = process.hrtime.bigint() | ||
|
||
// Try creating all Canvas users; failure often means user already exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment may no longer be exactly accurate, since we're handling the unique ID error internally and returning false.
@@ -103,6 +134,16 @@ export class APIController { | |||
return enrollmentsResult | |||
} | |||
|
|||
// Do NOT use `InvalidTokenInterceptor` here! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could just remove this, or if you want to have something, maybe say something like // Uses admin token, so InvalidTokenInterceptor omitted
. If you do keep it, please add something to the other endpoint,admin/createExternalUsers
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🏁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lsloan, okay, I've done some testing, and I think we're in good shape. I would give approval, but the compilation error hasn't been fixed. I also think it would be a good improvement to tighten and simplify the CanvasUser
interface(s) as described in my latest comments. I will keep an eye on these and give an approval as soon as these two things are taken care of. Everything else is minor and can be resolved as you see fit. Let's finish this!
…rse-manager-next into 221-enroll-non-umich
…into 221-enroll-non-umich * '221-enroll-non-umich' of https://github.com/lsloan/ccm: check for unique ID error during Canvas user creation (#7) Update ccm_web/server/src/invitation/cirrus-invitation.interfaces.ts
I didn't fix refs after accepting change suggestion in GH PR. I also hadn't pulled from GH, so I didn't see the compile errors this had caused.
I didn't fix refs after accepting change suggestion in GH PR. I also hadn't pulled from GH, so I didn't see the compile errors this had caused. (EDIT: Updated to use Pushyami's correct username, "pushyamig". Apologies to @pushyami. I wish I could easily go back to correct the other commit message, too.)
…into 221-enroll-non-umich * '221-enroll-non-umich' of https://github.com/lsloan/ccm: tl-its-umich-edu#221 - correct case from @pushyami's code
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
@ssciolla wrote in #247 (comment):
Thanks for working that through with me in Zoom. As you went along, I think you ended up with an even better solution than you anticipated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm approving. Thanks @lsloan for being flexible as we changed some of the architecture choices, and for considering my PR requests to your branch. There are some comments and suggestions still pending, but none of them seem serious enough to prevent merging or require additional review from me. Please resolve them as you see fit.
@pushyamig, you may want to take a look, for your general awareness, at some of the logic and error handling changes we made -- I'm thinking particularly of api.service.ts
, canvas.interfaces.ts
and api.utils.ts
.
Thanks for approving the PR, @ssciolla. I'm glad to see this PR near its end. I feel my involvement in CCM has aged me significantly. |
Apparently I'd forgotten to commit this doc change.
Resolves #221.
As well as…
Resolves #169.
Resolves #252.
Resolves #291.
PR review etiquette requests
To my reviewer friends, I humbly request…
To Do
Older to do items (click to expand)
/api/admin/user/:loginId
) function may cause an errorInvalidTokenInterceptor
may need to be removed from this route, and possibly others that use admin token. The user’s token shouldn’t be deleted in this case.@IsIn
decorator for enrollment type in DTOHttpModule
are successful and whetherHttpModule
request should utilizeFormData
.admin/createExternalUsers
.Open new issue for this to be done in the UILayout
component capturing base styling, breadcrumbs, footer, and development tools (#347, #352, #353) #348User
object from function arguments where it is unnecessary