openhealthcare / elcid

elCID deployment for UCLH
http://elcid.openhealthcare.org.uk
Other
7 stars 3 forks source link

1521 can we change elcid.models.appointment type to use choices #1576

Closed pacharanero closed 6 years ago

pacharanero commented 6 years ago

closes #1521

image

fredkingham commented 6 years ago

@pacharanero can we have choices being both the same value. Its a style and consistency choice more than anything.

We don't use django choices in quite the same way as django as we only ever use the last field. So to make sure developers understand what's stored in the db/shown on the front end, we always use the first and last field in the individual choices field with the same values.

David wrote a helper function to help, because who likes typing? http://opal.openhealthcare.org.uk/docs/reference/core_fields/

pacharanero commented 6 years ago

The helper function is neat, although much more 'magical' than just writing both bits out longhand. A few weeks ago I wouldn't have known what was going on had I read

    APPOINTMENT_CHOICES = enum(
        "Doctor Clinic review appointment",
        "Nurse Clinic review appointment")

in the code. Whereas:

    APPOINTMENT_CHOICES = [
        ("Doctor Clinic review appointment", "Doctor Clinic review appointment"),
        ("Nurse Clinic review appointment", "Nurse Clinic review appointment")
    ]

is just plain old Django, you can google it and work out what's going on.

davidmiller commented 6 years ago

You don't think you would have found from opal.core.fields import enum and then googled openhealthcare opal enum ?

pacharanero commented 6 years ago

You don't think you would have found from opal.core.fields import enum and then googled openhealthcare opal enum ?

Maybe, maybe not. Obvious things are only obvious with a certain amount of context. Many new Opal devs will not yet have this context. I'm in between.

For the difference it makes to the readability of the code, there's probably not a great advantage to using enum IMO. If it was a larger list we'd be using a FreeTextOrForeignKeyField and LookupList anyway.

fredkingham commented 6 years ago

👍