Closed hana9235 closed 9 years ago
From what I understand it works
Thanks for tackling this; I'm going to test this extensively later to make sure we don't have any more issues with constraint removal
Actually I was thinking this was for issue #48. Disregard that.
From my testing, you seemed to fix the issue, though I'm having trouble adding instructor_day constraints. It's having a similar conflict detection error.
The YAML input has a Shade prefers TR constraint, but I do not see it in the list of constraints added. It sets off the conflict detection when I try to add a Shade day constraint though.
Edit: We determined the cause of this; it's a separate issue
Issue #48 has been fixed with the latest commit. The delete buttons were not taking into account the hard constraints (ex: subsequent courses different building) and were deleting them instead of the selected user constraints.
So to update: This pull request fixes the computer_preference/max_course constraints conflict (which was not actually a conflict), and fixes issues with deleting constraints.
:100:
-All default constraints are being showed in the listbox -Delete and delete all seem to be working -Computer preference and max course do not conflict -Hard constraints are not being removed
Excellent job; with this fix, I do not believe we have any known bugs remaining, just a few interface improvements and the tickets for this sprint.
Worked great for me. Fantastic job!
This resolves issue #49
Adding both max_courses and computer_preference constraints for the same instructor were incorrectly marked as conflicting. This has been resolved.
There was also no error checking for max_courses constraints (duplicates were possible, and Shade_max_courses_2 and Shade_max_courses_3 could both be added to the program without an issue). That functionality has also been added in this pull request.
Note: I have a commit in here (50dd680) that is also in the fix constraint toggle PR. I think that was just because of a mistake I made while branching. If it'll cause problems for merging both PRs, let me know.