From d86a55754c64894974b2516c771eea8b549e5126 Mon Sep 17 00:00:00 2001 From: AddisonDunn Date: Tue, 25 Jun 2024 12:39:42 -0400 Subject: [PATCH 1/7] add tests for util function --- corehq/apps/locations/tests/test_util.py | 43 ++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 corehq/apps/locations/tests/test_util.py diff --git a/corehq/apps/locations/tests/test_util.py b/corehq/apps/locations/tests/test_util.py new file mode 100644 index 000000000000..5b0cf9fbd921 --- /dev/null +++ b/corehq/apps/locations/tests/test_util.py @@ -0,0 +1,43 @@ +from corehq.apps.domain.shortcuts import create_domain +from corehq.apps.commtrack.tests.util import TEST_LOCATION_TYPE, make_loc +from corehq.apps.users.models import CommCareUser, WebUser +from corehq.apps.locations.util import does_location_type_have_users + +from django.test import TestCase +from corehq.apps.users.dbaccessors import delete_all_users +from corehq.apps.es.client import manager +from corehq.apps.es.tests.utils import es_test +from corehq.apps.es.users import user_adapter +from corehq.util.es.testing import sync_users_to_es +from corehq.apps.locations.models import LocationType + + +@es_test(requires=[user_adapter]) +class UtilTest(TestCase): + @classmethod + def setUpClass(cls): + super(UtilTest, cls).setUpClass() + cls.domain = create_domain('locations-test') + cls.loc = make_loc('loc', type='outlet', domain=cls.domain.name) + cls.loc_type = LocationType.objects.get(name=TEST_LOCATION_TYPE) + + @classmethod + def tearDownClass(cls): + cls.domain.delete() + super(UtilTest, cls).tearDownClass() + + def tearDown(self): + delete_all_users() + + @sync_users_to_es() + def test_loc_type_has_users(self): + delete_all_users() + user1 = WebUser.create(self.domain.name, 'web-user@example.com', '123', None, None) + user2 = CommCareUser.create(self.domain.name, 'mobile-user', '123', None, None) + user1.set_location(self.domain.name, self.loc) + user2.set_location(self.loc) + manager.index_refresh(user_adapter.index_name) + self.assertTrue(does_location_type_have_users(self.loc_type)) + + def _loc_type_does_not_have_user(self): + self.assertFalse(does_location_type_have_users(self.loc_type)) From e4c66aafaff78684849cccfad415d97d188524af Mon Sep 17 00:00:00 2001 From: AddisonDunn Date: Tue, 25 Jun 2024 12:40:40 -0400 Subject: [PATCH 2/7] add util function --- corehq/apps/locations/util.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/corehq/apps/locations/util.py b/corehq/apps/locations/util.py index 6e0c41750d1d..4fbe96989ee7 100644 --- a/corehq/apps/locations/util.py +++ b/corehq/apps/locations/util.py @@ -30,6 +30,7 @@ from corehq.blobs import CODES, get_blob_db from corehq.form_processor.interfaces.supply import SupplyInterface from corehq.util.files import safe_filename_header +from corehq.apps.locations.dbaccessors import user_ids_at_locations def load_locs_json(domain, selected_loc_id=None, include_archived=False, @@ -429,3 +430,10 @@ def get_location_type(domain, location, parent, loc_type_string, exception, is_n return loc_type_obj # --- + + +# Checks if a location type has any users assigned to it's related locations +def does_location_type_have_users(loc_type): + location_ids = list(SQLLocation.objects.filter(location_type=loc_type).values_list('location_id', flat=True)) + users = user_ids_at_locations(location_ids) + return bool(users) From 11f6ff0a6b92b07dbd4634a0d5341d4dbef61e6d Mon Sep 17 00:00:00 2001 From: AddisonDunn Date: Tue, 25 Jun 2024 12:50:51 -0400 Subject: [PATCH 3/7] add backend validation code for loc type 'has users' check --- corehq/apps/locations/views.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/corehq/apps/locations/views.py b/corehq/apps/locations/views.py index fa84c4f2c615..b3da6f6be448 100644 --- a/corehq/apps/locations/views.py +++ b/corehq/apps/locations/views.py @@ -3,6 +3,7 @@ from django.contrib import messages from django.core.cache import cache +from django.core.exceptions import ObjectDoesNotExist from django.http import Http404, HttpResponseRedirect from django.http.response import HttpResponseServerError from django.shortcuts import get_object_or_404, redirect, render @@ -66,7 +67,7 @@ user_can_edit_location_types, ) from .tree_utils import assert_no_cycles -from .util import load_locs_json, location_hierarchy_config +from .util import does_location_type_have_users, load_locs_json, location_hierarchy_config from django.http import JsonResponse, HttpResponseBadRequest from corehq.apps.locations.dbaccessors import get_filtered_locations_count @@ -389,6 +390,17 @@ def unique_name_and_code(): return False return True + def _verify_has_users_config(loc_type_payload, pk): + if not loc_type.get('has_users'): + try: + existing_loc_type_sql_obj = LocationType.objects.get(pk=pk) + except ObjectDoesNotExist: # Location type not created yet + return + if does_location_type_have_users(existing_loc_type_sql_obj): + raise LocationConsistencyError(f"Locations of the organization level '{loc_type['name']}' " + "have users assigned to them. You can't uncheck the " + "'Has Users' setting for this level!") + loc_types = payload['loc_types'] pks = [] payload_loc_type_name_by_pk = {} @@ -405,6 +417,10 @@ def unique_name_and_code(): payload_loc_type_name_by_pk[loc_type['pk']] = loc_type['name'] if loc_type.get('code'): payload_loc_type_code_by_pk[loc_type['pk']] = loc_type['code'] + # All location types have users until UI allows otherwise. + # Remove below line when UI work is being done. + loc_type['has_users'] = True + _verify_has_users_config(loc_type, pk) names = list(payload_loc_type_name_by_pk.values()) names_are_unique = len(names) == len(set(names)) codes = list(payload_loc_type_code_by_pk.values()) From 71048bfdae6efa4f8b28c8e50301fad3ffb5ebba Mon Sep 17 00:00:00 2001 From: AddisonDunn Date: Tue, 25 Jun 2024 15:48:07 -0400 Subject: [PATCH 4/7] add FF --- corehq/apps/locations/views.py | 6 ++---- corehq/toggles/__init__.py | 7 +++++++ 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/corehq/apps/locations/views.py b/corehq/apps/locations/views.py index b3da6f6be448..a0ce9c335647 100644 --- a/corehq/apps/locations/views.py +++ b/corehq/apps/locations/views.py @@ -417,10 +417,8 @@ def _verify_has_users_config(loc_type_payload, pk): payload_loc_type_name_by_pk[loc_type['pk']] = loc_type['name'] if loc_type.get('code'): payload_loc_type_code_by_pk[loc_type['pk']] = loc_type['code'] - # All location types have users until UI allows otherwise. - # Remove below line when UI work is being done. - loc_type['has_users'] = True - _verify_has_users_config(loc_type, pk) + if toggles.LOCATION_HAS_USERS.enabled(self.domain): + _verify_has_users_config(loc_type, pk) names = list(payload_loc_type_name_by_pk.values()) names_are_unique = len(names) == len(set(names)) codes = list(payload_loc_type_code_by_pk.values()) diff --git a/corehq/toggles/__init__.py b/corehq/toggles/__init__.py index db76bc7ce716..5f4b814f2736 100644 --- a/corehq/toggles/__init__.py +++ b/corehq/toggles/__init__.py @@ -2888,3 +2888,10 @@ def domain_has_privilege_from_toggle(privilege_slug, domain): tag=TAG_CUSTOM, namespaces=[NAMESPACE_DOMAIN], ) + +LOCATION_HAS_USERS = StaticToggle( + slug='location_has_users', + label='USH Dev: Allows marking whether a location should have users assigned or not.', + tag=TAG_PRODUCT, + namespaces=[NAMESPACE_DOMAIN], +) From 0c3073473d494cf1317b98ab93de5c3e3a81cb15 Mon Sep 17 00:00:00 2001 From: AddisonDunn Date: Mon, 1 Jul 2024 17:06:30 -0400 Subject: [PATCH 5/7] use count for more efficient ES query --- corehq/apps/locations/util.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/corehq/apps/locations/util.py b/corehq/apps/locations/util.py index 4fbe96989ee7..6f202d017126 100644 --- a/corehq/apps/locations/util.py +++ b/corehq/apps/locations/util.py @@ -20,6 +20,7 @@ get_loaded_default_monthly_consumption, ) from corehq.apps.domain.models import Domain +from corehq.apps.es.users import UserES from corehq.apps.locations.const import ( LOCATION_SHEET_HEADERS_BASE, LOCATION_SHEET_HEADERS_OPTIONAL, @@ -30,7 +31,6 @@ from corehq.blobs import CODES, get_blob_db from corehq.form_processor.interfaces.supply import SupplyInterface from corehq.util.files import safe_filename_header -from corehq.apps.locations.dbaccessors import user_ids_at_locations def load_locs_json(domain, selected_loc_id=None, include_archived=False, @@ -435,5 +435,4 @@ def get_location_type(domain, location, parent, loc_type_string, exception, is_n # Checks if a location type has any users assigned to it's related locations def does_location_type_have_users(loc_type): location_ids = list(SQLLocation.objects.filter(location_type=loc_type).values_list('location_id', flat=True)) - users = user_ids_at_locations(location_ids) - return bool(users) + return bool(UserES().location(location_ids).count()) From 96060a6b62ee42cb3412e93325ca0e4bdce26a93 Mon Sep 17 00:00:00 2001 From: AddisonDunn Date: Mon, 1 Jul 2024 17:09:35 -0400 Subject: [PATCH 6/7] use view's existing way of telling whether location type is newly created --- corehq/apps/locations/views.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/corehq/apps/locations/views.py b/corehq/apps/locations/views.py index a0ce9c335647..815ddee2d34b 100644 --- a/corehq/apps/locations/views.py +++ b/corehq/apps/locations/views.py @@ -3,7 +3,6 @@ from django.contrib import messages from django.core.cache import cache -from django.core.exceptions import ObjectDoesNotExist from django.http import Http404, HttpResponseRedirect from django.http.response import HttpResponseServerError from django.shortcuts import get_object_or_404, redirect, render @@ -392,11 +391,9 @@ def unique_name_and_code(): def _verify_has_users_config(loc_type_payload, pk): if not loc_type.get('has_users'): - try: - existing_loc_type_sql_obj = LocationType.objects.get(pk=pk) - except ObjectDoesNotExist: # Location type not created yet + if _is_fake_pk(pk): return - if does_location_type_have_users(existing_loc_type_sql_obj): + if does_location_type_have_users(LocationType.objects.get(pk=pk)): raise LocationConsistencyError(f"Locations of the organization level '{loc_type['name']}' " "have users assigned to them. You can't uncheck the " "'Has Users' setting for this level!") From 372f5c89195c0b408a64e8a4179b74b47f0f7c16 Mon Sep 17 00:00:00 2001 From: AddisonDunn Date: Tue, 2 Jul 2024 11:37:57 -0400 Subject: [PATCH 7/7] specify domain when grabbing loc type --- corehq/apps/locations/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/corehq/apps/locations/views.py b/corehq/apps/locations/views.py index 815ddee2d34b..44bfe0edbc6a 100644 --- a/corehq/apps/locations/views.py +++ b/corehq/apps/locations/views.py @@ -393,7 +393,7 @@ def _verify_has_users_config(loc_type_payload, pk): if not loc_type.get('has_users'): if _is_fake_pk(pk): return - if does_location_type_have_users(LocationType.objects.get(pk=pk)): + if does_location_type_have_users(LocationType.objects.get(pk=pk, domain=self.domain)): raise LocationConsistencyError(f"Locations of the organization level '{loc_type['name']}' " "have users assigned to them. You can't uncheck the " "'Has Users' setting for this level!")