search

learning-unlimited / ESP-Website

A website to help manage the logistics of large, short-term educational programs
82 stars 57 forks source link

Lunch constraint generator doesn't set the right seqs #2510

Closed benjaminjkraft closed 6 years ago

benjaminjkraft commented 6 years ago

The lunch constraint generator generates a set of ScheduleConstraint based on the selections in the interface and the program's timeblocks; each consists of two BooleanExpressions, a condition and a requirement, which consist of an RPN-style stack of BooleanTokens, so for example [ScheduleTestOccupied(timeblock_1), ScheduleTestOccupied(timeblock_2), '||'] would be true if the user has a class in timeblock_1 or timeblock_2. The ordering of the stack is stored in the seq field of the BooleanToken.

For some reason, the lunch constraint generator hasn't always been setting distinct seq objects on each of its BooleanTokens, such that their ordering is not well-defined. In the past, they seem to somehow have always come out in the right order, I'm guessing because the database broke ties by id, which happened to be the intended ordering. But for MIT ESP Spark 2018, possibly because of a postgres upgrade, they didn't, which caused an expression like the above to always evaluate to False, which as the requirement meant the constraint not satisfied when it should have been.

The fix will be to correctly set the seq on each token. I'm not totally sure why it's not getting set correctly, but it seems that a simple OR like the above consistently results in all three seqs being 0, whereas really the OR should have a strictly higher seq.

benjaminjkraft commented 6 years ago

cc @betaveros I think you were interested in hearing about this.

betaveros commented 6 years ago

Do we really need the flexibility of storing a DSL that can express arbitrarily complicated boolean conditions in terms of its atoms in the database?

benjaminjkraft commented 6 years ago

Certainly not the way they're used now! Replacing lunch constraints with something a lot less general would also be a valid solution to this problem, and definitely a worthwhile undertaking. Or even something almost as general but less Clever -- e.g. a simple text or JSON DSL for the conditions rather than storing them as database structures would have avoided this issue as well as making them much easier to read. But probably if you're replacing them wholesale, just do it right and don't make it general at all.