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

Patient demographics property #1559

Closed
wants to merge 4 commits into from

Conversation

davidmiller
Copy link
Member

Adds a shortcut method for demographics_set.get()

I had originally implemented as a property before realising that it would be infuriating to lose the data in this code:

patient.demographics.hospital_number = '1234'
patient.demographics.surname = 'Doe'
patient.demographics.save()

Thought about caching the property - but what about race conditions / other updates etc.

@fredkingham
Copy link
Contributor

fredkingham commented Jun 24, 2018

do you think a method or a property?

I think I probably want to write patient.demographics.first_name

(Supreme bike shedding, in general pro)

Update, I'm a moron who doesn't read comments.

@davidmiller
Copy link
Member Author

I think you could have it as a cached property of the Patient object - you just roll back the last commit and add a decorator...

I'm not strongly convinced either way TBH

How does Django loading foreign keys to properties work with updates to the DB ?
e.g. Episode.Patient - the data is loaded at the moment you first access it, and then never reloaded ?

We could probably roll back and do that...

@davidmiller
Copy link
Member Author

I guess we could do cached_property(get_demographics, name='demographics;) and get_demographics()

@fredkingham
Copy link
Contributor

I've gone the other way, I think this is good, I think as soon as we change it to a cached property we may almost one to change it to a OneToOneField, which we may want to anyway (genuinely don't know, I do think it wouldn't be worth the time and effort).

@fredkingham
Copy link
Contributor

closing this to move to v0.12.0

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.

3 participants