Fix internal errors from filters #2346
Conversation
There was a problem hiding this comment.
In general this looks good and definitely as a new feature rather than a bugfix.
There are some questions left in the review:
- What spelling we should use, @maneesha?
- Which fix for the name ordering filter we should choose?
- What is the impact of
setUpClass/tearDownClasson parallelization of test runs?
Because of these questions I'm going to leave the review as "Request changes" and not push it into milestone https://github.com/carpentries/amy/milestone/95.
| @@ -58,7 +58,7 @@ def filter_nonpositive_remaining_seats(queryset, name, seats): | |||
|
|
|||
| class MembershipFilter(AMYFilterSet): | |||
| organization_name = django_filters.CharFilter( | |||
| label="Organisation name", | |||
| label="Organization name", | |||
There was a problem hiding this comment.
I think at some point in past British English was chosen as the actual spelling for user interface. @maneesha can you confirm or deny this?
There was a problem hiding this comment.
We use 'Organization' everywhere else in the UI. I don't know if British English spelling was agreed on or not, but in this case the 'z' is acceptable in British English anyway.
(I've just been down the rabbit hole and it turns out the Oxford English Dictionary even prefers the -ize/-ization endings...)
amy/fiscal/filters.py
Outdated
| @@ -76,13 +76,13 @@ class MembershipFilter(AMYFilterSet): | |||
| ) | |||
|
|
|||
| training_seats_only = django_filters.BooleanFilter( | |||
| label="Only show memberships with non-zero allowed training seats", | |||
| label="Only show memberships with more than zero allowed training seats", | |||
| method=filter_training_seats_only, | |||
| widget=widgets.CheckboxInput, | |||
| ) | |||
|
|
|||
| nonpositive_remaining_seats_only = django_filters.BooleanFilter( | |||
There was a problem hiding this comment.
Can you rename this field to negative_remaining_seats_only or similar to indicate the logic behind it?
I would suggest remaining the filter method too.
amy/fiscal/filters.py
Outdated
| method=filter_training_seats_only, | ||
| widget=widgets.CheckboxInput, | ||
| ) | ||
|
|
||
| nonpositive_remaining_seats_only = django_filters.BooleanFilter( | ||
| label="Only show memberships with zero or less remaining seats", | ||
| label="Only show memberships with less than zero remaining seats", | ||
| method=filter_nonpositive_remaining_seats, |
There was a problem hiding this comment.
I suggest renaming to filter_negative_remaining_seats or similar.
amy/workshops/filters.py
Outdated
| return ordering.order_by("personal", "middle", "family") | ||
| elif any(v in ["-firstname"] for v in value): | ||
| elif any(v in ["-personal"] for v in value): | ||
| return ordering.order_by("-personal", "-middle", "-family") |
There was a problem hiding this comment.
I also fixed this filter in here: https://github.com/carpentries/amy/pull/2335/files#diff-76c8f195f620772cf38ce87a84d610ada62278260c666e317c8387d8f274adf7
I would prefer my approach unless you find something wrong with it. I think the extra choices were intentionally set to "lastname" and "firstname", as we don't have these fields in any model, and NamesOrderingFilter allows to filter by additional fields if provided.
There was a problem hiding this comment.
Your approach looks good, I'll revert these changes
amy/fiscal/tests/test_filters.py
Outdated
|
|
||
| class TestMembershipFilter(TestCase): | ||
| @classmethod | ||
| def setUpClass(cls) -> None: |
There was a problem hiding this comment.
Is there a particular reason why you chose setUpClass and tearDownClass class methods?
The docs say it should be used with caution:
Note that shared fixtures do not play well with [potential] features like test parallelization and they break test isolation. They should be used with care.
In all other cases we used setUp() and tearDown methods.
There was a problem hiding this comment.
I am a little afraid about parallelization of these tests (I use it often with make fast_test or python manage.py test --parallel commands).
There was a problem hiding this comment.
Habit from a past project, and a desire to keep the number of database calls down. Happy to change to setUp/tearDown. (and actually learned about the helper setup methods in TestBase since I wrote this, so will look at using those too)
amy/fiscal/tests/test_filters.py
Outdated
| cls.member = Member.objects.create( | ||
| membership=cls.membership, organization=cls.organization, role=member_role | ||
| ) | ||
| # create a test membership that sphould be filtered out |
There was a problem hiding this comment.
Small typo: sphould -> should.
amy/fiscal/tests/test_filters.py
Outdated
|
|
||
| # Assert | ||
| self.assertTrue(self.membership in result) | ||
| self.assertFalse(self.membership2 in result) |
There was a problem hiding this comment.
There's also assertIn and assertNotIn, they may provide better insight if the condition fails.
| ) | ||
| - Coalesce("inhouse_instructor_training_seats_rolled_over", 0) | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Since it looks like we have this same query in multiple places (twice in this file, one in fiscal.views.AllMemberships.queryset and one in reports.views.membership_trainings_stats) then I propose to extract it out, for example into a utility function, and use it in all 4 places instead. Hopefully this would lead to less changes in future and less errors in the calculations since they are quite difficult.
There was a problem hiding this comment.
Good idea. I moved this to a new Manager for the Membership object - could do with another review
amy/fiscal/tests/test_filters.py
Outdated
|
|
||
| # Assert | ||
| # we don't have any unexpected fields | ||
| self.assertListEqual(list(fields.keys()), list(expected_results.keys())) |
There was a problem hiding this comment.
Does assertEqual(fields.keys(), expected_results.keys()) also work? .keys() returns a set-like object, so we should be able to compare them.
There was a problem hiding this comment.
It does, I think I iterated this a few times and then didn't think about simplifying 😄
amy/fiscal/tests/test_filters.py
Outdated
|
|
||
| class TestMembershipTrainingsFilter(TestCase): | ||
| """ | ||
| Duplicate of TestMembershipFilter with some fields removed. |
There was a problem hiding this comment.
What fields were removed?
Fixes #2327 and introduces tests for membership filters.
Includes combining some columns in the membership training stats view as requested in that issue.