Feature patient history #66

Merged
merged 39 commits into from Jan 18, 2012

Conversation

Projects
None yet
3 participants
@kbl
Collaborator

kbl commented Jan 13, 2012

Not fully functional pull request mainly for code review sake.

I have some cases which aren't fully clear for me:

  • could user be "checked in" in many treatment areas?
  • if so which one should be presented on assignmet desk (which readio button should be checked?)
  • what should I do with assignments when admin is about to 'reset distribution'?
@jordanbyron

This comment has been minimized.

Show comment
Hide comment
@jordanbyron

jordanbyron Jan 13, 2012

Owner

Hey Marcin,

Here are the answers to your questions:

could user be "checked in" in many treatment areas?

I think we might be using the same term, checked in, for different meanings. When a patient is checked in, that just means a Patient record has been generated and they are given a "chart number" aka Patient#id. Then they go to the Assignment Desk where they are routed to one Treatment Area. So at any given time, patients should only be assigned to one treatment area.

That might seem weird, but we need to keep the relationship a one to many because radiology is an exception. Some patients require x-rays be taken before they go onto their final treatment area. In those cases, they are assigned to two treatment areas: radiology and any other treatment area. I know that's confusing, and if you need me to explain a bit better just let me know.

if so which one should be presented on assignmet desk (which readio button should be checked?

The radio button should show the treatment area assignment which the patient hasn't been check out of and is not radiology. The radiology check box should be checked if an assignment exists for radiology that hasn't been checked out.

what should I do with assignments when admin is about to 'reset distribution'?

Good question. Let me explain the purpose for that function first: Reset Distribution was created because not all patients which check in actually go through the entire clinic and check out. Some have to leave, others just get tired of waiting. So at the end of the first day, we want to reset our reports so those assignments are back to zero.

Now we might be able to fix this by scoping those reports to a given day. So show all PatientAssignment records created on a provided date (which should default to today). Then we can remove the Reset Distribution function entirely. Sound good?


I hope to check out your code sometime today, but I've got a few other things to do first. Thanks for working on this!!! 👍

Owner

jordanbyron commented Jan 13, 2012

Hey Marcin,

Here are the answers to your questions:

could user be "checked in" in many treatment areas?

I think we might be using the same term, checked in, for different meanings. When a patient is checked in, that just means a Patient record has been generated and they are given a "chart number" aka Patient#id. Then they go to the Assignment Desk where they are routed to one Treatment Area. So at any given time, patients should only be assigned to one treatment area.

That might seem weird, but we need to keep the relationship a one to many because radiology is an exception. Some patients require x-rays be taken before they go onto their final treatment area. In those cases, they are assigned to two treatment areas: radiology and any other treatment area. I know that's confusing, and if you need me to explain a bit better just let me know.

if so which one should be presented on assignmet desk (which readio button should be checked?

The radio button should show the treatment area assignment which the patient hasn't been check out of and is not radiology. The radiology check box should be checked if an assignment exists for radiology that hasn't been checked out.

what should I do with assignments when admin is about to 'reset distribution'?

Good question. Let me explain the purpose for that function first: Reset Distribution was created because not all patients which check in actually go through the entire clinic and check out. Some have to leave, others just get tired of waiting. So at the end of the first day, we want to reset our reports so those assignments are back to zero.

Now we might be able to fix this by scoping those reports to a given day. So show all PatientAssignment records created on a provided date (which should default to today). Then we can remove the Reset Distribution function entirely. Sound good?


I hope to check out your code sometime today, but I've got a few other things to do first. Thanks for working on this!!! 👍

Gemfile
+ gem 'highline', :require => false
+ gem 'faker', :require => false
+ gem 'rubyzip', :require => false
+ gem 'spork-testunit', :require => false

This comment has been minimized.

@jordanbyron

jordanbyron Jan 15, 2012

Owner

Why did you include spork-testunit as a development dependency? In general I try to keep all the dependencies to a minimum.

@jordanbyron

jordanbyron Jan 15, 2012

Owner

Why did you include spork-testunit as a development dependency? In general I try to keep all the dependencies to a minimum.

This comment has been minimized.

@kbl

kbl Jan 16, 2012

Collaborator

Good point. I thought that it wouldn't be possible to run spork without such dependency in dev scope which seems to be false.

@kbl

kbl Jan 16, 2012

Collaborator

Good point. I thought that it wouldn't be possible to run spork without such dependency in dev scope which seems to be false.

+ end
+
+ end
+end

This comment has been minimized.

@jordanbyron

jordanbyron Jan 15, 2012

Owner

Adding a new controller is probably overkill for just one view. I think an action in the existing Admin::PatientsController would do the trick. Something like Admin::PatientsController#history

@jordanbyron

jordanbyron Jan 15, 2012

Owner

Adding a new controller is probably overkill for just one view. I think an action in the existing Admin::PatientsController would do the trick. Something like Admin::PatientsController#history

This comment has been minimized.

@kbl

kbl Jan 16, 2012

Collaborator

There is no such thing as Admin::PatientsController, I assume that you thought about 'plain' PatientsController (without Admin namespace). Talking about latter - on the one hand you are right and separate controller seems to be redundant, but on the other hand now PatientsController has ~ 150 lines which in my opinion is far from skinny so I created separated one.

Or maybe I misunderstood you and you were talking about renaming Admin::PatientAssigmentsController to Admin::PatientsController (thus having 2 separate PatientsController's in separate namespaces)?

@kbl

kbl Jan 16, 2012

Collaborator

There is no such thing as Admin::PatientsController, I assume that you thought about 'plain' PatientsController (without Admin namespace). Talking about latter - on the one hand you are right and separate controller seems to be redundant, but on the other hand now PatientsController has ~ 150 lines which in my opinion is far from skinny so I created separated one.

Or maybe I misunderstood you and you were talking about renaming Admin::PatientAssigmentsController to Admin::PatientsController (thus having 2 separate PatientsController's in separate namespaces)?

This comment has been minimized.

@jordanbyron

jordanbyron Jan 17, 2012

Owner

There is no such thing as Admin::PatientsController

Ahhh you are right. I'm sorry about that. Let's create an Admin::PatientsController and make sure we move the edit, update, and destroy methods from PatientsController to Admin::PatientsController. That is how I would do it today and it simplifies both controllers.

Admin::PatientAssigmentsController doesn't exist right? Because I don't know what that controller would do. We only need to display patient assignments in Admin::PatientsController#history

@jordanbyron

jordanbyron Jan 17, 2012

Owner

There is no such thing as Admin::PatientsController

Ahhh you are right. I'm sorry about that. Let's create an Admin::PatientsController and make sure we move the edit, update, and destroy methods from PatientsController to Admin::PatientsController. That is how I would do it today and it simplifies both controllers.

Admin::PatientAssigmentsController doesn't exist right? Because I don't know what that controller would do. We only need to display patient assignments in Admin::PatientsController#history

This comment has been minimized.

@kbl

kbl Jan 17, 2012

Collaborator

Done, edit, destroy, history moved to Admin::PatientsController. During testing I've noticed error (same happens also on master branch): https://gist.github.com/1627050

Fix it on this feature branch or maybe in separate pull request?

@kbl

kbl Jan 17, 2012

Collaborator

Done, edit, destroy, history moved to Admin::PatientsController. During testing I've noticed error (same happens also on master branch): https://gist.github.com/1627050

Fix it on this feature branch or maybe in separate pull request?

This comment has been minimized.

@jordanbyron

jordanbyron Jan 17, 2012

Owner

During testing I've noticed error (same happens also on master branch): https://gist.github.com/1627050

File a ticket for the error and give me a bit more context. IE where exactly that error is happening, etc. Thanks 😄

@jordanbyron

jordanbyron Jan 17, 2012

Owner

During testing I've noticed error (same happens also on master branch): https://gist.github.com/1627050

File a ticket for the error and give me a bit more context. IE where exactly that error is happening, etc. Thanks 😄

This comment has been minimized.

@kbl

kbl Jan 18, 2012

Collaborator

Issue created, details provided in ticket - #69

@kbl

kbl Jan 18, 2012

Collaborator

Issue created, details provided in ticket - #69

@@ -14,11 +14,21 @@ def edit
def update
patient = Patient.find(params[:id])
-
- patient.update_attributes(params[:patient])
+ patient.update_attributes({ radiology: params[:patient][:radiology] })

This comment has been minimized.

@jordanbyron

jordanbyron Jan 15, 2012

Owner

This is going to be tricky, but I want to remove the radiology column from Patient and instead create a PatientAssignment for radiology. That way we can track when they went through the radiology department and display that on their history page.

It's tricky because radiology is a special case since it is the only treatment area which patients can be routed to in addition to their final destination. I explained this a bit in my initial response so I would get into it again, but we may have to unpack the best way to tackle this problem together.

@jordanbyron

jordanbyron Jan 15, 2012

Owner

This is going to be tricky, but I want to remove the radiology column from Patient and instead create a PatientAssignment for radiology. That way we can track when they went through the radiology department and display that on their history page.

It's tricky because radiology is a special case since it is the only treatment area which patients can be routed to in addition to their final destination. I explained this a bit in my initial response so I would get into it again, but we may have to unpack the best way to tackle this problem together.

This comment has been minimized.

@kbl

kbl Jan 16, 2012

Collaborator

To be honest I was thinking about same thing during weekend and want to suggest this change (;

@kbl

kbl Jan 16, 2012

Collaborator

To be honest I was thinking about same thing during weekend and want to suggest this change (;

+
+ private
+
+ def check_in(patient)

This comment has been minimized.

@jordanbyron

jordanbyron Jan 15, 2012

Owner

Since we are only using this check_in method once it probably doesn't need to exist. Just make sure Patient#check_in doesn't fail if given a nil value for area.

So line 18 would look like:

patient.check_in TreatmentArea.find(params[:patient][:checked_in_at])
@jordanbyron

jordanbyron Jan 15, 2012

Owner

Since we are only using this check_in method once it probably doesn't need to exist. Just make sure Patient#check_in doesn't fail if given a nil value for area.

So line 18 would look like:

patient.check_in TreatmentArea.find(params[:patient][:checked_in_at])

This comment has been minimized.

@kbl

kbl Jan 16, 2012

Collaborator

Such line will cause error if params[:patient][:checked_in_at].nil? == true (ActiveRecord::RecordNotFound: Couldn't find MODEL without an ID). So I could do it slightly different by passing id not actual object:

patient.check_in(params[:patient][:checked_in_at])
@kbl

kbl Jan 16, 2012

Collaborator

Such line will cause error if params[:patient][:checked_in_at].nil? == true (ActiveRecord::RecordNotFound: Couldn't find MODEL without an ID). So I could do it slightly different by passing id not actual object:

patient.check_in(params[:patient][:checked_in_at])

This comment has been minimized.

@jordanbyron

jordanbyron Jan 17, 2012

Owner

Such line will cause error

You're right. Change that to:

patient.check_in TreatmentArea.find_by_id(params[:patient][:checked_in_at])
@jordanbyron

jordanbyron Jan 17, 2012

Owner

Such line will cause error

You're right. Change that to:

patient.check_in TreatmentArea.find_by_id(params[:patient][:checked_in_at])
app/models/patient.rb
@@ -103,16 +106,26 @@ def procedures_grouped
end
def check_out(area)
+ raise "Can't check out" if area != checked_in_at

This comment has been minimized.

@jordanbyron

jordanbyron Jan 15, 2012

Owner

We don't want to do this. Patients can be checked out even if they haven't been assigned to a treatment area.

@jordanbyron

jordanbyron Jan 15, 2012

Owner

We don't want to do this. Patients can be checked out even if they haven't been assigned to a treatment area.

This comment has been minimized.

@kbl

kbl Jan 16, 2012

Collaborator

Sure, I wasn't certain about this before your initial comment on this pull request.

@kbl

kbl Jan 16, 2012

Collaborator

Sure, I wasn't certain about this before your initial comment on this pull request.

app/models/patient.rb
end
end
+ def check_in(area)

This comment has been minimized.

@jordanbyron

jordanbyron Jan 15, 2012

Owner

The name of this method is wrong. It should probably be assign since that is what it does: It assigns a patient to a given area.

@jordanbyron

jordanbyron Jan 15, 2012

Owner

The name of this method is wrong. It should probably be assign since that is what it does: It assigns a patient to a given area.

This comment has been minimized.

@kbl

kbl Jan 16, 2012

Collaborator

Good point. I always struggle, among others, with proper naming ):

@kbl

kbl Jan 16, 2012

Collaborator

Good point. I always struggle, among others, with proper naming ):

app/models/patient.rb
end
end
+ def check_in(area)
+ raise 'Already checked in!' if checked_in_at

This comment has been minimized.

@jordanbyron

jordanbyron Jan 15, 2012

Owner

Again we don't want to raise errors if a patient is already assigned to an area. Patients can be re-routed several times throughout their visit, and the system should allow that

@jordanbyron

jordanbyron Jan 15, 2012

Owner

Again we don't want to raise errors if a patient is already assigned to an area. Patients can be re-routed several times throughout their visit, and the system should allow that

This comment has been minimized.

@kbl

kbl Jan 16, 2012

Collaborator

So lets assume following case:

  1. Check in Radiology & Surgery - creating assignment #1 (#a1 surgery) and #a2 (radiology)
  2. Check in Hygiene - creating #a3
  3. Check in Surgery - creating #a4
  4. Check out Surgery

Is it valid? If so:

  • on which assignment check_out_at should be set? #a1? #a4? or maybe both of them?
  • which of them #a1/#a4 or #a3 should be showed in assignment desk (which radio button should be checked)?
@kbl

kbl Jan 16, 2012

Collaborator

So lets assume following case:

  1. Check in Radiology & Surgery - creating assignment #1 (#a1 surgery) and #a2 (radiology)
  2. Check in Hygiene - creating #a3
  3. Check in Surgery - creating #a4
  4. Check out Surgery

Is it valid? If so:

  • on which assignment check_out_at should be set? #a1? #a4? or maybe both of them?
  • which of them #a1/#a4 or #a3 should be showed in assignment desk (which radio button should be checked)?

This comment has been minimized.

@jordanbyron

jordanbyron Jan 17, 2012

Owner

Let's make sure we stop using the phrase "Check in" when we really mean "Assigned". It just confuses these discussions and I want to make sure we are both talking about the same thing. To recap: Patients are "Checked in" when they first come to the clinic and their Patient record is created. They are then "Assigned" to a TreatementArea

So the only way a patient is assigned is through the assignment desk: AssignmentDeskController#update. There are two types of scenarios which the assignment desk needs to handle:

  1. A patient hasn't been assigned to any treatment area (patient. assignments.not_checked_out.empty? == true)
  2. A patient is already assigned to a treatment area and possibly radiology

For the first case, all the radio boxes and the radiology box are unchecked when the form is generated. Then the assignment desk volunteer checks one of the treatment area radio boxes and possibly the radiology check box. In that case, a PatientAssignment record is created for the selected TreatmentArea and, if the radiology check box is selected, one for TreatmentArea.radiology.

The second case we need to potentially change where that patient is assigned. So if "John Smith" was originally assigned to "Surgery", but then re-assigned to "Hygiene", his Patient#assign method would first check if he has an existing, open assignment:

def assign(treatment_area)
  return unless treatement_area
  assignment = assignments.not_checked_out.not_radiology.first
  assignment ||= assignments.build

  assignment.treatment_area = treatment_area
  assignment.save
end

So in this case we would never have more than one "non-radiology" treatment area assignment open at a time. And in the end, Patient#check_out would set check_out_at on all open assignments.

NOTE: Patients should be "checked out" of radiology via PatientsController#export_to_dexis_file

@jordanbyron

jordanbyron Jan 17, 2012

Owner

Let's make sure we stop using the phrase "Check in" when we really mean "Assigned". It just confuses these discussions and I want to make sure we are both talking about the same thing. To recap: Patients are "Checked in" when they first come to the clinic and their Patient record is created. They are then "Assigned" to a TreatementArea

So the only way a patient is assigned is through the assignment desk: AssignmentDeskController#update. There are two types of scenarios which the assignment desk needs to handle:

  1. A patient hasn't been assigned to any treatment area (patient. assignments.not_checked_out.empty? == true)
  2. A patient is already assigned to a treatment area and possibly radiology

For the first case, all the radio boxes and the radiology box are unchecked when the form is generated. Then the assignment desk volunteer checks one of the treatment area radio boxes and possibly the radiology check box. In that case, a PatientAssignment record is created for the selected TreatmentArea and, if the radiology check box is selected, one for TreatmentArea.radiology.

The second case we need to potentially change where that patient is assigned. So if "John Smith" was originally assigned to "Surgery", but then re-assigned to "Hygiene", his Patient#assign method would first check if he has an existing, open assignment:

def assign(treatment_area)
  return unless treatement_area
  assignment = assignments.not_checked_out.not_radiology.first
  assignment ||= assignments.build

  assignment.treatment_area = treatment_area
  assignment.save
end

So in this case we would never have more than one "non-radiology" treatment area assignment open at a time. And in the end, Patient#check_out would set check_out_at on all open assignments.

NOTE: Patients should be "checked out" of radiology via PatientsController#export_to_dexis_file

app/models/patient.rb
+ assignments.create(treatment_area: area)
+ end
+
+ def checked_in_at

This comment has been minimized.

@jordanbyron

jordanbyron Jan 15, 2012

Owner

This too is probably not the right name for the data it returns. But more importantly I don't think we need this method.

@jordanbyron

jordanbyron Jan 15, 2012

Owner

This too is probably not the right name for the data it returns. But more importantly I don't think we need this method.

This comment has been minimized.

@kbl

kbl Jan 16, 2012

Collaborator

+1 to name.

This method is used in 2 places:

  • in views to check valid radio button (showing to which TreatmentArea user is subscribed)
  • in tests
@kbl

kbl Jan 16, 2012

Collaborator

+1 to name.

This method is used in 2 places:

  • in views to check valid radio button (showing to which TreatmentArea user is subscribed)
  • in tests
app/models/patient_assignment.rb
+
+ def check_out
+ self[:checked_out_at] = Time.now
+ save

This comment has been minimized.

@jordanbyron

jordanbyron Jan 15, 2012

Owner

Those two lines can be replaced with:

update_attribute(:checked_out_at, Time.now)
@jordanbyron

jordanbyron Jan 15, 2012

Owner

Those two lines can be replaced with:

update_attribute(:checked_out_at, Time.now)

This comment has been minimized.

@kbl

kbl Jan 16, 2012

Collaborator

Good point!

@kbl

kbl Jan 16, 2012

Collaborator

Good point!

config/routes.rb
@@ -33,6 +33,7 @@
resources :assignment_desk
match '/patients/:id/print' => 'patients#print', :as => :print_chart
match '/patients/:patient_id/export' => 'patients#export_to_dexis_file', :as => :export_to_dexis_file
+ match '/patients/:patient_id/history' => 'admin/patient_assignments#show', :as => :patient_history

This comment has been minimized.

@jordanbyron

jordanbyron Jan 15, 2012

Owner

This route should be in the /admin namespace

@jordanbyron

jordanbyron Jan 15, 2012

Owner

This route should be in the /admin namespace

This comment has been minimized.

@kbl

kbl Jan 16, 2012

Collaborator

+1
I've some problems with this so I've put it outside this namespace, I'll change this.

@kbl

kbl Jan 16, 2012

Collaborator

+1
I've some problems with this so I've put it outside this namespace, I'll change this.

test/test_helper.rb
- Time.parse "#{[y, m, d] * '-'} #{time}"
- else
- Date.strptime(date, "%m/%d/%Y")
+require 'spork'

This comment has been minimized.

@jordanbyron

jordanbyron Jan 15, 2012

Owner

I asked above why you introduced spork, and if you wanted to add a new test dependency to the project that is something we should talk about first. Any new infrastructure you add to the project I have to learn and maintain.

Additionally, adding that new dependency in with a feature complicates matters as this branch is now doing two things:

  1. Adding patient history to the project
  2. Introducing a new test dependency (Spork)

Please remove spork from this branch, and if you want to discuss adding it to the project create a new ticket in the issue tracker.

@jordanbyron

jordanbyron Jan 15, 2012

Owner

I asked above why you introduced spork, and if you wanted to add a new test dependency to the project that is something we should talk about first. Any new infrastructure you add to the project I have to learn and maintain.

Additionally, adding that new dependency in with a feature complicates matters as this branch is now doing two things:

  1. Adding patient history to the project
  2. Introducing a new test dependency (Spork)

Please remove spork from this branch, and if you want to discuss adding it to the project create a new ticket in the issue tracker.

This comment has been minimized.

@kbl

kbl Jan 16, 2012

Collaborator

I'll remove this dependency just before sending final pull request ok? Such dependency speeds up a little bit testing.

@kbl

kbl Jan 16, 2012

Collaborator

I'll remove this dependency just before sending final pull request ok? Such dependency speeds up a little bit testing.

This comment has been minimized.

@practicingruby

practicingruby Jan 16, 2012

Collaborator

For maintenance reasons, it's important to be careful not to introduce things like this mixed in with functional changes. It's reasonable to suggest things that will improve a project's testing environment, but just mixing in tools that you like to use in a functional patch is bad form. As a general rule of thumb for contributing to all open source projects: isolate environment changes from functional changes, and don't be surprised if environment changes are met with "no thanks" :)

@practicingruby

practicingruby Jan 16, 2012

Collaborator

For maintenance reasons, it's important to be careful not to introduce things like this mixed in with functional changes. It's reasonable to suggest things that will improve a project's testing environment, but just mixing in tools that you like to use in a functional patch is bad form. As a general rule of thumb for contributing to all open source projects: isolate environment changes from functional changes, and don't be surprised if environment changes are met with "no thanks" :)

test/unit/patient_assignment_test.rb
+ def test_factory_should_build_valid_assignment
+ a = Factory.build(:patient_assignment)
+ assert a.valid?
+ end

This comment has been minimized.

@jordanbyron

jordanbyron Jan 15, 2012

Owner

Maybe this is just a placeholder, but this test doesn't test anything specific about the project. All it's testing is that our factories are setup and satisfy validation requirements. Not something we need to be testing.

@jordanbyron

jordanbyron Jan 15, 2012

Owner

Maybe this is just a placeholder, but this test doesn't test anything specific about the project. All it's testing is that our factories are setup and satisfy validation requirements. Not something we need to be testing.

This comment has been minimized.

@kbl

kbl Jan 16, 2012

Collaborator

I'm going to fill this with appropriate test cases, but in my opinion such test case if it's failing points out exact place in code causing problems (e.g. adding some validations to PatientAssignment causing other tests to fail)

@kbl

kbl Jan 16, 2012

Collaborator

I'm going to fill this with appropriate test cases, but in my opinion such test case if it's failing points out exact place in code causing problems (e.g. adding some validations to PatientAssignment causing other tests to fail)

This comment has been minimized.

@practicingruby

practicingruby Jan 16, 2012

Collaborator

While I suppose that's reasonable, any time I've introduced a validations-related problem into my tests it's been immediately obvious what the problem was. Have you been in a situation where it was difficult to find the source of the issue without this kind of test? If so, can you provide an example?

@practicingruby

practicingruby Jan 16, 2012

Collaborator

While I suppose that's reasonable, any time I've introduced a validations-related problem into my tests it's been immediately obvious what the problem was. Have you been in a situation where it was difficult to find the source of the issue without this kind of test? If so, can you provide an example?

This comment has been minimized.

@kbl

kbl Jan 16, 2012

Collaborator

Assuming that we are doing TDD described situation couldn't happen, in such case this kind of tests are redundant - I've just removed it in my local repo.

@kbl

kbl Jan 16, 2012

Collaborator

Assuming that we are doing TDD described situation couldn't happen, in such case this kind of tests are redundant - I've just removed it in my local repo.

This comment has been minimized.

@practicingruby

practicingruby Jan 16, 2012

Collaborator

Even if you're not doing strict TDD, it's reasonable to assume that you're at least running the tests before committing anything. That means it'd be easy to track down problems to a particular commit. Assuming that something did fly past and break the build, git bisect would find problems like these easily.

I'm on the fence about this. It does a bit more than just testing the framework, but it's such a trivial test that its results might be obvious anyway. What it does do is make the issue clearer, but it also feels pretty pedantic :)

Thanks for explaining, anyway.

@practicingruby

practicingruby Jan 16, 2012

Collaborator

Even if you're not doing strict TDD, it's reasonable to assume that you're at least running the tests before committing anything. That means it'd be easy to track down problems to a particular commit. Assuming that something did fly past and break the build, git bisect would find problems like these easily.

I'm on the fence about this. It does a bit more than just testing the framework, but it's such a trivial test that its results might be obvious anyway. What it does do is make the issue clearer, but it also feels pretty pedantic :)

Thanks for explaining, anyway.

@jordanbyron

This comment has been minimized.

Show comment
Hide comment
@jordanbyron

jordanbyron Jan 15, 2012

Owner

@kbl I went through most of your code and gave you a TON of feedback 😁 Please don't be overwhelmed I think most of it is fairly easy stuff. If anything doesn't make sense just let me know. Also don't be afraid to ask @sandal or @semmons99 for feedback on your code. Thanks for working on this 😄

Owner

jordanbyron commented Jan 15, 2012

@kbl I went through most of your code and gave you a TON of feedback 😁 Please don't be overwhelmed I think most of it is fairly easy stuff. If anything doesn't make sense just let me know. Also don't be afraid to ask @sandal or @semmons99 for feedback on your code. Thanks for working on this 😄

@kbl

This comment has been minimized.

Show comment
Hide comment
@kbl

kbl Jan 17, 2012

Collaborator

I'll create new pull request with refactored code

Collaborator

kbl commented Jan 17, 2012

I'll create new pull request with refactored code

@kbl kbl closed this Jan 17, 2012

@jordanbyron

This comment has been minimized.

Show comment
Hide comment
@jordanbyron

jordanbyron Jan 17, 2012

Owner

@kbl you didn't need to close this request. Any new commits you push to your fork are automatically added to this pull request. Plus I haven't responded to a bunch of your comments and we'd have to start over on a new request.

I'm going to re-open this pull request and close the new one you sent over so we don't have to start over again from scratch.

Owner

jordanbyron commented Jan 17, 2012

@kbl you didn't need to close this request. Any new commits you push to your fork are automatically added to this pull request. Plus I haven't responded to a bunch of your comments and we'd have to start over on a new request.

I'm going to re-open this pull request and close the new one you sent over so we don't have to start over again from scratch.

@jordanbyron jordanbyron reopened this Jan 17, 2012

@jordanbyron jordanbyron referenced this pull request Jan 17, 2012

Closed

Feature patient history #68

@kbl

This comment has been minimized.

Show comment
Hide comment
@kbl

kbl Jan 18, 2012

Collaborator

Please take a look, I hope that now it's ready to be merged (:

Collaborator

kbl commented Jan 18, 2012

Please take a look, I hope that now it's ready to be merged (:

@jordanbyron

This comment has been minimized.

Show comment
Hide comment
@jordanbyron

jordanbyron Jan 18, 2012

Owner

@kbl thanks for kicking butt on this. Hopefully I'll have some time today to look at this and if we're close I'll merge it into master. 🎉

Owner

jordanbyron commented Jan 18, 2012

@kbl thanks for kicking butt on this. Hopefully I'll have some time today to look at this and if we're close I'll merge it into master. 🎉

- = f.label :assigned_treatment_area_id, area.name, :value => area.id
+ - @areas.reject {|ar| ar.radiology? }.each do |area|
+ = radio_button :patient_params, :assigned_to, area.id, checked: @patient.assigned_to?(area)
+ = label :assigned_treatment_area_id, area.name

This comment has been minimized.

@jordanbyron

jordanbyron Jan 18, 2012

Owner

Labels should always be associated with an input element, especially check boxes and radio buttons. This allows users to click the label to toggle the buttons, and not have to position the mouse over the much smaller checkbox.

This even applies to text fields / areas. You can see my fix for this here: 3df3777

@jordanbyron

jordanbyron Jan 18, 2012

Owner

Labels should always be associated with an input element, especially check boxes and radio buttons. This allows users to click the label to toggle the buttons, and not have to position the mouse over the much smaller checkbox.

This even applies to text fields / areas. You can see my fix for this here: 3df3777

@jordanbyron jordanbyron merged commit fca3dc4 into jordanbyron:master Jan 18, 2012

@jordanbyron

This comment has been minimized.

Show comment
Hide comment
@jordanbyron

jordanbyron Jan 18, 2012

Owner

Wahoooo!!!! Thanks for making this feature a reality. I think this ended up being more complicated than we both expected but I'm really glad you kept working and got everything done.

Now we get to party! 🎈 🎆 🎈 🎉 🎈

Owner

jordanbyron commented Jan 18, 2012

Wahoooo!!!! Thanks for making this feature a reality. I think this ended up being more complicated than we both expected but I'm really glad you kept working and got everything done.

Now we get to party! 🎈 🎆 🎈 🎉 🎈

end
end
+ def assign(assigned_to_area_id, assigned_to_radiology)

This comment has been minimized.

@jordanbyron

jordanbyron Jan 18, 2012

Owner

I ended up re-writing this method. Check out my code in this commit: 654b98f

I don't know if it's much better, but there is definitely a code smell here which I will need to address soon.

@jordanbyron

jordanbyron Jan 18, 2012

Owner

I ended up re-writing this method. Check out my code in this commit: 654b98f

I don't know if it's much better, but there is definitely a code smell here which I will need to address soon.

@kbl

This comment has been minimized.

Show comment
Hide comment
@kbl

kbl Jan 18, 2012

Collaborator

Great! Nice to hear that code is approved (:

Do this pull request solves also #26? If not I must misunderstood sth deeply (;

Collaborator

kbl commented Jan 18, 2012

Great! Nice to hear that code is approved (:

Do this pull request solves also #26? If not I must misunderstood sth deeply (;

@jordanbyron

This comment has been minimized.

Show comment
Hide comment
@jordanbyron

jordanbyron Jan 18, 2012

Owner

Do this pull request solves also #26?

Yes it does. I forgot we had a separate ticket for that. Thanks!

Owner

jordanbyron commented Jan 18, 2012

Do this pull request solves also #26?

Yes it does. I forgot we had a separate ticket for that. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment