107 bulk enroll um user #177
Conversation
some AddUMUsers layout
Disable submit while processing.
…wland/canvas-course-manager-next into 107_bulk_enroll_um_user
validate uniqnames and roles
to login id per requirement change no longer applying uniqname regex
| const isValidLoginID = (loginID: string): boolean => { | ||
| // return uniqname.match(UNIQNAME_REGEX) !== null | ||
| // Don't apply any validation here, just pass anything through | ||
| // Flag this function for deletion in 3.. 2.. 1... | ||
| return true | ||
| } |
There was a problem hiding this comment.
Should this code be removed? since it just returning true
There was a problem hiding this comment.
I'm sure the reviewers will vote yes. I left it there b/c it seemed reasonable that we might want to do some validation of the text.. I had been applying the aforementioned uniqname regex. Maybe there is nothing to do here ever.
There was a problem hiding this comment.
Is there any value that is absolutely 100% always illegal every single time in the client?
There was a problem hiding this comment.
I think this should be removed if we are not validating anything. We can added it back if it is needed in future since all the validation is happening in BE
I'm sure I'm doing something incorrect CSS wise here. I'm looking into it but its not obvious to me. Addressed it. Not sure if it was the right fix but it works. |
| } | ||
| } | ||
|
|
||
| export class CanvaCoursesSectionNameValidator { |
There was a problem hiding this comment.
@chrisrrowland This is nice addition, that you are checking if the section name you created already exist or not. I did not think about this use case.
There was a problem hiding this comment.
And I misspelled it 🤦♂️
If there are other client side validations to be done they should be easy to add.
|
I think the pop should suggest a sample section name , but currently it is repeat of what show on label. That doesn't help. below is the suggested from wireframe |
|
@chrisrrowland Nice work, this is lot of code and I like the way you modularized the code with create section widget and select section widget and that made the AddUMUser file not too big. |
| import ValidationErrorTable, { ValidationError } from '../components/ValidationErrorTable' | ||
|
|
||
| const USER_ROLE_TEXT = 'Role' | ||
| const USER_ID_TEXT = 'Login ID' |
There was a problem hiding this comment.
These 2 constants can go inside AddUMUsers function since these are not used anywhere else other than in the AddUMUsers
| const [file, setFile] = useState<File|undefined>(undefined) | ||
| const [enrollments, setEnrollments] = useState<IAddUMUserEnrollment[]|undefined>(undefined) | ||
| const [errors, setErrors] = useState<ValidationError[]|undefined>(undefined) | ||
| const [isAddingEnrollments, setIsAddingEnrollments] = useState<boolean>(false) |
There was a problem hiding this comment.
I see red wiggle line, this will probably go away when we start integrating UI with BE. should we leave this for now for temporarily disable it? I don't know what have been followed in the past.
There was a problem hiding this comment.
Do you want to disable the wiggle line until the FE/BE integrate or you want to leave it like that?
| } | ||
| }).catch(e => { | ||
| // TODO Not sure how to produce this error in real life | ||
| console.log('error parsing file') |
There was a problem hiding this comment.
Should this case be handled in case of error? I see that you did handled it in bulk create section case https://github.com/tl-its-umich-edu/canvas-course-manager-next/blob/main/ccm_web/client/src/pages/BulkSectionCreate.tsx#L337
There was a problem hiding this comment.
@chrisrrowland in case of parsing error i feel this should be handled and give a valid message to user. you are already doing that with Bulk create sections case
| const isValidLoginID = (loginID: string): boolean => { | ||
| // return uniqname.match(UNIQNAME_REGEX) !== null | ||
| // Don't apply any validation here, just pass anything through | ||
| // Flag this function for deletion in 3.. 2.. 1... | ||
| return true | ||
| } |
There was a problem hiding this comment.
I think this should be removed if we are not validating anything. We can added it back if it is needed in future since all the validation is happening in BE
| ( | ||
| <div className={classes.swaggerLink}> | ||
| <Link href={`/swagger?csrfToken=${String(getCSRFToken())}`}>Swagger UI</Link> | ||
| </div> |
There was a problem hiding this comment.
The swagger link is looking better now. Thanks for the fix!
|
Closes #107
Depends on #108 for completion.
UX for adding UM users, complete up until actually submitting to the backend.
Includes new single section creation component and a section selection component that supports single and multi select. Single select is used in this feature.