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

location data quick fix #151

Merged
merged 4 commits into from Jun 17, 2020
Merged

location data quick fix #151

merged 4 commits into from Jun 17, 2020

Conversation

czue
Copy link
Member

@czue czue commented Jun 17, 2020

So the location_data property is actually a dict which was causing DET's --location and --with-organization flags to crash for any location that actually had any custom properties set.

This quick fix just serializes it to a string going into the DB allows the import to (mostly) succeed (also needs dimagi/commcare-hq#27831).

Longer term I think we would want to either:

  1. (easier) Convert this to a JSON column in the DB (at least for postgres)
  2. (better) "Flatten" the dict, by creating a separate column for each possible field value inside the location_data dict.

@solleks one observation is that DET appears to be adding an "id" property to the dict. Is this expected and do you have a suggested way of handling it? I just left it for now.

@czue czue requested a review from solleks June 17, 2020 08:38
@@ -207,11 +207,11 @@ def get_expected_locations_results(include_parent):
],
"rows": [
["id1", "2020-04-01 21:57:26", "d1", "eid1",
"2020-04-01 21:58:23", "11.2", "ld1", "lid1", "lt1",
"2020-04-01 21:58:23", "11.2", '{"p1": "ld1", "id": "id1.location_data"}', "lid1", "lt1",
Copy link
Member Author

Choose a reason for hiding this comment

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

this is an example of the id property that gets injected

Copy link
Collaborator

Choose a reason for hiding this comment

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

It was a surprise to me, but the 'id' comes from this line:
https://github.com/dimagi/commcare-export/blob/master/commcare_export/env.py#L203

It seems to be recording the path to the dictionary within its enclosing object. I would leave it as is for now because I don't know how to avoid it for location_data without affecting its intended use.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for chasing this down!

@solleks
Copy link
Collaborator

solleks commented Jun 17, 2020

Thanks for fixing this bug.

One problem for the solution of flattening the dict is that the possible properties of location_data may not be discoverable before reading the actual locations and minilinq doesn't seem to offer a mechanism to dynamically create a new output column as properties are discovered.

A json type column does seem like a good fit, though. And hopefully that wouldn't be much work to implement.

@solleks solleks merged commit c64a329 into master Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants