Cht-core: Remove `parent: {}` in `district_hospitals`

[estellecomment]

The district_hospitals in christianaid2 have parent: {}, rather than no parent field at all in 2.x systems.

Figure out if it's the case for all 0.4 systems, and if so, whether it's necessary or can be deleted without issues.

[sglangevin]

I've also seen parent: null in 2.x systems, FWIW. That didn't seem to have any negative effect.

[mandric]

The parent field is optional for district_hospital on all versions, it can be missing (not used) or of type national_hospital.

[estellecomment]

Thanks for the inputs!
parent: null would behave about the same as no parent field, so that's cool.
parent: national_hospital is cool too.

Ok, so if it's all the same, I would rather have no parent field than empty field, because we're likely to do tests like
if (doc.parent) { ... }
instead of the full-blown
if (doc.parent && Object.keys(doc.parent).length !== 0) { ... } or tests for each field : if (doc.parent.blah)
and then have crashes because of unexpectedly missing fields.

So AIs :

• write a migration to remove empty parent fields
• check all the places which create places, and fix them if they're setting empty parents

[mandric]

It's hard/expensive to mandate/enforce that docs use undefined instead of null on a property. We'd have to look at every property on a doc and either convert or reject null values. Up until now we've been treating null and undefined as equivalent, I think we can continue writing code that also makes that assumption without too much pain?

> if (undefined) { console.log('truthy');}
undefined
> if (null) { console.log('truthy');}
undefined

[estellecomment]

@mandric, yes, that case is fine. I agree that making a difference between the two would be kinda splitting hairs.

The case that's more of a problem is doc.parent: {}, because then doc.parent is truthy (but empty!). That's the case that I want to get rid of.

[mandric]

I still think it's easier to handle that case in the application code rather than enforce it on the database level. Underscore also has an _.isEmpty function we could use?

> u.isEmpty(null)
true
> u.isEmpty({})
true
> u.isEmpty(undefined)
true

[estellecomment]

Yeah... Ideally, both. I agree that we can live with it. It's just one of these little things that can trip you up, so getting rid of it will be more robust.
Do you not like the change because it's too low-priority to be worth it, or because you don't like migrations in general, or something else?

[mandric]

To get rid of it you would need to write a database constraint that looks at every property or parent properties of every doc, a migration is not a fix. And it's more error prone for the application to have an expectation for a specific type rather than check for it.

[abbyad]

It seems worth it to figure out a convention since we want to handle additional levels at a later time, as well as a implicit root node. At a minimum we would want to make sure we document this well for developers AND tech leads.

[mandric]

That's even more reason to leave it flexible, because it's still being developed, and let the application handle it.

[estellecomment]

it's more error prone for the application to have an expectation for a specific type rather than check for it.

I see if (!foo.bar) all over the place, and pretty much never if (_.empty(foo.bar)). So we are not checking for it. And I think it's too much trouble to check for it: now you have to keep in mind that parent may be empty. What about contact, can it be empty? What about all the other fields? I can't count on future-me to remember which fields could be empty, or to bother to check for all fields to be potentially empty. So I think that's error-prone.

you would need to write a database constraint that looks at every property or parent property of every doc

Yeah, that's cool.
More precisely, given that we don't update the parent property, I would probably ignore it in the constraint, and just check for the bottom-level property. The migration would have already dealt with higher levels.

it's still being developed

It's always being developed... When will it not be in development! :D

[mandric]

I don't see a big deal with having all three values (null, undefined and {}) behave the same way for a given property. Also seems more useful to centralize some code to interact with our documents that allows the application to deal with those values in the same way. Like var record = new Record(doc); record.hasParent(); or some centralized javascript API around querying Places or People is more useful/flexible, and doesn't require any changes on the db level, and nothing breaks. The Places/People APIs won't be in development forever, at some point we'll have a structure that works for 99% of our needs, I think we are pretty close we just need to support a few more things like Marc mentioned.

[estellecomment]

Hmm, this nitpicking discussion has not disappeared, how annoying.
@garethbowen @browndav @SCdF @alxndrsn please express your opinion.

Thumbs-up this post if you think we should clean up parent: {} into parent: null (or other falsy value) when upgrading 0.4 -> 2.x.

[estellecomment]

Thumbs-up this post if you think we leave it as is and deal with variations of format in app code.

[alxndrsn]

PRs for review at: https://github.com/medic/medic-api/pull/95 & https://github.com/medic/medic-webapp/pull/2622

Will need a final commit to update API after the PRs are merged.

[garethbowen]

Loving the integration tests. Added a few comments to the api PR.

[estellecomment]

AT needs to test the remove-empty-parents migration.

I suggest

• create a district hospital. Edit it in futon to set parent: {}
• run the remove-empty-parents migration
• check the doc now has no parent field