Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Adding the notes and patient history #392

Closed
wants to merge 64 commits into from

Conversation

tangollama
Copy link
Member

Addresses issues #32 and #239

tangollama and others added 30 commits March 15, 2016 13:18
Now, we’re associating to a visit always.
Checked in for collaboration purposes.
I HAVE MADE FIRE!!!

Seriously though, the patient-notes feature is functional.
…e patient history.

@jkleinsc I want to talk to you about this change.
…tes-feature-try-again

# Conflicts:
#	app/patients/edit/template.hbs
Aligning the date of birth display with the implied standard from the
procedure and diagnosis references.
@jkleinsc
Copy link
Member

@tangollama unless I am missing something, it looks like your acceptance test doesn't really test the functionality of notes. It should test crud operations on notes, not just test that the modal shows up.

I needed it for a computed property in patient-note.
Added the translations to the patient note and a type-ahead search for
the physician list.
deletePatientNote and showDeletePatientNote
authoredBy computed property
Added an insert, update, and delete assertions.
@tangollama
Copy link
Member Author

@jkleinsc I've expanded the test suite, addressed several translation issues (including injecting the i18n service into the model), and found and fixed a new bugs along the way. I think this is ready to go. The big question is how we can run the node script for updates. Do we need a release note process?

An assertion failed in the travis build but passed in my environment.
I’m guessing that’s an execution issue on my part. Hoping this resolves
it.
The test passes locally. Hoping it will checked in as well.
@tangollama
Copy link
Member Author

Boom! @jkleinsc: MERGE. THAT. BRANCH!
gif

@jglovier
Copy link
Member

LOL well played gif @tangollama. 👏 😆

PS: to put your gifs inline, just use the markdown image syntax: ![name](url). I went ahead and edited it for you. 😉

@jkleinsc
Copy link
Member

@tangollama settle down... its not like I'm busy with something else right now! LOL!
I'm not thrilled with the UI on the "behalf of" input, but if @jglovier is ok, I guess I'm ok with it, but I don't like the undefined in the background either (undefined on behalf of test).
screen shot 2016-03-28 at 4 14 58 pm

@tangollama
Copy link
Member Author

Cool. As always, I'm open to instruction and recommendations.

Typed or dictated imperfectly on my iPhone (717.385.9970)

On Mar 28, 2016, at 5:17 PM, John Kleinschmidt [email protected] wrote:

@tangollama settle down... its not like I'm busy with something else right now! LOL!
I'm not thrilled with the UI on the "behalf of" input, but if @jglovier is ok, I guess I'm ok with it, but I don't like the undefined in the background either (undefined on behalf of test).


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

Per issues pointed out by @jkleinsc, now resolved the size of the On
Behalf Of input as well as the “undefined” issue with the background
display. Two-way data binding for the win!

Now… let’s merge this sucker.
@jglovier
Copy link
Member

I'm not thrilled with the UI on the "behalf of" input, but if @jglovier is ok, I guess I'm ok with it, but I don't like the undefined in the background either (undefined on behalf of test).

Oooo yeah, I didn't see that. @jkleinsc thanks for the screenshot (ALWAYS BE SCREENSHOTING 😉)

That definitely needs some help, although we could take one of two approaches here:

  1. punt on styling that as it's an issue with modal styling in general and fix in a new branch
  2. add the fixes for modal styling to this branch and knock it out here

I'm open to either. @tangollama which would you prefer?

@tangollama
Copy link
Member Author

@jglovier @jkleinsc I fixed the styling issue and the "undefined" text in the background last night. Behold!!

screen shot 2016-03-29 at 6 58 52 am

@jglovier
Copy link
Member

@tangollama awesome! I'd call that Good Enough™ for this branch. I'll address modal styling overall in a new PR.

:shipit:

@jkleinsc
Copy link
Member

Looks good @tangollama. I am going to merge this PR in

@jkleinsc jkleinsc closed this in e182cad Mar 30, 2016
@jglovier
Copy link
Member

@jkleinsc FYI when you merge a request that way the contributor never gets credit for the contributions as they all become your commits. Maybe not a big deal (especially if you are just doing it in certain cases), but ongoing that will be frustrating to contributors if we use that as a pattern.

@jkleinsc
Copy link
Member

@jglovier I do this for PRs that have lots of commits because I don't want to bring all those commit messages over. I guess we could ask contributors to squash the commits on their end and then we could just merge the PR. If you have other thoughts on how we can better do this, I'm all ears.

Actually I would love a Github feature where I can merge a PR and squash the commits into one commit that credits the author! That would make me 😀😀😀😀😀!

@jglovier
Copy link
Member

jglovier commented Apr 1, 2016

Actually I would love a Github feature where I can merge a PR and squash the commits into one commit that credits the author! That would make me 😀😀😀😀😀!

@jkleinsc ask and ye shall receive: https://github.com/blog/2141-squash-your-commits 😄

@jkleinsc
Copy link
Member

jkleinsc commented Apr 1, 2016

@jglovier I think I shall do a happy dance: My Reaction

@stukalin stukalin deleted the notes-feature-try-again branch August 29, 2018 07:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants