Feature/adding location #129
Conversation
to facilitate row security
Aka GroupIndividual
Also refactor individual mutation & query test to extract setup code
* Fix perforamnce for individual data update * added saving in bulk way validation errors * removed redundant print --------- Co-authored-by: sniedzielski <sniedzielski@soldevelo.com>
There was a problem hiding this comment.
@delcroip The change to use LocationManager.is_allowed instead of Location.get_queryset introduces the following issues:
- it no longer considers and respects
settings.ROW_SECURITY, - users with
user.is_imis_admin = Truenow is not permitted to mutate any individual or group with location. Case 2 is actually covered by a test case you removed (see my comment in code).
In other words, Location.get_queryset offers more functionalities needed than that LocationManager.is_allowed is currently offering. In the spirit of DRY, I'd suggest not replicating the checks inside is_allowed, but considering pushing the caching logic into LocationManager.allowed which is used by Location.get_queryset and keep using Location.get_queryset for location authorization checking.
| id = content['data']['editIndividualInGroup']['internalId'] | ||
| self.assert_mutation_error(id, 'mutation.individual_group_village_mismatch') |
There was a problem hiding this comment.
@delcroip This test case shouldn't have been deleted as it catches the case that an imis admin user can manipulate any individual's group assignment but we should validate to ensure the individual and their assigned group's locations are consistent.
| content = json.loads(response.content) | ||
| internal_id = content['data']['updateGroup']['internalId'] | ||
| self.assert_mutation_error(internal_id, 'mutation.authentication_required') |
There was a problem hiding this comment.
This shouldn't be necessary as the anonymous user case is already tested in the test above. I see why you added them – probably due to the extraneous line I have above where the response was not checked. I'll push a change to remove this and similar changes below.
There was a problem hiding this comment.
I added it to force the execution of the mutation, if not error could happen later in the process, if we remove the get_mutation_result we should also remove the query
There was a problem hiding this comment.
Yes, I removed the query in 324c578
- Also modify existing cases to ensure imis admin can modify individual and group with location - Show error message when assert_mutation_success fails
…/openimis-be-individual_py into feature/adding-location
| @@ -474,7 +474,7 @@ def test_add_individual_to_group_row_security(self): | |||
| ) | |||
| content = json.loads(response.content) | |||
| internal_id = content['data']['addIndividualToGroup']['internalId'] | |||
| self.assert_mutation_error(internal_id, _('unauthorized.location')) | |||
| self.assert_mutation_success(internal_id) | |||
There was a problem hiding this comment.
@delcroip why has this been changed to success? Shouldn't we prevent such a scenario to keep location data consistent between individual and their group?
On group assignments and updates
|
No description provided.