Merge pull request #35296 from dimagi/es/location-fields-2

Move location fields out of user data

corehq/apps/api/resources/v0_1.py

@@ -17,6 +17,7 @@
 from corehq.apps.groups.models import Group
 from corehq.apps.user_importer.helpers import UserChangeLogger
 from corehq.apps.users.models import CommCareUser, HqPermissions, WebUser
+from corehq.apps.users.util import user_location_data
 from corehq.const import USER_CHANGE_VIA_API
 
 
@@ -105,6 +106,14 @@ def dehydrate(self, bundle):
 
     def dehydrate_user_data(self, bundle):
         user_data = bundle.obj.get_user_data(bundle.obj.domain).to_dict()
+        if location_id := bundle.obj.get_location_id(bundle.obj.domain):
+            # This is all available in the top level, but add in here for backwards-compatibility
+            user_data['commcare_location_id'] = location_id
+            user_data['commcare_location_ids'] = user_location_data(
+                bundle.obj.get_location_ids(bundle.obj.domain))
+            user_data['commcare_primary_case_sharing_id'] = location_id
+
+        user_data['commcare_project'] = bundle.obj.domain
         if self.determine_format(bundle.request) == 'application/xml':
             # attribute names can't start with digits in xml
             user_data = {k: v for k, v in user_data.items() if not k[0].isdigit()}

corehq/apps/callcenter/sync_usercase.py

@@ -136,10 +136,10 @@ def valid_element_name(name):
     fields = {k: v for k, v in user.get_user_data(domain).items() if
               valid_element_name(k)}
 
-    if user.is_web_user():
-        fields['commcare_location_id'] = user.get_location_id(domain)
+    if location_id := user.get_location_id(domain):
+        fields['commcare_location_id'] = location_id
         fields['commcare_location_ids'] = user_location_data(user.get_location_ids(domain))
-        fields['commcare_primary_case_sharing_id'] = user.get_location_id(domain)
+        fields['commcare_primary_case_sharing_id'] = location_id
 
     # language or phone_number can be null and will break
     # case submission
@@ -155,6 +155,7 @@ def valid_element_name(name):
         'hq_user_id': user.get_id,
         'first_name': user.first_name or '',
         'last_name': user.last_name or '',
+        'commcare_project': domain,
     })
 
     return fields

corehq/apps/reports/tests/test_esaccessors.py

@@ -1025,7 +1025,6 @@ def test_active_user_query(self):
         self.assertEqual(len(results), 1)
         metadata = results[0].pop('user_data_es')
         self.assertEqual({
-            'commcare_project': 'user-esaccessors-test',
             PROFILE_SLUG: self.profile.id,
             'job': 'reporter',
             'office': 'phone_booth',

corehq/apps/user_importer/tests/test_importer.py

@@ -131,8 +131,7 @@ def _test_user_data(self, is_web_upload=False):
             self.upload_record.pk,
             is_web_upload
         )
-        self.assert_user_data_equals({
-            'commcare_project': self.domain.name, 'key': 'F#', 'commcare_profile': '', 'mode': ''})
+        self.assert_user_data_equals({'key': 'F#', 'commcare_profile': '', 'mode': ''})
 
         # Update user_data
         import_users_and_groups(
@@ -143,8 +142,7 @@ def _test_user_data(self, is_web_upload=False):
             self.upload_record.pk,
             is_web_upload
         )
-        self.assert_user_data_equals({
-            'commcare_project': self.domain.name, 'key': 'Bb', 'commcare_profile': '', 'mode': ''})
+        self.assert_user_data_equals({'key': 'Bb', 'commcare_profile': '', 'mode': ''})
 
         # set user data to blank
         import_users_and_groups(
@@ -178,12 +176,7 @@ def _test_user_data_profile(self, is_web_upload=False):
             is_web_upload
         )
 
-        self.assert_user_data_equals({
-            'commcare_project': self.domain.name,
-            'key': 'F#',
-            'mode': 'minor',
-            PROFILE_SLUG: self.profile.id,
-        })
+        self.assert_user_data_equals({'key': 'F#', 'mode': 'minor', PROFILE_SLUG: self.profile.id})
         user_history = UserHistory.objects.get(
             user_id=self.user.get_id,
             changed_by=self.uploading_user.get_id,
@@ -452,7 +445,6 @@ def test_location_not_list(self):
             False
         )
         self.assertEqual(self.user.location_id, self.loc1._id)
-        self.assert_user_data_item('commcare_location_id', self.user.location_id)
         # multiple locations
         self.assertListEqual([self.loc1._id], self.user.assigned_location_ids)
 
@@ -495,12 +487,8 @@ def test_location_add(self):
         )
         # first location should be primary location
         self.assertEqual(self.user.location_id, self.loc1._id)
-        self.assert_user_data_item('commcare_location_id', self.user.location_id)
-        self.assert_user_data_item('commcare_primary_case_sharing_id', self.user.location_id)
         # multiple locations
         self.assertListEqual([loc._id for loc in [self.loc1, self.loc2]], self.user.assigned_location_ids)
-        # non-primary location
-        self.assert_user_data_item('commcare_location_ids', " ".join([self.loc1._id, self.loc2._id]))
 
         user_history = UserHistory.objects.get(action=UserModelAction.CREATE.value,
                                                user_id=self.user.get_id,
@@ -551,7 +539,6 @@ def test_location_remove(self):
 
         # user should have no locations
         self.assertEqual(self.user.location_id, None)
-        self.assert_user_data_item('commcare_location_id', None)
         self.assertListEqual(self.user.assigned_location_ids, [])
 
         user_history = UserHistory.objects.get(action=UserModelAction.UPDATE.value,
@@ -578,8 +565,6 @@ def test_primary_location_replace(self):
 
         # user's primary location should be loc1
         self.assertEqual(self.user.location_id, self.loc1._id)
-        self.assert_user_data_item('commcare_location_id', self.loc1._id)
-        self.assert_user_data_item('commcare_location_ids', " ".join([self.loc1._id, self.loc2._id]))
         self.assertListEqual(self.user.assigned_location_ids, [self.loc1._id, self.loc2._id])
 
         user_history = UserHistory.objects.get(action=UserModelAction.CREATE.value,
@@ -604,8 +589,6 @@ def test_primary_location_replace(self):
 
         # user's location should now be loc2
         self.assertEqual(self.user.location_id, self.loc2._id)
-        self.assert_user_data_item('commcare_location_ids', self.loc2._id)
-        self.assert_user_data_item('commcare_location_id', self.loc2._id)
         self.assertListEqual(self.user.assigned_location_ids, [self.loc2._id])
 
         user_history = UserHistory.objects.get(action=UserModelAction.UPDATE.value,
@@ -653,7 +636,6 @@ def test_location_replace(self):
 
         # user's location should now be loc2
         self.assertEqual(self.user.location_id, self.loc2._id)
-        self.assert_user_data_item('commcare_location_id', self.loc2._id)
         self.assertListEqual(self.user.assigned_location_ids, [self.loc2._id])
 
         user_history = UserHistory.objects.get(action=UserModelAction.UPDATE.value,

corehq/apps/userreports/expressions/specs.py

@@ -708,6 +708,8 @@ def _get_user(domain, doc_id):
         else:
             doc[key] = getattr(user, key)
     doc['user_data'] = user.get_user_data(domain).to_dict()
+    # add location ID in here to not break existing reports - they _should_ just use user.location_id
+    doc['user_data']['commcare_location_id'] = user.get_location_id(domain)
     return doc
 
 

corehq/apps/users/models.py

@@ -1162,11 +1162,18 @@ def get_user_session_data(self, domain):
             session_data[COMMCARE_USER_TYPE_KEY] = COMMCARE_USER_TYPE_DEMO
 
         session_data.update({
+            f'{SYSTEM_PREFIX}_project': domain,
             f'{SYSTEM_PREFIX}_first_name': self.first_name,
             f'{SYSTEM_PREFIX}_last_name': self.last_name,
             f'{SYSTEM_PREFIX}_phone_number': self.phone_number,
             f'{SYSTEM_PREFIX}_user_type': self._get_user_type(),
         })
+
+        if location_id := self.get_location_id(domain):
+            session_data['commcare_location_id'] = location_id
+            session_data['commcare_location_ids'] = user_location_data(self.get_location_ids(domain))
+            session_data['commcare_primary_case_sharing_id'] = location_id
+
         return session_data
 
     def get_owner_ids(self, domain):
@@ -2647,14 +2654,6 @@ def get_location(self, domain):
     def get_usercase_by_domain(self, domain):
         return CommCareCase.objects.get_case_by_external_id(domain, self._id, USERCASE_TYPE)
 
-    def get_user_session_data(self, domain):
-        # TODO can we do this for both types of users and remove the fields from user data?
-        session_data = super(WebUser, self).get_user_session_data(domain)
-        session_data['commcare_location_id'] = self.get_location_id(domain)
-        session_data['commcare_location_ids'] = user_location_data(self.get_location_ids(domain))
-        session_data['commcare_primary_case_sharing_id'] = self.get_location_id(domain)
-        return session_data
-
 
 class FakeUser(WebUser):
     """

corehq/apps/users/tests/test_location_assignment.py

@@ -129,20 +129,15 @@ def test_create_with_location(self):
 
     def assertPrimaryLocation(self, expected):
         self.assertEqual(self.user.location_id, expected)
-        self.assertEqual(self.user.get_user_data(self.domain).get('commcare_location_id'), expected)
         self.assertTrue(expected in self.user.assigned_location_ids)
 
     def assertAssignedLocations(self, expected_location_ids):
         user = CommCareUser.get(self.user._id)
         self.assertListEqual(user.assigned_location_ids, expected_location_ids)
-        actual_ids = user.get_user_data(self.domain).get('commcare_location_ids', '')
-        actual_ids = actual_ids.split(' ') if actual_ids else []
-        self.assertListEqual(actual_ids, expected_location_ids)
 
     def assertNonPrimaryLocation(self, expected):
         self.assertNotEqual(self.user.location_id, expected)
         self.assertTrue(expected in self.user.assigned_location_ids)
-        self.assertTrue(expected in self.user.get_user_data(self.domain).get('commcare_location_ids'))
 
 
 class WebUserLocationAssignmentTest(TestCase):

corehq/apps/users/tests/test_user_data.py

@@ -38,7 +38,6 @@ def make_web_user(self):
     def test_user_data_accessor(self):
         user = self.make_commcare_user()
         user_data = user.get_user_data(self.domain)
-        self.assertEqual(user_data['commcare_project'], self.domain)
         user_data.update({
             'cruise': 'control',
             'this': 'road',
@@ -61,12 +60,10 @@ def test_web_users(self):
 
         # Each domain has a separate user_data object
         self.assertEqual(web_user.get_user_data(self.domain).to_dict(), {
-            'commcare_project': self.domain,
             'commcare_profile': '',
             'what_domain_is_it': 'domain 1',
         })
         self.assertEqual(web_user.get_user_data('another_domain').to_dict(), {
-            'commcare_project': 'another_domain',
             'commcare_profile': '',
             'what_domain_is_it': 'domain 2',
         })
@@ -198,14 +195,12 @@ def test_add_and_remove_profile(self):
         # Custom user data profiles get their data added to metadata automatically for mobile users
         user_data = self.init_user_data({'yearbook_quote': 'Not all who wander are lost.'})
         self.assertEqual(user_data.to_dict(), {
-            'commcare_project': self.domain,
             'commcare_profile': '',
             'yearbook_quote': 'Not all who wander are lost.',
         })
 
         user_data.profile_id = 'blues'
         self.assertEqual(user_data.to_dict(), {
-            'commcare_project': self.domain,
             'commcare_profile': 'blues',
             'favorite_color': 'blue',  # provided by the profile
             'yearbook_quote': 'Not all who wander are lost.',
@@ -214,7 +209,6 @@ def test_add_and_remove_profile(self):
         # Remove profile should remove it and related fields
         user_data.profile_id = None
         self.assertEqual(user_data.to_dict(), {
-            'commcare_project': self.domain,
             'commcare_profile': '',
             'yearbook_quote': 'Not all who wander are lost.',
         })
@@ -317,12 +311,5 @@ def test_no_location_info_in_user_data_when_no_location_assigned(self):
         user_data = self.user.get_user_data(self.domain)
 
         self.assertEqual(user_data.to_dict(), {
-            'commcare_project': self.domain,
             'commcare_profile': '',
         })
-
-    def test_change_data_provided_by_system_will_raise_user_data_error(self):
-        user_data = self.user.get_user_data(self.domain)
-
-        with self.assertRaisesMessage(UserDataError, "'commcare_location_id' cannot be set directly"):
-            user_data['commcare_location_id'] = self.loc1.location_id

corehq/apps/users/user_data.py

@@ -1,25 +1,17 @@
-from django.conf import settings
 from django.contrib.auth.models import User
 from django.db import models, transaction
 from django.utils.functional import cached_property
 from django.utils.translation import gettext as _
-from corehq.apps.users.util import user_location_data
 
 from dimagi.utils.chunked import chunked
 
 from corehq.apps.custom_data_fields.models import (
-    COMMCARE_LOCATION_ID,
-    COMMCARE_LOCATION_IDS,
-    COMMCARE_PRIMARY_CASE_SHARING_ID,
-    COMMCARE_PROJECT,
     PROFILE_SLUG,
     CustomDataFieldsProfile,
     CustomDataFieldsDefinition,
     is_system_key,
 )
 
-LOCATION_KEYS = {COMMCARE_LOCATION_ID, COMMCARE_LOCATION_IDS, COMMCARE_PRIMARY_CASE_SHARING_ID}
-
 
 class UserDataError(Exception):
     ...
@@ -67,36 +59,15 @@ def save(self):
 
     @property
     def _provided_by_system(self):
-        provided_data = {
+        return {
             **(self.profile.fields if self.profile else {}),
             PROFILE_SLUG: self.profile_id or '',
-            COMMCARE_PROJECT: self.domain,
         }
 
-        def _add_location_data():
-            if self._couch_user.get_location_id(self.domain):
-                provided_data[COMMCARE_LOCATION_ID] = self._couch_user.get_location_id(self.domain)
-                provided_data[COMMCARE_PRIMARY_CASE_SHARING_ID] = self._couch_user.get_location_id(self.domain)
-
-            if self._couch_user.get_location_ids(self.domain):
-                provided_data[COMMCARE_LOCATION_IDS] = user_location_data(
-                    self._couch_user.get_location_ids(self.domain))
-
-        # Some test don't have an actual user existed
-        # Web User don't store location fields in user data
-        if (self._couch_user or not settings.UNIT_TESTING) and self._couch_user.is_commcare_user():
-            _add_location_data()
-
-        return provided_data
-
-    @property
-    def _system_keys(self):
-        return set(self._provided_by_system.keys()) | LOCATION_KEYS
-
     def to_dict(self):
         return {
             **self._schema_defaults,
-            **{k: v for k, v in self._local_to_user.items() if k not in self._system_keys},
+            **{k: v for k, v in self._local_to_user.items() if k not in self._provided_by_system},
             **self._provided_by_system,
         }
 
@@ -189,7 +160,7 @@ def get(self, key, default=None):
         return self.to_dict().get(key, default)
 
     def __setitem__(self, key, value):
-        if key in self._system_keys:
+        if key in self._provided_by_system:
             if value == self._provided_by_system.get(key, object()):
                 return
             raise UserDataError(_("'{}' cannot be set directly").format(key))
@@ -209,17 +180,17 @@ def update(self, data, profile_id=...):
             self.profile_id = profile_id
         for k, v in data.items():
             if k != PROFILE_SLUG:
-                if v or k not in self._system_keys:
+                if v or k not in self._provided_by_system:
                     self[k] = v
         return original != self.to_dict() or original_profile != self.profile_id
 
     def __delitem__(self, key):
-        if key in self._system_keys:
+        if key in self._provided_by_system:
             raise UserDataError(_("{} cannot be deleted").format(key))
         del self._local_to_user[key]
 
     def pop(self, key, default=...):
-        if key in self._system_keys:
+        if key in self._provided_by_system:
             raise UserDataError(_("{} cannot be deleted").format(key))
         try:
             ret = self._local_to_user[key]