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

Enroll users to multiple sections with one CSV (#278) #301

Closed ssciolla closed 2 years ago

ssciolla commented 2 years ago

The PR aims to resolve #278.

To Do List

ssciolla commented 2 years ago

@pushyamig, I believe this is functional, so if you'd like to try it and give me feedback, you are welcome to. Keep in mind I did this all rather quickly, so there may be bugs, and I still plan to consider some significant refactoring of AddUMUsers.tsx.

ssciolla commented 2 years ago

Okay, I think this is ready for review. I will tidy up my testing files now and upload them to Google Drive. @lsloan, I think @pushyamig can take the lead here, but you may want to take a look, especially at the server changes.

Note that I have flagged some refactoring opportunities (and may flag more) in comments. These can be opened as separate issues if people agree it would be worthwhile.

pushyamig commented 2 years ago

Screen Shot 2022-01-27 at 3 17 21 PM

For the multiple sections use-case should we have 2 downloads? will it be simple for user they download a single file with all the header that is needed for user and they can delete the section name column or while parsing the file the code won't look at the section_name column at all( i think this will work with the way it is coded)

@janelilr @ssciolla Thoughts??

Note: Not asking for re-work, just thinking it loud here

ssciolla commented 2 years ago

For the multiple sections use-case should we have 2 downloads? will it be simple for user they download a single file with all the header that is needed for user and they can delete the section name column or while parsing the file the code won't look at the section_name column at all( i think this will work with the way it is coded)

I think it would be confusing to do it the way you are suggesting -- too many steps involved, between removing a column and then possibly rows if they don't want to use all sections. That's my view.

janelilr commented 2 years ago

For the multiple sections use-case should we have 2 downloads? will it be simple for user they download a single file with all the header that is needed for user and they can delete the section name column or while parsing the file the code won't look at the section_name column at all( i think this will work with the way it is coded)

I would suggest keeping the downloads separate. Since the "Download an example" relates to that particular feature, it is consistent with the other features that offer an example. The other download is specific to canvas section IDs and is clear based on the download label.

pushyamig commented 2 years ago

I would suggest keeping the downloads separate. Since the "Download an example" relates to that particular feature, it is consistent with the other features that offer an example. The other download is specific to canvas section IDs and is clear based on the download label.

I think it would be confusing to do it the way you are suggesting -- too many steps involved, between removing a column and then possibly rows if they don't want to use all sections. That's my view.

OK

pushyamig commented 2 years ago

Over all this PR functionally looking good. I have few suggestions mostly removing the redundant section check in the BE if that issue is resolved then this PR is ready to merge.

ssciolla commented 2 years ago

Thanks, @pushyamig, I'm gonna merge this so we can get folks to start testing.