Skip to content
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

Admin Edit Course Info Page Frontend #884

Merged
merged 10 commits into from
Jul 26, 2023

Conversation

tiingweii-shii
Copy link
Member

Description

Added the Course Info page to the Admin Panel to allow professors (and course coordinators) to update course information (course display name, coordinator email, ical url, and crns).

Please include a summary of the change:

  • added Course Information page to the Admin Panel
  • removed courseId from EditCourseInfoParams, updated the edit_course endpoint to handle CRN deletion.
  • updated the get (course) endpoint to correctly get the crns (was broken before : (

Closes #854

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • This requires a run of yarn install

How Has This Been Tested?

Please describe how you tested this PR (both manually and with tests)
Provide instructions so we can reproduce.

  • Tested manually

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code where needed
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@vercel
Copy link

vercel bot commented Apr 26, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
office-hours ❌ Failed (Inspect) May 12, 2023 4:07pm

@dfarooq610
Copy link
Collaborator

is there any way to increase the size of the CRN tags via some CSS magic? They are a bit small and hard to read

Copy link
Collaborator

@dfarooq610 dfarooq610 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nits about the general layout/style of the form but everything seems present and functional, and the code is pretty clean too!

- moved cancel/save button to the bottom
- increased the size of CRN tags
- rel="noopener noreferrer"
packages/app/components/Settings/CourseInformation.tsx Outdated Show resolved Hide resolved
packages/app/components/Settings/CourseInformation.tsx Outdated Show resolved Hide resolved
packages/app/components/Settings/CourseInformation.tsx Outdated Show resolved Hide resolved
packages/server/src/course/course.service.ts Outdated Show resolved Hide resolved
for (const crn of new Set(coursePatch.crns)) {
const courseCrnMaps = await CourseSectionMappingModel.find({
courseCrnMaps = await CourseSectionMappingModel.find({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my bad for not getting to review the earlier backend PR, and I might be wrong - but I don't think courseCrnMaps has to be requeried once per loop. I think you can maybe just get away with querying it the once at line 143 and using that array throughout. It won't be the most up to date CRNs for that course on each iteration, but I don't think you need it to be up to date each time. You've already made the patched crns into a set so there won't be any conflicts here, and any crns removed from above shouldn't be any reason for conflict either.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This courseCrnMaps here is for any mappings with that crn, which is different from line 143 (mappings for that course), so I think it is still necessary here? Let me know if I misunderstood anything (and sorry for just addressing this after 7 months ∠( ᐛ 」∠)_ )

Thank you for reviewing my PR and leaving so many great comments (。・ω・。) (I have addressed all other ones!)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh yes i think i understand. ive also mostly forgotten what all of this code does but regardless i catch the vibes here

@dfarooq610
Copy link
Collaborator

dfarooq610 commented Jun 2, 2022

can you add validation on the ical input to enforce that it ends in ".ics"? that will fix one step on this issue

#891

Copy link
Collaborator

@IrisLiu-00 IrisLiu-00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM babe sorry this did actually take 50000 years

@dfarooq610 dfarooq610 merged commit cfe4901 into master Jul 26, 2023
@dfarooq610 dfarooq610 deleted the ts-az-edit-course-info-page-frontend branch July 26, 2023 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Course Settings page
3 participants