tl-its-umich-edu / canvas-course-manager-next

Canvas Course Manager Next: A redesign of the existing CCM application. It extends Canvas features, makes cumbersome features easier to use, and adds new features.
8 stars 9 forks source link

Integrate existing UI with API endpoints for Add Non-UM Users (#254) #363

Closed ssciolla closed 2 years ago

ssciolla commented 2 years ago

This PR integrates the separately developed UI components and API endpoints for the Add Non-UM Users feature. Changes were made to the server to minimize data returned, streamline the response data structure, and align validation with the client. A new approach for describing errors (especially those from bulk processes) is introduced. The PR aims to resolve #254.

To Do List

pushyamig commented 2 years ago

This response from B/E is not comprehended in the FE. This is case when already existing 6 user was uploaded and even set of user are deliberately failed in the BE.

{"statusCode":422,"data":[{"email":"sur1@sur.com","userCreated":{"canvasStatusCode":422,"message":"No response body was found.","failedInput":"sur1+sur.com"}},{"email":"gam1@gam.com","userCreated":false},{"email":"gam2@gam.com","userCreated":{"canvasStatusCode":422,"message":"No response body was found.","failedInput":"gam2+gam.com"}},{"email":"gam6@gam.com","userCreated":false},{"email":"gam3@gam.com","userCreated":{"canvasStatusCode":422,"message":"No response body was found.","failedInput":"gam3+gam.com"}},{"email":"gam4@gam.com","userCreated":false}]}

I would expect that it will display in a nice table instead as below

Screen Shot 2022-03-07 at 4 13 26 PM

pushyamig commented 2 years ago

I have been testing this PR and capturing my notes here

pushyamig commented 2 years ago

Already existing users and few failure in User creation step and enrolling to section. Same results as this https://github.com/tl-its-umich-edu/canvas-course-manager-next/pull/363#issuecomment-1061142776 JSon response from BE

{"statusCode":422,"data":[{"email":"jdoe@exa.edu","userCreated":{"canvasStatusCode":422,"message":"No response body was found.","failedInput":"jdoe+exa.edu"}},{"email":"jdoe1@exa.edu","userCreated":false},{"email":"jdoe2@exa.edu","userCreated":{"canvasStatusCode":422,"message":"No response body was found.","failedInput":"jdoe2+exa.edu"}},{"email":"jdoe3@exa.edu","userCreated":false},{"email":"jdoe4@exa.edu","userCreated":{"canvasStatusCode":422,"message":"No response body was found.","failedInput":"jdoe4+exa.edu"}},{"email":"jdoe5@exa.edu","userCreated":false},{"email":"joe6@exa.edu","userCreated":{"canvasStatusCode":422,"message":"No response body was found.","failedInput":"joe6+exa.edu"}},{"email":"joe7@exa.edu","userCreated":false}]}

I have my notes captured here https://docs.google.com/spreadsheets/d/1gOLsyYj1k3XYm40jzInoMSDtucLLmhtFTzNiidOVZA4/edit#gid=0

pushyamig commented 2 years ago

I know this PR is still WIP, please consider my comments as early feedback so we can catch any issues we can. I will look into code tomorrow.

ssciolla commented 2 years ago

@pushyamig, thanks for the testing! I think I fixed the case you found. The interface and type checking for ExternalUserSuccess needed to be tightened up. Please let me know what you think.

pushyamig commented 2 years ago

In case of existing user enrolling in a section, the confirmation message is as shown below. I think this is working correctly (success msg wording) in the Multiple user workflow for existing user case as shown in screenshot.

  1. Single user flow for existing user -Success msg The new user was added to Canvas, sent an email invitation to choose a login method, and enrolled in the selected section!
  2. Multiple user for existing user - Success msg Screen Shot 2022-03-08 at 12 50 41 PM

I have update my findings here. I marked the above case as failure. But I think you can differentiate if already user exists or not since you don't gather first and last name in this specific case. Just a thought!

pushyamig commented 2 years ago

When I was testing the semi failure during each step, I saw this JSOn response from BE. UserCreated object with be either false or true. Why is extra information is being sent. I know some point you were telling to change the UserCreated to be boolean. This is just a question for clarity.

{
   "statusCode":422,
   "data":[
      {
         "email":"1@one.com",
         "userCreated":{
            "canvasStatusCode":422,
            "message":"No response body was found.",
            "failedInput":"1+one.com"
         }
      },
      {
         "email":"2@one.com",
         "userCreated":false
      },
      {
         "email":"3@one.com",
         "userCreated":{
            "canvasStatusCode":422,
            "message":"No response body was found.",
            "failedInput":"3+one.com"
         }
      },
      {
         "email":"4@one.com",
         "userCreated":false
      },
      {
         "email":"5@one.com",
         "userCreated":{
            "canvasStatusCode":422,
            "message":"No response body was found.",
            "failedInput":"5+one.com"
         }
      },
      {
         "email":"6@one.com",
         "userCreated":false
      },
      {
         "email":"7@one.com",
         "userCreated":{
            "canvasStatusCode":422,
            "message":"No response body was found.",
            "failedInput":"7+one.com"
         }
      },
      {
         "email":"8@one.com",
         "userCreated":false
      }
   ]
}
pushyamig commented 2 years ago

When I was testing the semi failure during each step, I saw this JSOn response from BE. UserCreated object with be either false or true. Why is extra information is being sent. I know some point you were telling to change the UserCreated to be boolean. This is just a question for clarity.

  1. does userCreated = false means Already user existed case?'
  2. Does this mean some thing failed in the process of user creating in BE?
{
  "email":"[7@one.com](mailto:7@one.com)",
  "userCreated": {
    "canvasStatusCode":422,
    "message":"No response body was found.",
    "failedInput":"7+[one.com](http://one.com/)"
  }
}
ssciolla commented 2 years ago
1. does userCreated = false means Already user existed case?'

Yes, userCreated: false means the user was not created because they already exist.

2. Does this mean some thing failed in the process of user creating in BE?

Yes, if it's not a boolean, it will be an error object describing what went wrong during the process.

ssciolla commented 2 years ago

I'm going to continue doing some testing with this, but I think we can get the review process started. This may not be fully optimal, but I think it's working. @lsloan, let me know if you have any feedback on the backend changes, especially.

pushyamig commented 2 years ago

@ssciolla was there any agreement that incase of mix of UserCreated = true/false (already existing user) we are going to show success message as below?

New users have also been added to Canvas and sent an email invitation to choose a login method.

I know that we agreed not to show the list of already existing users.

ssciolla commented 2 years ago

Thanks for the reviews, @pushyamig and @lsloan. This may very well still have some issues, but I'd like to get it in front of testers, so I'm merging. Please open new issues and assign me if something occurs to you.