seas-computing / course-planner

SEAS Course Planning Application. JSDoc Docs at: https://seas-computing.github.io/course-planner/
0 stars 1 forks source link

Update course modal to allow same as courses to be set #636

Closed rmainwork closed 1 year ago

rmainwork commented 1 year ago

This PR adds front-end UI functionality to set the "Same As" attribute for a given course. This already existed on the server, but was missing the front-end UI component to correctly set that data. This PR does not gracefully disable/hide the modal dropdown to completely prevent the selection of child courses as a parent. This will be provided by another PR/ticket.

Types of changes

Checklist:

Priority:

Related Issues:

Fixes #629

rmainwork commented 1 year ago

@jonseitz I think this is ready for re-review. I do want to just call out 045427ab though where I had to convert value to unknown. I experimented with variations of typeof, Pick etc. but none of them seemed to do exactly what we wanted so had to resort to casting to unknown instead...which in this case I think is probably fine.

codecov[bot] commented 1 year ago

Codecov Report

Merging #636 (79e9f60) into develop (8454770) will increase coverage by 0.08%. The diff coverage is 94.54%.

@@             Coverage Diff             @@
##           develop     #636      +/-   ##
===========================================
+ Coverage    95.11%   95.20%   +0.08%     
===========================================
  Files          193      194       +1     
  Lines         4487     4505      +18     
  Branches       544      545       +1     
===========================================
+ Hits          4268     4289      +21     
+ Misses         102      100       -2     
+ Partials       117      116       -1     
Impacted Files Coverage Δ
src/client/components/pages/CourseAdmin.tsx 100.00% <ø> (ø)
...ent/components/pages/RoomAdmin/CreateRoomModal.tsx 93.00% <ø> (ø)
...lient/components/pages/RoomAdmin/EditRoomModal.tsx 89.55% <ø> (ø)
src/server/course/course.service.ts 100.00% <ø> (ø)
src/server/course/course.controller.ts 90.38% <40.00%> (+2.62%) :arrow_up:
src/client/classes/Validation.ts 100.00% <100.00%> (ø)
src/client/classes/index.ts 100.00% <100.00%> (ø)
...rc/client/components/pages/Courses/CourseModal.tsx 87.37% <100.00%> (+0.64%) :arrow_up:
...lient/components/pages/Courses/EnrollmentModal.tsx 88.05% <100.00%> (ø)
src/client/components/pages/Courses/NotesModal.tsx 89.09% <100.00%> (ø)
... and 9 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

jonseitz commented 1 year ago

re: the @DoesNotMatch decorator and the type of value, I think leaving it as unknown is fine.

In looking at this a bit more though I realized that the generic DTO type we've been using in our validators is really just being inferred as { id: string }, or "An object with the key you've provided here." So it's not actually checking that the argument is a valid property in the DTO!

That's not ideal, but I don't think it's actually a huge issue -- if you provided a field that doesn't exist it looks like it would just attempt to compare the decorated property to undefined, and end up passing validation (which is technically accurate). We could provide that stricter type-checking by giving an explicit type for DTO, e.g.

  @DoesNotMatch<UpdateCourseDTO>('id')
  public sameAs?: string;

... but that's a little unwieldy and, again, I don't know how valuable it really is. So I'm fine leaving it as it is for now, if you feel the same.

rmainwork commented 1 year ago

Yeah, I'm good with that!