chore: update test for running on empty db #409
Conversation
| default_perm_names = [ | ||
| "gql_query_roles_perms", | ||
| "gql_mutation_create_roles_perms", | ||
| "gql_mutation_update_roles_perms", | ||
| "gql_mutation_replace_roles_perms", | ||
| "gql_mutation_duplicate_roles_perms", | ||
| "gql_mutation_delete_roles_perms", | ||
| "gql_query_users_perms", | ||
| "gql_query_users_profile_perms", | ||
| "gql_mutation_create_users_perms", | ||
| "gql_mutation_update_users_perms", | ||
| "gql_mutation_delete_users_perms", | ||
| "gql_query_enrolment_officers_perms", | ||
| "gql_mutation_create_enrolment_officers_perms", | ||
| "gql_mutation_update_enrolment_officers_perms", | ||
| "gql_mutation_delete_enrolment_officers_perms", | ||
| "gql_query_claim_administrator_perms", | ||
| "gql_mutation_create_claim_administrator_perms", | ||
| "gql_mutation_update_claim_administrator_perms", | ||
| "gql_mutation_delete_claim_administrator_perms", | ||
| ] |
There was a problem hiding this comment.
Don't we already have a set of user roles configured with permissions somewhere? If so, can we reuse what already exists? If not, can all these perms and below perms be in fixture files so 1) the test file is slimmer and 2) those fixtures may potentially be reused, even just as sample setups?
There was a problem hiding this comment.
the issue is the perms are a mess, and any fixture will use id of perms only referenced in the code; I use that helper to be able to make better test; but I had to support existing test while not spending week on it
honestly I think we should check how to use the django perm model
| # Create French language if it doesn't exist | ||
| Language.objects.get_or_create( | ||
| code="fr", | ||
| defaults={"name": "Français", "sort_order": 1} | ||
| ) |
There was a problem hiding this comment.
Is this setup required for testing any specific language logic below?
There was a problem hiding this comment.
likely an user with 'fr' as a language
There was a problem hiding this comment.
Consider putting common test data setup in a custom TestRunner that can be reused so the same setup doesn't need to be repeated over and over again in different modules
There was a problem hiding this comment.
@weilu there is 800 tests that I need to update, for now I aim of having the CI back to green, then we could check if we want to invest in cleaning; for now I rather have the minimun data per test
| create_test_interactive_user(username="one") | ||
| create_test_interactive_user(username="two") |
There was a problem hiding this comment.
How are these two created users referenced? I don't see them referenced by username
There was a problem hiding this comment.
because all the rest of the test in to make sure the cache update properly, if username not specified, the test helper will return the same user
…ew tests for role creation and error handling.
…feature/test-empty-db
weilu
left a comment
weilu
left a comment
There was a problem hiding this comment.
@delcroip this is not a pure chore changeset to update test for running on empty DB. It's mixed in with non-test code changes some of which changes behavior in critical path of production code with unclear intentions. Please kindly explain the rationale behind those proposed changes.
| import datetime | ||
| from django.core.cache import caches | ||
| from functools import lru_cache | ||
| # utils/request_local.py |
| default_filter = kwargs.pop("applyDefaultValidityFilter", False) | ||
| date_valid_from = kwargs.pop("dateValidFrom__Gte", None) | ||
| date_valid_to = kwargs.pop("dateValidTo__Lte", None) |
There was a problem hiding this comment.
Why changing this from get to pop? With this change any subsequent attempt to access these keys would fail
| def _get_user(self, user=None, username=None): | ||
|
|
||
| audit_user = get_current_user() | ||
| if audit_user: | ||
| return audit_user | ||
| elif user: | ||
| return user | ||
| elif username: | ||
| audit_user = User.objects.get(username=username, *User.filter_validity()) | ||
| return audit_user | ||
| else: | ||
| raise ValidationError( | ||
| "Save error! Provide user or the username of the current user in `username` argument" | ||
| ) | ||
|
|
There was a problem hiding this comment.
@delcroip as discussed in #408 (comment) "Models shouldn't be aware of current user as that's at the controller layer." This breaks separation of concerns and should be removed.
The original super().setUp(self) was incorrect syntax; updated to super(openIMISGraphQLTestCase, self).setUp() to properly invoke the parent class setUp method from GraphQLTestCase. This ensures the test setup is initialized correctly.
- Add cache clearing and current user reset to prevent stale data - Include user validity filter to ensure only valid users are retrieved - Ensure user object is saved in appropriate scenarios for consistency
need openimis/openimis-be-location_py#166