senior-knights / course-schedulizer

📝 Create semester schedules without stress
https://senior-knights.github.io/course-schedulizer/
MIT License
10 stars 4 forks source link

First & Second half courses are incorrectly flagged as conflicting #239

Closed kvlinden closed 1 year ago

kvlinden commented 2 years ago

Two courses that meet at the same time in the first and the second half of the semester are marked as being in conflict. Such course pairs are not in conflict.

E.g., DATA 102 & DATA 103 will signal room and instructor conflicts.

rpruim commented 1 year ago

This is a subset of #244

jmw-75 commented 1 year ago

We have resolved this issue by creating a new value in the conflictData interface within conflictsService.ts file. We then included the variable in the conflictData[]. This will now bring in the semester length value. We added a check for the semester length of the two classes which resolved the issue.

rpruim commented 1 year ago

Is it ready for testing? Which branch?

jmw-75 commented 1 year ago

@rpruim we are currently pushing it to a branch named halfSemesterConflict. Should be there in a few minutes.

samuelth47 commented 1 year ago

We will do some more testing and push it to production.

rpruim commented 1 year ago

Take a look at my comments in (https://github.com/senior-knights/course-schedulizer/commit/009a55c9b37d96cb60ba62fb7ca4e1b39637451d#commitcomment-90648941).

Short version: need to handle conflicts with courses of different lengths (Fall vs Fall 1; Fall 1 vs Fall B; etc.)

rpruim commented 1 year ago

I decided to just add my code to the branch and in minimal testing, it appears to be working.

Give it some more testing. (Ideally we should create some formal tests for this.)

Let me know if you don't understand something.

(Note: I also changed a line to avoid a eslint warning about the use of ! See the || "Full" as an alternative solution -- basically make courses default to Full semester courses if nothing is listed for them.)

rpruim commented 1 year ago

I've refactored a bit and added low level tests for the components. We could add another layer of testing in the context of a schedule, but things are looking good:

 PASS  src/utilities/services/conflictServices.spec.ts
  ✓ sign function works correctly (2ms)
  ✓ sign function works correctly (2ms)
  ✓ sign function works correctly (1ms)
  ✓ rangesOverlap() works correctly
  ✓ rangesOverlap() works correctly (1ms)
  ✓ rangesOverlap() works correctly
  ✓ check for conflict between Full and First is correct
  ✓ check for conflict between Full and Second is correct (1ms)
  ✓ check for conflict between Full and A is correct
  ✓ check for conflict between Full and B is correct
  ✓ check for conflict between Full and C is correct (1ms)
  ✓ check for conflict between Full and D is correct
  ✓ check for conflict between First and Full is correct
  ✓ check for conflict between First and A is correct
  ✓ check for conflict between First and B is correct (1ms)
  ✓ check for conflict between Second and Full is correct
  ✓ check for conflict between Second and C is correct
  ✓ check for conflict between Second and D is correct
  ✓ check for conflict between First and Second is correct
  ✓ check for conflict between First and C is correct (1ms)
  ✓ check for conflict between First and D is correct
  ✓ check for conflict between Second and First is correct
  ✓ check for conflict between Second and A is correct
  ✓ check for conflict between Second and B is correct

Test Suites: 1 passed, 1 total
Tests:       24 passed, 24 total
Snapshots:   0 total
Time:        2.637s
rpruim commented 1 year ago

Closing for now based on #278.

We may need to open a new issue to deal with custom terms at some point.