-
Notifications
You must be signed in to change notification settings - Fork 5
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
Fix current_position_nonexec filter so it only filters out current exec positions, not past #373
Conversation
When filtering exec members from current staff, only consider _current_ exec committee titles.
Codecov Report
@@ Coverage Diff @@
## develop #373 +/- ##
===========================================
- Coverage 98.20% 98.18% -0.03%
===========================================
Files 88 88
Lines 3624 3638 +14
===========================================
+ Hits 3559 3572 +13
- Misses 65 66 +1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for figuring this out! does this resolve #368?
def _current_position_query(self): | ||
# query to find a person with a current cdh position | ||
# person *has* a position and it has no end date or date after today | ||
return models.Q(positions__isnull=False) & ( | ||
today = date.today() | ||
return models.Q(positions__start_date__lte=today) & ( | ||
models.Q(positions__end_date__isnull=True) | ||
| models.Q(positions__end_date__gte=date.today()) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this get used elsewhere? seems like it'd affect other things too (and glad we fixed it!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it gets used in multiple places, which is why we had a method for it. Yeah, not sure how we missed that one (although it's an edge case for sure)
No, it doesn't — I think it's a separate bug that we should make an issue for. For #368 we need to include faculty director and any interim faculty directors at the beginning of the exec committee member list, and I'm not sure how to do that with the current queries we have without breaking other things. (Maybe we don't make it part of the current query and just look for faculty directors separately from exec...) |
@thatbudakguy how do you feel about the test coverage? I don't think this method is tested explicitly anywhere although codecov says the method is covered — not sure where the patch diff is coming from |
i noticed that too. i'm fine with merging; doesn't seem like something we need to fix right now — might get revisited or invalidated as part of #232 anyway. |
this was preventing acting faculty director with past exec membership from showing up on the staff page — they were listed, but always listed as past because the filter logic was catching on their other positions