Add 'Same as previous patient' button for contact information

[brentvatne]

With respect to Issue #31

Comments to consider

New dependencies in test

I went ahead and added capybara and database_cleaner as test dependencies, in order to be able to write integration tests using the capybara api.

My reasoning for this is that there was no infrastructure in place for integration tests, and this app runs very slowly on my local machine. Integration tests were important to enable me to test javascript behaviour without having to manually click around the page and wait for each load (each test would take me about 2 minutes; it takes about as long with capybara but I do not have to do it manually). The integration testing support file was adapted from PuzzleNode.

Send previous patients information along with original request

There are two options I see for getting the previous patients data:

1. Add a JSON string at the top of the file that contains all relevant information
2. Make an AJAX call when the button is pressed to fetch the patient's data

I went for the first option because it was very straightforward to implement and the asynchronous version does not add much except reduced performance and more code. We have already queried the object and loaded it into memory on each request, so I figure we might as send over all of that information with the original request, rather than query for it again if it is needed. The data overhead for adding this is minimal, especially considering the networked environment this application will be operating in. That said, if you think this is not a good idea I would be interested in hearing why.

Hide the button if there is no previous user

I couldn't decide between disabling it or hiding it, so I just picked one to move along. Any preference?

Run the tests with:
rake test TEST=test/integration/check_in_test.rb

[jordanbyron]

Hey Brent!

This is some great stuff. Here is my feedback to your questions in the main pull request. Any comments about specific code I'll do inline.

I went ahead and added capybara and database_cleaner as test dependencies, in order to be able to write integration tests using the capybara api.

Capybara is great, but why do we need database_cleaner? I'm fairly confident that if we have the tests setup correctly shouldn't the database be reset for each test?

There are two options I see for getting the previous patients data ...

I think you made the right call, but I think we can do a better job with the code. I'll give you comments inline.

Hide the button if there is no previous user

Hide works for me. Excellent choice :wink2:

---

I'm really excited to get this wrapped. I think with just a few changes we'll be ready to merge this 🎈

[jordanbyron]

I think we can do a better job with this. Since we are using helpers in this app, let's just create a method that outputs this information which will clean up the view. I'd even like if lastPatient == null if there isn't a last patient. No point in passing all that blank data if there is no patient right?

[brentvatne]

Yeah good point. I just cleaned this up using an instance method on Patient and to_json, will push in a moment.

[brentvatne]

Capybara is great, but why do we need database_cleaner? I'm fairly confident that if we have the tests setup correctly shouldn't the database be reset for each test?

I was hoping to avoid that, but it seems that when we run tests that require selenium (or any type of browser driver), the tests will not work because the database appears to be empty. The reasoning for this is documented in the capybara readme under the heading Transactions and database setup:

Some Capybara drivers need to run against an actual HTTP server. Capybara takes care of this and starts one for you in the same process as your test, but on another thread. Selenium is one of those drivers, whereas RackTest is not.

If you are using an SQL database, it is common to run every test in a transaction, which is rolled back at the end of the test, rspec-rails does this by default out of the box for example. Since transactions are usually not shared across threads, this will cause data you have put into the database in your test code to be invisible to Capybara.

Cucumber handles this by using truncation instead of transactions, i.e. they empty out the entire database after each test. You can get the same behaviour by using a gem such as database_cleaner.

The other alternative would be to monkey patch ActiveRecord::Base

It is also possible to force your ORM to use the same transaction for all threads. This may have thread safety implications and could cause strange failures, so use caution with this approach. It can be implemented in ActiveRecord through the following monkey patch:

class ActiveRecord::Base
  mattr_accessor :shared_connection
  @@shared_connection = nil

  def self.connection
    @@shared_connection || retrieve_connection
  end
end
ActiveRecord::Base.shared_connection = ActiveRecord::Base.connection

I opted for database_cleaner because it seemed well, cleaner, than monkey patching ActiveRecord. Thoughts?

[jordanbyron]

I think you made the right call. I've never been able to get selenium to work for me so I've never encountered that problem. Database cleaner was the right call. Thanks 😄

On Jan 20, 2012, at 10:49 PM, Brent Vatnereply@reply.github.com wrote:

Capybara is great, but why do we need database_cleaner? I'm fairly confident that if we have the tests setup correctly shouldn't the database be reset for each test?

I was hoping to avoid that, but it seems that when we run tests that require selenium (or any type of browser driver), the tests will not work because the database appears to be empty. The reasoning for this is documented in the capybara readme under the heading Transactions and database setup:

Some Capybara drivers need to run against an actual HTTP server. Capybara takes care of this and starts one for you in the same process as your test, but on another thread. Selenium is one of those drivers, whereas RackTest is not.

If you are using an SQL database, it is common to run every test in a transaction, which is rolled back at the end of the test, rspec-rails does this by default out of the box for example. Since transactions are usually not shared across threads, this will cause data you have put into the database in your test code to be invisible to Capybara.

Cucumber handles this by using truncation instead of transactions, i.e. they empty out the entire database after each test. You can get the same behaviour by using a gem such as database_cleaner.

The other alternative would be to monkey patch ActiveRecord::Base

It is also possible to force your ORM to use the same transaction for all threads. This may have thread safety implications and could cause strange failures, so use caution with this approach. It can be implemented in ActiveRecord through the following monkey patch:

class ActiveRecord::Base
 mattr_accessor :shared_connection
 @@shared_connection = nil

 def self.connection
   @@shared_connection || retrieve_connection
 end
end
ActiveRecord::Base.shared_connection = ActiveRecord::Base.connection

I opted for database_cleaner because it seemed well, cleaner, than monkey patching ActiveRecord. Thoughts?

---

Reply to this email directly or view it on GitHub:
#71 (comment)

[brentvatne]

I wasn't sure what to do about the branch for issue #29 waiver confirmation because I originally branched it off of this pull request due to dependencies on some of the code (integration testing). I merged it in here so you could look at it all at once. What should I have done / what are the best practices for working concurrently on multiple issues that touch some of the same code?

I had to make a few stylistic decisions on the waiver, but in the end I think this accomplishes very well the goal you had in mind. Yes, that is Mendicant red 😉 #70120B from the Mendicant University text on the homepage.

[jordanbyron]

What should I have done / what are the best practices for working concurrently on multiple issues that touch some of the same code?

That is a problem I've run into as well. What I usually do is send them over in seperate requests, and just note that one depends on the other. As best I can, I try to sperate them out. For example, I did a few pull requests for the slugger gem, and broke out some code refactoring / cleanup into one branch, then did my feature requests on different branches. I sent over the code cleanup pull request first. Once that was merged I sent over the features.

[jordanbyron]

@brentvatne can you help me out with these selenium tests? Is there anything special I need to do for them to work? I can see firefox trying to connect to localhost:63263 but the server isn't running so the tests fail. I tried running the server by running script/rails s -e test but then the tests couldn't run because they tried dropping the database but couldn't obviously because the server was using it.

[jordanbyron]

@brentvatne a few minor little changes I think we need to make:

Patient agree's to waver should disappear after "Agree" is checked. I still feel like instead of a checkbox it should be a button. And maybe be right aligned next to the text.

For some reason, if you toggle the date of birth entry mode the free form field is still disabled. Are you seeing the same problem on your end? To reproduce: Agree to the waver, and then toggle the gear next to the DOB field. 😞

⚠️ It also looks like we broke a very important set of functionality. After a patient is checked in their chart number is supposed to be displayed. Look at mom.checkin.js line 74:

if(options.lastPatientId)

Now you should be able to check options.lastPatient and if that exists just use options.lastPatient.Id. That requires some modification to your code, but I think that's pretty straightforward.

Same as previous patient works great. I think once we get these few things working we'll be ready to merge

[brentvatne]

can you help me out with these selenium tests? Is there anything special I need to do for them to work? I can see firefox trying to connect to localhost:63263 but the server isn't running so the tests fail. I tried running the server by running script/rails s -e test but then the tests couldn't run because they tried dropping the database but couldn't obviously because the server was using it.

The same thing happens for me - it opens up Firefox and, after a while of loading, it begins clicking around. The server is started automatically in the testing environment. Odd, I did not have to configure anything for this personally. Does the app usually run very slowly for you? Perhaps it is timing out.

[jordanbyron]

Does the app usually run very slowly for you? Perhaps it is timing out.

As with most rails 3.x apps the startup time does take a little while, but once the server is running there aren't any speed issues. I have no idea why the server isn't started when I run the tests. I'll have to do some more googling and see if I can figure out why these tests are failing.

[brentvatne]

@jordanbyron - be sure to pull the newest commit while trying to get Selenium working.

[jordanbyron]

Great. We are super close here. One last detail which needs to be addressed:

Right now if you click "Agree", and then forget to fill out a required field, and click "Check In", you are forced to click "Agree" a second time. We need to make sure this waiver confirmation box doesn't show in that case. There are a few ways you could do this, and if you need somewhere to start just let me know.

Thanks again for doing a kick ass job on this!

[brentvatne]

@jordanbyron Alright, fixed that! Too much happy path testing, oops. Also added integration test to protect against regression.

[jordanbyron]

🎉 🎈 Merged! I switched the driver over to webkit which runs in headless mode, and works on my computer. Thanks so much @brentvatne for working on this. Make sure you you update @sandal and @semmons99

[brentvatne]

Great! Headless mode should be much faster. Hopefully as we continue making changes we can add integration tests to each one so that we eventually build up a robust suite :) Later on I'm going to look over the commits you made before merging and maybe ask some questions in-line. After Monday's checkpoint I'll be in touch about the next issue we're going to tackle.

[jordanbyron]

Sounds great. Thanks so much for working on this!!!

Jordan

On Jan 21, 2012, at 6:24 PM, Brent Vatnereply@reply.github.com wrote:

Great! Headless mode should be much faster. Hopefully as we continue making changes we can add integration tests to each one so that we eventually build up a robust suite :) Later on I'm going to look over the commits you made before merging and maybe ask some questions in-line. After Monday's checkpoint I'll be in touch about the next issue we're going to tackle.

---

Reply to this email directly or view it on GitHub:
#71 (comment)