-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add delete button to allow coordinators to drop sections #443
base: master
Are you sure you want to change the base?
Conversation
Passing run #235 ↗︎
Details:
Review all test suite changes for PR #443 ↗︎ |
Adding another test for student count blocking section deletion would be good! After that, rebase and squash this all down into one commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several small things here; most important ones are additional tests, and adding failure cases in the frontend.
There are also some linting warnings in the JS; mostly about unused variables, which should be removed in the code. There is also a lot of commented out code that should be deleted as well.
Lastly, there are still 11 commits here, which should be rebased and squashed down to one commit (or two) before merging (feel free to wait until these reviews are resolved prior to squashing).
@@ -1,20 +1,37 @@ | |||
import React, { useState } from "react"; | |||
import { useSectionStudents } from "../../utils/queries/sections"; | |||
import { Mentor, Spacetime, Student } from "../../utils/types"; | |||
import { Navigate, Route, Routes } from "react-router-dom"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of ESLint warnings here about unused variables; you should delete the unused variables here.
// background-color: #fc4a14; | ||
background-color: #ff7272; | ||
cursor: pointer; | ||
border-radius: 10px; | ||
border: none; | ||
outline: none; | ||
transition: background-color 0.4s; | ||
//box-shadow: 0px 8px 24px rgba(149, 157, 165, 0.5); | ||
box-shadow: 0px 4px 4px rgba(198, 198, 198, 0.25); | ||
} | ||
|
||
.coordinator-delete-link { | ||
display: flex; | ||
text-decoration: none; | ||
color: white; | ||
justify-content: space-between; | ||
align-items: center; | ||
// background: none; | ||
|
||
// border: none; | ||
// padding: 0; | ||
// font: inherit; | ||
// cursor: pointer; | ||
// outline: inherit; | ||
|
||
// :hover { | ||
// background-color: #f15120; | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these styles are unused, they should be deleted, rather than commented out.
const performDrop = () => { | ||
sectionDropMutation.mutate(undefined, { | ||
onSuccess: () => { | ||
// console.log(sectionId) | ||
setStage(DropSectionStage.DROPPED); | ||
} | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a failure case handled here (ex. if there are students in the section). The query hook will also need to be modified to raise this error as well.
Currently, there is no way for a coordinator or the tech team to remove sections after they are no longer valid. Eric has to manually drop individual entries from our database for stale/inactive sections. This PR adds a button and an API endpoint that deletes a section from the database when an HTTP request to
DELETE /api/sections/<id>/
.