Added stupoli.yml and settings for adding patient forms #36
Conversation
There was a problem hiding this comment.
Overall great work! It looks like you really dug into the guts of how crispy forms and django forms work, and I am glad you were able to find a simple solution!
There are two things going on in this PR: setting up a stupoli deployment and getting the homeless/no address thing working. I would suggest strongly separating this into two PRs, one for each, and focusing on the homeless thing first, since there is some stuff we'll have to get figured out for the deployment that might take longer/be annoying.
Anyway, a few todos:
config/settings/local-*.py->config/settings/local_*.py.envs/.local/.stupoliis unused, I think.- Set
OSLER_DEFAULT_CITYand friends correctly or not at all - Potential changes to
PatientForm, definitely not hard coding index, maybe a custom widget? - A couple of stray linebreaks (
osler/core/models.py:192-193,osler/core/forms.py:114) stupoli.ymlbased onproduction-umkc.yml, add nginx (maybe)- Tests? We should be able to write selenium tests that verify this works.
Strong work!
.envs/.production/.stupoli
Outdated
| @@ -0,0 +1 @@ | |||
| DJANGO_SETTINGS_MODULE=config.settings.production-stupoli | |||
There was a problem hiding this comment.
There is generally a convention in python filenames not to use a hyphen, because you - is interpreted as a minus and as such the token production-stupoli is interpreted as production minus stupoli. Should probably be production_stupoli. Or, honestly, I'm not sure we need a separate stupoli local environment. We should maybe discuss how deployments are laid out...
I notice that I (or someone else?) did this for production-umkc, though, and so I can see why you did it that way...
osler/core/forms.py
Outdated
| self.fields['address'].widget.attrs = {'placeholder': settings.OSLER_DEFAULT_ADDRESS} | ||
| self.fields['address'].widget.attrs = {'placeholder': settings.OSLER_DEFAULT_ADDRESS, | ||
| 'id': 'address'} | ||
| self.fields['city'].widget.attrs = {'id': 'city'} |
There was a problem hiding this comment.
are these ids not prespecified by crispy forms? It seems strange we have to manually add this in. If we really do have to, you could do:
for fname in ['city', 'state', 'zip_code', ...]:
self.fields[fname].widget.attrs = {'id': fname}it would be more DRY.
There was a problem hiding this comment.
Yeah, I remember being surprised that crispy form doesnt auto generate an id too, but yeah this is a cleaner way to do it. Here's the documentation on crispy forms below:
| self.fields['pcp_preferred_zip'].widget.attrs = {'id': 'pcp preferred zip'} | ||
| if settings.OSLER_HOMELESS_OPTION: | ||
| self.helper.layout.remove('homeless') | ||
| self.helper.layout.insert(14,'homeless') |
There was a problem hiding this comment.
I'm not thrilled about this hard-coded index... maybe we should ask for the index of the homeless field first? If ever we reorder the form we'll have a problem tracking down this spot in the code.
As an aside, I wonder if we should be writing a special kind of widget that does this? I know django offers the ability to subclass widget.
There was a problem hiding this comment.
I've looked pretty hard into the documentation for crispy form, and it seems that we're going to end up having to do some amount of hard-coding... What would your ideal widget do @justinrporter ?
There was a problem hiding this comment.
So manually adding a new field to crispy forms always appends it at the end. Since the crispy form is generated from the model, we could have the homeless setting alter the model before we load the form so that it's in the right place. But I don't really know if this is bad practice or if its even possible. What do you guys think?
stupoli.yml
Outdated
| image: selenium/standalone-chrome-debug:3.141 | ||
| ports: | ||
| - 4444:4444 # Selenium | ||
| - 5900:5900 # VNC server |
There was a problem hiding this comment.
There's no reason for you to know this, but the only docker compose yml files that are used are local.yml and production-umkc.yml. We should make a production-stupoli.yml file that is based on production-umkc.yml but that includes a VM for nginx. For historical reasons the UMKC deployment's nginx is running on the host machine but this is suboptimal imo. @wwick do you agree?
There was a problem hiding this comment.
It was my idea to create production-umkc and local-umkc. I actually use local-umkc.yml a ton in order to test the UMKC settings fo hiding certain elements of the UI locally.
There was a problem hiding this comment.
I think when I was working on this issue with @ericsallinger , there's no good way to inherit .yml files, and you need to specify the settings in the .yml file. Since we modify .yml so infrequently, we don't have to repeat ourselves very often, although I agree that it's not ideal.
There was a problem hiding this comment.
local-umkc.yml a ton in order to test the UMKC settings
Great! This is very smart--actually one of the big advantages of docker is supposed to be that your local and production environments can be almost identical. We should put some thought into how to make this a bit more scalable as we expand to more institutions.
| OSLER_DEFAULT_COUNTRY = "USA" | ||
| OSLER_DEFAULT_ADDRESS = "205 East 9th St." | ||
| OSLER_HOMELESS_OPTION = True | ||
| OSLER_PHONE_NUMBER_OPTION = False |
There was a problem hiding this comment.
We should probably set these defaults correctly or not at all.
There was a problem hiding this comment.
What do you mean by setting them correctly?
…r yml loading bug by adding settings to base.py
Changes:
Bugs: