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

Add delete button to allow coordinators to drop sections #443

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

KartavyaSharma
Copy link
Member

@KartavyaSharma KartavyaSharma commented Sep 13, 2023

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>/.

@cypress
Copy link

cypress bot commented Sep 13, 2023

Passing run #235 ↗︎

0 78 0 0 Flakiness 0

Details:

Add delete button to allow coordinators to drop sections
Project: csm_web Commit: c8b1998431
Status: Passed Duration: 03:13 💡
Started: Oct 10, 2023 2:40 AM Ended: Oct 10, 2023 2:43 AM

Review all test suite changes for PR #443 ↗︎

@KartavyaSharma KartavyaSharma added enhancement New feature or request sev5 Low severity - General guidance main-pr Central PR for a project do-not-merge labels Sep 13, 2023
@KartavyaSharma KartavyaSharma marked this pull request as ready for review September 14, 2023 04:29
@KartavyaSharma KartavyaSharma marked this pull request as draft September 14, 2023 04:29
@KartavyaSharma KartavyaSharma changed the title Add delete button for coordinators to drop sections Add delete button to allow coordinators to drop sections Sep 14, 2023
@jacovkim jacovkim marked this pull request as ready for review September 26, 2023 08:32
csm_web/scheduler/views/section.py Outdated Show resolved Hide resolved
csm_web/scheduler/views/section.py Outdated Show resolved Hide resolved
csm_web/scheduler/views/section.py Outdated Show resolved Hide resolved
@edwardneo
Copy link
Contributor

Adding another test for student count blocking section deletion would be good! After that, rebase and squash this all down into one commit.

Copy link
Member

@smartspot2 smartspot2 left a 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";
Copy link
Member

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.

Comment on lines 324 to 351
// 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;
// }
Copy link
Member

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.

Comment on lines 273 to 280
const performDrop = () => {
sectionDropMutation.mutate(undefined, {
onSuccess: () => {
// console.log(sectionId)
setStage(DropSectionStage.DROPPED);
}
});
};
Copy link
Member

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.

csm_web/scheduler/views/section.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request main-pr Central PR for a project sev5 Low severity - General guidance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants