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

The cancer is gone -- phase 1 #38

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

The cancer is gone -- phase 1 #38

wants to merge 5 commits into from

Conversation

justinrporter
Copy link
Contributor

@justinrporter justinrporter commented Jan 18, 2021

Phase 1 generates a new class, PatientPhoneNumber, that represents a phone number for a patient. Adding phone numbers is now done with a '+' sign on the patient detail view.

Phase 2 will actually remove the old phone numbers, after we copy everything over using the python shell.

  • Add PatientPhoneNumber with link to Patient object
  • Add phone_number to User
  • Add PatientPhoneForm,
  • Add package phonenumber_field for handling phone numbers.
  • PHONENUMBER_DEFAULT_REGION controls default country code
  • modify local.yml to add delegated, cached maybe improving MacOS performance?

@justinrporter justinrporter added the enhancement New feature or request label Jan 18, 2021
@justinrporter justinrporter self-assigned this Jan 18, 2021
@justinrporter justinrporter changed the title The cancer is gone The cancer is gone -- phase 1 Jan 18, 2021
@justinrporter justinrporter requested a review from wwick January 18, 2021 05:14
- local_postgres_data:/var/lib/postgresql/data
- local_postgres_data_backups:/backups
- local_postgres_data:/var/lib/postgresql/data:cached
- local_postgres_data_backups:/backups:cached
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How often is the backup updated. Does it make sense to cache it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm totally honest, I have no idea. According to that article, the difference should be imperceptible to the user. I haven't really pushed on it that much but after I did this (and restarted Docker) it really improved the performance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to say to uncache the backups but keep postgresql cached, since the backup isn't read from very often.

@zhouminerva
Copy link
Member

Hahahahahahaha nice. Looks great

Copy link
Member

@wwick wwick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like these changes a lot

- local_postgres_data:/var/lib/postgresql/data
- local_postgres_data_backups:/backups
- local_postgres_data:/var/lib/postgresql/data:cached
- local_postgres_data_backups:/backups:cached
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to say to uncache the backups but keep postgresql cached, since the backup isn't read from very often.

@wwick
Copy link
Member

wwick commented Jan 29, 2021

I want to wait to merge this until phase 2 is done so that there isn't confusion about duplicate phone number fields

@justinrporter
Copy link
Contributor Author

Totally agree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants