Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IA-3456 improve get or create for orgunit (duplicate on uuid) #1661

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion iaso/api/instances.py
Original file line number Diff line number Diff line change
Expand Up @@ -686,6 +686,7 @@ def instance_logs(self, request, pk=None, logId=None):

def import_data(instances, user, app_id):
project = Project.objects.get_for_user_and_app_id(user, app_id)
default_version = project.account.default_version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's possible to not have a default version on an account. I think that situation would probably blow up anyways, but not sure if we should consider it here 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beygorghor I think we already had to code special cases when there is no default_version so it can happen right ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed, the frontend allows no default version


for instance_data in instances:
uuid = instance_data.get("id", None)
Expand All @@ -709,7 +710,7 @@ def import_data(instances, user, app_id):
if str(tentative_org_unit_id).isdigit():
instance.org_unit_id = tentative_org_unit_id
else:
org_unit = OrgUnit.objects.get(uuid=tentative_org_unit_id)
org_unit = OrgUnit.objects.filter(version=default_version).get(uuid=tentative_org_unit_id)
instance.org_unit = org_unit

instance.form_id = instance_data.get("formId")
Expand Down
37 changes: 35 additions & 2 deletions iaso/api/mobile/org_units.py
Original file line number Diff line number Diff line change
Expand Up @@ -337,11 +337,19 @@ def bbox_merge(a: Optional[tuple], b: Optional[tuple]) -> Optional[tuple]:
)


def orgunits_for_default_version_qs(default_version):
"The uuid is supposed to be unique per source_version"
return OrgUnit.objects.filter(version=default_version)


def import_data(org_units, user, app_id):
new_org_units = []
project = Project.objects.get_for_user_and_app_id(user, app_id)
org_units = sorted(org_units, key=get_timestamp)

project = Project.objects.get_for_user_and_app_id(user, app_id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate of line 347?

default_version = project.account.default_version

for org_unit in org_units:
uuid = org_unit.get("id", None)
latitude = org_unit.get("latitude", None)
Expand All @@ -351,7 +359,29 @@ def import_data(org_units, user, app_id):
if latitude and longitude:
altitude = org_unit.get("altitude", 0)
org_unit_location = Point(x=longitude, y=latitude, z=altitude, srid=4326)
org_unit_db, created = OrgUnit.objects.get_or_create(uuid=uuid)

org_unit_defaults = {
"custom": True,
"validated": False,
"name": org_unit.get("name", None),
"version": project.account.default_version,
}

if not user.is_anonymous:
org_unit_defaults["creator"] = user

created_at = None
if org_unit.get("created_at", None):
created_at = timestamp_to_utc_datetime(int(org_unit.get("created_at", None)))
else:
created_at = org_unit.get("created_at", None)
if created_at:
org_unit_defaults["created_at"] = created_at
if org_unit_location:
org_unit_defaults["location"] = org_unit_location
org_unit_db, created = orgunits_for_default_version_qs(default_version).get_or_create(
uuid=uuid, defaults=org_unit_defaults
)

if created:
org_unit_db.custom = True
Expand All @@ -368,14 +398,17 @@ def import_data(org_units, user, app_id):
if str.isdigit(parent_id):
org_unit_db.parent_id = parent_id
else:
parent_org_unit = OrgUnit.objects.get(uuid=parent_id)
parent_org_unit = orgunits_for_default_version_qs(default_version).get(uuid=parent_id)
org_unit_db.parent_id = parent_org_unit.id
# TODO should org_unit_db.parent_id is "ok" for the project/account ?

org_unit_type_id = org_unit.get(
"orgUnitTypeId", None
) # there exist versions of the mobile app in the wild with both orgUnitTypeId and org_unit_type_id
if not org_unit_type_id:
org_unit_type_id = org_unit.get("org_unit_type_id", None)

# TODO should we verify the org_unit_type is "ok" for the project/account ?
org_unit_db.org_unit_type_id = org_unit_type_id

t = org_unit.get("created_at", None)
Expand Down
5 changes: 3 additions & 2 deletions iaso/api/org_units.py
Original file line number Diff line number Diff line change
Expand Up @@ -777,6 +777,7 @@ def import_data(org_units: List[Dict], user, app_id):
project = Project.objects.get_for_user_and_app_id(user, app_id)
if project.account.default_version.data_source.read_only:
raise Exception("Creation of org unit not authorized on default data source")
default_version = project.account.default_version
for org_unit in org_units:
uuid = org_unit.get("id", None)
latitude = org_unit.get("latitude", None)
Expand All @@ -786,7 +787,7 @@ def import_data(org_units: List[Dict], user, app_id):
if latitude and longitude:
altitude = org_unit.get("altitude", 0)
org_unit_location = Point(x=longitude, y=latitude, z=altitude, srid=4326)
org_unit_db, created = OrgUnit.objects.get_or_create(uuid=uuid)
org_unit_db, created = OrgUnit.objects.filter(version=default_version).get_or_create(uuid=uuid)

if created:
org_unit_db.custom = True
Expand All @@ -801,7 +802,7 @@ def import_data(org_units: List[Dict], user, app_id):
if str.isdigit(parent_id):
org_unit_db.parent_id = parent_id
else:
parent_org_unit = OrgUnit.objects.get(uuid=parent_id)
parent_org_unit = OrgUnit.objects.filter(version=default_version).get(uuid=parent_id)
org_unit_db.parent_id = parent_org_unit.id

# there exist versions of the mobile app in the wild with both orgUnitTypeId and org_unit_type_id
Expand Down
40 changes: 40 additions & 0 deletions iaso/tests/api/test_mobile_orgunits.py
Original file line number Diff line number Diff line change
Expand Up @@ -320,3 +320,43 @@ def test_post_to_retrieve_list_with_wrong_id(self):
self.client.force_authenticate(self.user)
response = self.client.get(BASE_URL, data={APP_ID: BASE_APP_ID, IDS: f"{self.goku.id},-1,{self.goten.id}"})
self.assertEqual(response.status_code, 404)

def test_post_to_create_orgunit_uuid_not_so_unique(self):
"""
make sure we don't cross walls between account when using uuid
"""
OTHER_BASE_APP_ID = "the.gallagher.brothers"

other_account = Account.objects.create(name="Oasis")
other_project = Project.objects.create(name="Reformation", app_id=OTHER_BASE_APP_ID, account=other_account)
other_user = self.create_user_with_profile(
username="Liam", account=other_account, permissions=["iaso_org_units"]
)
other_sw_source = DataSource.objects.create(name="Morning Glory")
other_sw_source.projects.add(other_project)
other_sw_version_1 = SourceVersion.objects.create(data_source=other_sw_source, number=1)
other_account.default_version = other_sw_version_1
other_account.save()

NOT_SO_UNIQUE_ID = "notsounique"

self.client.force_authenticate(other_user)
response = self.client.post(
BASE_URL + "?app_id=" + OTHER_BASE_APP_ID,
data=[{"name": "Don't Look Back in Anger", "created_at": 1, "id": NOT_SO_UNIQUE_ID}],
format="json",
)

orgunit_other = response.json()

self.client.force_authenticate(self.user)
response = self.client.post(
BASE_URL + "?app_id=" + BASE_APP_ID,
data=[{"name": "Don't Look Back in Anger", "created_at": 1, "id": NOT_SO_UNIQUE_ID}],
format="json",
)

orgunit = response.json()

self.assertEqual(orgunit_other[0]["name"], orgunit[0]["name"])
self.assertNotEqual(orgunit_other[0]["id"], orgunit[0]["id"])
Loading