caktus / rapidsms-healthcare

A Django/RapidSMS application for managing patient and healthcare provider records in a pluggable fashion.
BSD 3-Clause "New" or "Revised" License
8 stars 8 forks source link

Added contact to Provider #17

Open ghost opened 11 years ago

mlavin commented 11 years ago

Travis is failing because rapidsms isn't declared as a dependency in tox or the setup.py.

ghost commented 11 years ago

@mlavin: Thanks for the comments; I updated these things and the build passes.

mlavin commented 11 years ago

It would be less of a burden on the individual storage backends to always store this via the Django ORM (in healthcare/models.py). This doesn't really have anything to do with the actual provider data which might make it cumbersome to add to OpenMRS or other storage backends. Or would it be too odd to have a mix of the backend API and normal Django models?

ghost commented 11 years ago

In general I agree that using the ORM is the best approach, but in this case it seems to go against the concept of "dummy" storage.

mlavin commented 11 years ago

My point is mainly that this connection isn't here for the actual provider but for other RapidSMS applications that might use this project. It doesn't really belong in the backends. We are really just adding this so that other applications won't need to create this type of connection model themselves (though they are free to do so if they need additional information on it). I could be wrong. @ewheeler care to weigh in?

ghost commented 11 years ago

Ah, sorry, misunderstood the destination of the model. Thanks for clarifying.

ghost commented 11 years ago

@mlavin: Yes, all backends will need certain Django things and it probably makes sense to abstract that out into a model in healthcare itself. However, I think that this change could be made later (@ewheeler is okay with this).

Something I've been thinking about: the dummy backend should probably all Django storage (even when we have a general model), as it's meant to be as lean as possible for testing.

In f7d98c5 I propose using a unicode string rather than a long as the dummy generated uuid. I store the identifier as a character string on the Report model, and when using the dummy backend for testing I have to cast everything as a long to get a result but this isn't the case when using the Django backend. Unless there is a more preferred way of storing what essentially is a foreign key, it is more consistent to use a string here.

Using these changes I have a working integration of providers at rapidsms-nutrition.

mlavin commented 11 years ago

I don't have a problem with using a string instead of a long though it would more closely match the behavior if internally the dummy backend did the conversion of the id on the get/update lookup.

I don't agree that the dummy backend should use Django storage. Part of the point of the dummy backend is to have two working implementations of the storage APIs: one which uses the Django ORM and one that doesn't. I can agree it is probably easier to use the Django backend for testing and with SQLite the difference is probably quite small.

ghost commented 11 years ago

Ah, I mistyped - meant to say "avoid all Django storage." I am glad we're in agreement.

mlavin commented 11 years ago

I'm not sure that we are. I still feel like this connection (provider --> contact) has nothing to do with the provider itself and doesn't belong in the backend.