Hookup create sections FE/BE #164

chrisrrowland · 2021-07-27T15:16:02Z

Closes #88

ccm_web/client/src/api.ts

pushyamig · 2021-07-28T12:18:25Z

ccm_web/client/src/pages/BulkSectionCreate.tsx

+          <Typography>New sections have been added!</Typography>
+        </CardContent>
+        <CardActions className={successClasses.cardFooter}>
+          See your sections in the Canvas Setting


We need to provide a link to the page setting /courses/:course_id/settings#tab-sections to see if sections has been added or not.

This requires additional functionality that were going to handle in a separate issue. The canvas url needs to be loaded into globals

The globals already is sending the course_id, why can't we make the URL like /courses/403334/setting#tab-sections?

global call response
{"environment":"development","userLoginId":"dddd","course":{"id":403334,"roles":["TeacherEnrollment","Account Admin"]}}

We know the ending of the url but not the environment dependent beginning

relative URL's won't work in this case? just like <a href = '/course/40404/setting#tab-section' or using a link Material UI component?

I'll open a PR to pass the Canvas URL, you can integrate here, or handle it later. Feels related enough and should be straightforward.

pushyamig · 2021-07-28T12:27:18Z

There are some alignment issue with the way file name is displayed

pushyamig · 2021-07-28T12:36:45Z

ccm_web/client/src/api.ts

+// usage:
+//   const data = await delay(nnnn).then(() => {return something})
+// const delay = async (ms: number): Promise<void> => {
+//   await new Promise<void>(resolve => setTimeout(() => resolve(), ms))
+// }


Suggested change

// usage:

// const data = await delay(nnnn).then(() => {return something})

// const delay = async (ms: number): Promise<void> => {

// await new Promise<void>(resolve => setTimeout(() => resolve(), ms))

// }

Are code comments considered poor practice now? I feel like any time I leave commented code it's asked to be removed.

Generally I won't leave code that is not used in the repo, But you can ask other developers what is the best practice here

I guess, for me, I don't see the value of keeping this. Are we going to use it again? Even if we did, you could find it in the commit history.

Are code comments considered poor practice now? I feel like any time I leave commented code it's asked to be removed.

Yeah. It's because they probably need to be removed in the future, but nobody ever remembers to go back and remove them. Or in the future, someone stumbles across it and doesn't understand the code or doesn't know whether should it be removed. So, out of fear of breaking something, it's never touched again.

On the flip side, don't worry about losing code forever when it's deleted. It will still be in the repo history.

Don't they just get minified out in production anyways?

I might remember that once upon a time I left myself a comment somewhere (Narrator: he won't) and look it up in the history, but nobody else is going to know that once upon a time there was a comment that might have been helpful when they are looking at some code for the first time.

tldr; get off my lawn ;-)

ccm_web/client/src/models/canvas.ts

pushyamig · 2021-07-28T13:01:00Z

I found this interesting case when validation error is caught saying sections already occurred by so and so name in course, I opened another window i deleted sections and got back to same CCM window with validation error for create section and tried to upload again but CCM state still thinks the sections loaded already exits. How should we handle this case? which to me not usually a regular workflow that subaccount admins will do ( like delete the sections)

chrisrrowland · 2021-07-28T13:05:48Z

I found this interesting case when validation error is caught saying sections already occurred by so and so name in course, I opened another window i deleted sections and got back to same CCM window with validation error for create section and tried to upload again but CCM state still thinks the sections loaded already exits. How should we handle this case? which to me not usually a regular workflow that subaccount admins will do ( like delete the sections)

Clicking the fix and upload again button could refresh the existing course list.

Probably some other options too

pushyamig · 2021-07-28T15:18:01Z

I found this interesting case when validation error is caught saying sections already occurred by so and so name in course, I opened another window i deleted sections and got back to same CCM window with validation error for create section and tried to upload again but CCM state still thinks the sections loaded already exits. How should we handle this case? which to me not usually a regular workflow that subaccount admins will do ( like delete the sections)

Clicking the fix and upload again button could refresh the existing course list.

Probably clicking the upload button could get the section again? could be easiest thing to do

pushyamig · 2021-07-30T21:21:46Z

@chrisrrowland you can work with other team member for getting approval since I am OOO next week.

Apply context free validation on file upload. Apply validation requiring awareness of server data on submit.

lsloan

I don't have much time to complete this review at this time, but I'll make more time this afternoon. In the meantime, maybe @ssciolla will review and approve. I just have a couple of comments to make at the moment, because I need to take my dog to the vet.

lsloan · 2021-08-03T14:48:45Z

ccm_web/client/src/models/canvas.ts

+export interface CanvasCourseSection {
+  id: number
+  name: string
+  // total_students?: number


If it doesn't interfere with your current code, you can probably uncomment this. If it's not being used, your IDE may give you some warnings about it. If eslint warns about it, I think you can add a linting directive in a comment following the field to tell eslint to ignore it.

It'd be nice if the client Canvas models could use interfaces from the server Canvas models.

It'd be nice if the client Canvas models could use interfaces from the server Canvas models.

💯

Not sure if we ever created an issue for this. I did look into it, hopefully we'll get to it at some point.

lsloan · 2021-08-03T14:54:44Z

ccm_web/client/src/api.ts

+// usage:
+//   const data = await delay(nnnn).then(() => {return something})
+// const delay = async (ms: number): Promise<void> => {
+//   await new Promise<void>(resolve => setTimeout(() => resolve(), ms))
+// }


Are code comments considered poor practice now? I feel like any time I leave commented code it's asked to be removed.

Yeah. It's because they probably need to be removed in the future, but nobody ever remembers to go back and remove them. Or in the future, someone stumbles across it and doesn't understand the code or doesn't know whether should it be removed. So, out of fear of breaking something, it's never touched again.

On the flip side, don't worry about losing code forever when it's deleted. It will still be in the repo history.

ccm_web/client/src/api.ts

ssciolla

Looking good, @chrisrrowland. Lots of small comments. I think there are a couple areas worth addressing -- related to reducing duplication in validation, removing unused stuff, fixing important text, or streamlining a useEffect. I'm going to test and open that settings URL PR now, and I'll approve if I'm satisfied with how it works. Feel free to discuss any of the below with me in the meantime.

Reminder, if I preface something with "Nit-pick", you can consider it optional (or extra credit 😉 ).

ssciolla · 2021-08-02T21:55:08Z

ccm_web/client/src/api.ts

+// usage:
+//   const data = await delay(nnnn).then(() => {return something})
+// const delay = async (ms: number): Promise<void> => {
+//   await new Promise<void>(resolve => setTimeout(() => resolve(), ms))
+// }


I guess, for me, I don't see the value of keeping this. Are we going to use it again? Even if we did, you could find it in the commit history.

ccm_web/client/src/models/canvas.ts

ccm_web/client/src/models/models.ts

ccm_web/client/src/api.ts

ccm_web/client/src/pages/BulkSectionCreate.tsx

ssciolla · 2021-08-03T16:58:54Z

ccm_web/client/src/pages/BulkSectionCreate.tsx

+          <Typography>New sections have been added!</Typography>
+        </CardContent>
+        <CardActions className={successClasses.cardFooter}>
+          See your sections in the Canvas Setting


I'll open a PR to pass the Canvas URL, you can integrate here, or handle it later. Feels related enough and should be straightforward.

ccm_web/client/src/pages/BulkSectionCreate.tsx

ssciolla

I tested and submitted a PR for the settings URL (chrisrrowland#3). I flagged a couple more things around the validation flow and error handling that I'd like to discuss. Generally the code works well and handles most of the cases fluently. I will wait til I've at least heard your opinion about some of my comments, then I'll approve. I hope the detailed review feels at least somewhat productive to you here.

ccm_web/client/src/pages/BulkSectionCreate.tsx

ssciolla · 2021-08-03T17:59:14Z

ccm_web/client/src/pages/BulkSectionCreate.tsx

+    if (getSaveCanvasSectionDataError !== undefined) {
+      const errors = (getSaveCanvasSectionDataError as unknown as IDefaultError).errors
+      const rowInvalidations = convertErrorsToRowInvalidations(errors)
+      setPageState({ state: BulkSectionCreatePageState.CreateSectionsError, schemaInvalidation: [], rowInvalidations: rowInvalidations })


So, right now, errors thrown by the validation layer in the backend have a different format than the Canvas errors. We could modify how these errors are formatted in the backend, or we could add more handling such that 400 "Bad Request" errors expect a ValidationError object. This is beyond the scope of this PR, but right now the UI basically says nothing when you hit this case (for instance, I tried submitting a section name that was too long). I'm wondering if there's some minimal handling we should add in the interim.

Co-authored-by: Mr. Lance E Sloan «UMich» <lsloan-git.luolix.top@umich.edu>

…and/canvas-course-manager-next into 88_get_sections_fe_be

…ataError is defined

Suggested formatting change Co-authored-by: Sam Sciolla <35741256+ssciolla@users.noreply.github.com>

Fix alert text Co-authored-by: Sam Sciolla <35741256+ssciolla@users.noreply.github.com>

ssciolla

Cool, thanks for going through my comments, I'm approving. Feel free to handle anything remaining as you see fit. Questions about validation errors from the backend and the settings link can be handled later (or separately). 🚀

* Send Canvas URL in /api/globals; wire up BulkSectionCreate to use it for settings link * Use Link component; add target _parent

chrisrrowland added 3 commits July 20, 2021 15:20

Wire up get course sections w/ create sections FE

44f3d51

Wire up adding sections

04bcb1a

Handle error display

64071b7

chrisrrowland requested a review from pushyamig July 27, 2021 15:16

pushyamig reviewed Jul 28, 2021

View reviewed changes

ccm_web/client/src/api.ts Show resolved Hide resolved

pushyamig reviewed Jul 28, 2021

View reviewed changes

ccm_web/client/src/models/canvas.ts Outdated Show resolved Hide resolved

chrisrrowland added 2 commits July 28, 2021 11:06

File name alignment fix

76a1419

File name alignment fix

0af6590

pushyamig self-requested a review July 28, 2021 16:18

Refactor flow.

69c4ecd

Apply context free validation on file upload. Apply validation requiring awareness of server data on submit.

chrisrrowland requested a review from ssciolla August 2, 2021 21:17

lsloan self-requested a review August 3, 2021 01:25

lsloan reviewed Aug 3, 2021

View reviewed changes

ssciolla reviewed Aug 3, 2021

View reviewed changes

ccm_web/client/src/pages/BulkSectionCreate.tsx Outdated Show resolved Hide resolved

ssciolla mentioned this pull request Aug 3, 2021

Add Canvas settings URL chrisrrowland/canvas-course-manager-next#3

Merged

ssciolla reviewed Aug 3, 2021

View reviewed changes

chrisrrowland and others added 6 commits August 4, 2021 09:54

Uncoment a thus far unused property

4dd1d9e

Update ccm_web/client/src/api.ts

586ffdb

Co-authored-by: Mr. Lance E Sloan «UMich» <lsloan-git.luolix.top@umich.edu>

Merge branch '88_get_sections_fe_be' of https://github.com/chrisrrowl…

477e551

…and/canvas-course-manager-next into 88_get_sections_fe_be

Non functional tweaking of getPost getPut

dfdbbf0

Delete the forbidden comment

c551ff2

Reorder some imports ~asthetics~

75c9da5

chrisrrowland and others added 10 commits August 4, 2021 11:36

Remove an unused css style

2ab72f6

rename a variable

6df77d6

Only change to LoadingExistingSectionNamesFailed if getCanvasSectionD…

f03449b

…ataError is defined

Prevent multiple submits

d951d16

Update ccm_web/client/src/pages/BulkSectionCreate.tsx

6a60c85

Suggested formatting change Co-authored-by: Sam Sciolla <35741256+ssciolla@users.noreply.github.com>

Update ccm_web/client/src/pages/BulkSectionCreate.tsx

5ef8b47

Suggested formatting change Co-authored-by: Sam Sciolla <35741256+ssciolla@users.noreply.github.com>

Update ccm_web/client/src/pages/BulkSectionCreate.tsx

4503bbb

Fix alert text Co-authored-by: Sam Sciolla <35741256+ssciolla@users.noreply.github.com>

Escape quotes in text

4bd2796

Learning to spell basic words

e70ddfb

Get rid of redundant code. Need to get better at javascript

27b081d

ssciolla approved these changes Aug 5, 2021

View reviewed changes

Add Canvas settings URL (#3)

06dfdfd

* Send Canvas URL in /api/globals; wire up BulkSectionCreate to use it for settings link * Use Link component; add target _parent

chrisrrowland merged commit f0b7440 into tl-its-umich-edu:main Aug 5, 2021

chrisrrowland deleted the 88_get_sections_fe_be branch August 5, 2021 15:53

chrisrrowland mentioned this pull request Aug 27, 2021

107 bulk enroll um user #177

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hookup create sections FE/BE #164

Hookup create sections FE/BE #164

chrisrrowland commented Jul 27, 2021

pushyamig Jul 28, 2021

chrisrrowland Jul 28, 2021

pushyamig Jul 28, 2021

chrisrrowland Jul 28, 2021

pushyamig Jul 28, 2021

ssciolla Aug 3, 2021

pushyamig commented Jul 28, 2021

pushyamig Jul 28, 2021

chrisrrowland Jul 28, 2021

pushyamig Jul 28, 2021

ssciolla Aug 2, 2021

lsloan Aug 3, 2021

chrisrrowland Aug 4, 2021

pushyamig commented Jul 28, 2021

chrisrrowland commented Jul 28, 2021

pushyamig commented Jul 28, 2021 •

edited

Loading

pushyamig commented Jul 30, 2021

lsloan left a comment

lsloan Aug 3, 2021

chrisrrowland Aug 4, 2021

ssciolla Aug 5, 2021

lsloan Aug 3, 2021

ssciolla left a comment •

edited

Loading

ssciolla Aug 2, 2021

ssciolla Aug 3, 2021

ssciolla left a comment

ssciolla Aug 3, 2021

ssciolla left a comment

Hookup create sections FE/BE #164

Hookup create sections FE/BE #164

Conversation

chrisrrowland commented Jul 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pushyamig commented Jul 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pushyamig commented Jul 28, 2021

chrisrrowland commented Jul 28, 2021

pushyamig commented Jul 28, 2021 • edited Loading

pushyamig commented Jul 30, 2021

lsloan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssciolla left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssciolla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssciolla left a comment

Choose a reason for hiding this comment

pushyamig commented Jul 28, 2021 •

edited

Loading

ssciolla left a comment •

edited

Loading