Skip to content
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

Merged
merged 2 commits into from
Aug 16, 2021

Conversation

rlskoeser
Copy link
Contributor

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

When filtering exec members from current staff,
only consider _current_ exec committee titles.
@rlskoeser rlskoeser requested a review from thatbudakguy August 12, 2021 21:55
@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2021

Codecov Report

Merging #373 (f61bcb8) into develop (fdf077b) will decrease coverage by 0.02%.
The diff coverage is 85.00%.

@@             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     

Copy link
Contributor

@thatbudakguy thatbudakguy left a 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?

Comment on lines 178 to 185
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())
)
Copy link
Contributor

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!)

Copy link
Contributor Author

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)

@rlskoeser
Copy link
Contributor Author

thanks for figuring this out! does this resolve #368?

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...)

@rlskoeser
Copy link
Contributor Author

rlskoeser commented Aug 16, 2021

@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

@thatbudakguy
Copy link
Contributor

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.

@rlskoeser rlskoeser merged commit 7c66f36 into develop Aug 16, 2021
@rlskoeser rlskoeser deleted the bugfix/staff-nonexec-fix branch August 16, 2021 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants