move from current and next term to all active terms#408
Conversation
|
@miretskydatwit pls convert back from draft when you've tested :) Thx! |
|
Two issues worth fixing before this merges: 1. N+1 query in active_terms = all.select do |term|
reg_start = term.registration_start # fires a SQL query per term
...
end
Options:
The Ruby-level filtering approach is fine for small datasets in tests, but will hurt in prod. 2. Fragile .where("summary ILIKE ?", "%registration%open%")This requires the summary to contain both "registration" and "open" in that order. Real university calendar events could say "Open Registration", "Registration Begins", "Registration Period", "Priority Registration", etc. — all of which would be silently missed, causing the fallback to Suggest either:
|
jaspermayone
left a comment
There was a problem hiding this comment.
@miretskydatwit plz address above feedback before marking as ready :)
|
weird that went twice lol |
|
@jaspermayone (preferably before you review this PR again) pls review and merge #439 and after merging that, pls merge from main into this branch ( |
|
@jaspermayone bump, pls review, thx! i do see rspec is failing but not entirely sure why. |
|
This should kind of get solved with the rewrite, but I'll check in on it |
* hi * fix codespace * apply jasper's suggestions --------- Signed-off-by: Daniel Miretsky <miretskyd@wit.edu>
* hi * fix codespace * apply jasper's suggestions * fix misc spec * fix misc spec * fix term start and end dates --------- Signed-off-by: Daniel Miretsky <miretskyd@wit.edu>
a11f4fe to
bb4ec50
Compare
Closes #369