Medmasterpro API Review

Overview

This is too review the Medmasterpro api code at http://github.com/medmasterpro/openemr . It gets it's own wiki page because it is an extensive and exciting ongoing project.

Functions

Overview

These are all in the api directory.

Global Issues

• COMPLETED Change the 'push_notification' global to something more specific like 'device_push_notification_service'
• ACTIVE One thing we need to consider is what is being populated in the fields that are mapped to items in the list_options table. For example, some of the items in demographics and other forms. To begin with, when you create a prescription via addpresciption.php, can you provide me a sample of what you are populating the POST data fields with?

MEDMASTERPRO RESPONSE:

• Changed the 'push_notification' global to 'device_push_notification_service'
• Sample post data to add prescription:

• NEW We don't map items to the list_options table for populating POST data fields in addprescription.php script.

Core functions/scripts in the includes directory

functions.php

• COMPLETED add_escape_custom($userId) in the 2nd query of createToken() function is not wrapped with single quotes.
• COMPLETED query in validateToken() function should use binding
• COMPLETED the getUserData() function looks like it should be removed (since it is just returning results of getUsername() function)

MEDMASTERPRO RESPONSE:

• Wrapped add_escape_custom($userId) in the 2nd query of createToken() function with single quotes.
• Applied binding in query of validateToken() function.
• Removed getUserData() function.

addappointment.php

• COMPLETED Surround the entire $device_token_badge with the 'push_notification' global switch. Also need to skip the $notification_res logic when not using the 'push_notification'.
• COMPLETED In $strQuery suery, need single quotes around the add_escape_custom($patientId)
• COMPLETED All the getUserData() function does is return two separate but identical variables with the getUsername() function. Clean this up, since it appears all you need is a $user = getUsername($userId) and no need for the other variables (emr/password/username).
• COMPLETED Setting $_SESSION['authGroup'] incorrectly. Check out how it is set in library/auth.inc and set it that way. (note I don't even see where this is used in your script)
• COMPLETED Use the InsertEvent() function in library/encounter_events.inc.php to create the appointment.

MEDMASTERPRO RESPONSE:

• We don't need to check 'device_push_notification_service' on this page because it is applied in notification() function in functions.php script.
• Removed query and use InsertEvent() function to add appointments.
• Removed getUserData() function.
• Removed $_SESSION['authGroup'] variable because we are not using it anymore.
• Use the InsertEvent() function to create the appointment.

addcheckout.php

• Strip add_escape_custom() from $units = add_escape_custom($_POST['units']);
• Note that to protect against sql injection the items that are in the sql queries with the add_escape_custom() function need to be surrounded by quotes. For example, the following is needed: $strQuery1 .= " WHERE encounter = '" . add_escape_custom($visit_id) . "' AND pid = '" . add_escape_custom($patientId)."'";. Note I placed single quotes around the variables. Make sure you do that for the rest of the sql queries here.
• Note that copays are no longer stored in the billing table, but are now stored in the ar_activity and ar_session tables. This was a new change in OpenEMR 4.1.1 . Look in the OpenEMR codebase and you'll find some good examples, which you can then mimick in this script.
• Setting $_SESSION['authGroup'] incorrectly. Check out how it is set in library/auth.inc and set it that way. (note I don't even see where this is used in your script).

MEDMASTERPRO RESPONSE:

• Set $_SESSION['authUser'] variable as set in library/auth.inc script.
• Modified code as per changes made in checkout process of OpenEMR 4.1.1.

addcontactgeneral.php

• COMPLETED Setting $_SESSION['authGroup'] incorrectly. Check out how it is set in library/auth.inc and set it that way. (note I don't even see where this is used in your script).
• ACTIVE The userdata imagedata is not a feature included within OpenEMR, so unable to even see these within the main OpenEMR. Would need to discuss this feature on the forums at some point to ensure this strategy makes sense; although it seems to make sense to store them where you are and name them via timestamp to avoid overwrites.

• Storing the id/label information in list_options is definitely not the right way to go, though (would be much better to store it in the users table entry).
• Also, since you know where these files are, seems like all you need to store is the name (ie. not the path, which could change, if OpenEMR instance is placed on another server).

MEDMASTERPRO RESPONSE:

• Removed $_SESSION['authGroup'] variable.

addfacility.php

• COMPLETED Setting $_SESSION['authGroup'] incorrectly. Check out how it is set in library/auth.inc and set it that way. (note I don't even see where this is used in your script).
• COMPLETED You have $user = getUsername($userId); twice.

MEDMASTERPRO RESPONSE:

• Removed $_SESSION['authGroup'] variable.
• Removed duplicate getUsername($userId) function.

addfeesheet.php

• Setting $_SESSION['authGroup'] incorrectly. Check out how it is set in library/auth.inc and set it that way. (note I don't even see where this is used in your script).

MEDMASTERPRO RESPONSE:

• Set $_SESSION['authProvider'] and $_SESSION['authId'] variables as set in library/auth.inc script.

addinsurancecompany.php

• COMPLETED Setting $_SESSION['authGroup'] incorrectly. Check out how it is set in library/auth.inc and set it that way. (note I don't even see where this is used in your script).

MEDMASTERPRO RESPONSE:

• Removed $_SESSION['authGroup'] variable.

addlist.php

• Setting $_SESSION['authGroup'] incorrectly. Check out how it is set in library/auth.inc and set it that way. (note I don't even see where this is used in your script).
• There is a new sql column in the lists table (I just committed it to codebase on sourceforge 1 minute ago), called 'modifydate' that should be set to NOW() when create a new item and set to NOW() whenever modify the item.

MEDMASTERPRO RESPONSE:

• Removed $_SESSION['authGroup'] variable.
• Add 'modifydate' column in insert query and set to NOW().

addonotes.php

• COMPLETED Setting $_SESSION['authGroup'] incorrectly. Check out how it is set in library/auth.inc and set it that way. (note I don't even see where this is used in your script).
• ACTIVE Setting $_SESSION['authUser'] and $_SESSION['authProvider'] (see 5/12/13 Reviewer response).

MEDMASTERPRO RESPONSE:

• Set $_SESSION['authUser'] variable as set in library/auth.inc script.

REVIEWER RESPONSE (5/12/2013):

• Note that following should be set for the addOnote function: $_SESSION['authUser']=$user and $_SESSION['authProvider']=getAuthGroup($user) (The authProvider is actually the group name). And no need for the $authgroup query since you already have the $user variable.

addpatientdocument.php

• COMPLETED Setting $_SESSION['authGroup'] incorrectly. Check out how it is set in library/auth.inc and set it that way. (note I don't even see where this is used in your script).
• ACTIVE Should be using the documents class, which is rather simple(then you will support all the document related functionality, such as couchdb support, automatically). See the phimail_store() function in the library/direct_message_check.inc script for a nice example of how to do this.
• COMPLETED Use the notify_push global to ignore the device_token_badge and notification_res code elements.
• ACTIVE It also appears you are hard-coding the id_cat of 2 to be labs. Note it is better to hard-code the name of the folder that holds them rather than the id. For an example of this check out the Advanced Directives widget in the demographics.php script.

MEDMASTERPRO RESPONSE:

• Removed $_SESSION['authGroup'] variable.
• We can't use document class in api because we got data in base64_encoded format and then we decoded and create document.
• We don't need to check 'device_push_notification_service' on this page because it is applied in notification() function in functions.php script.
• Created new getIdByDocumentCatName() function in functions.php script and use name of the folder instead of hard-code 2.

REVIEWER RESPONSE (5/12/2013):

• Include the library/documents.php script and use the addNewDocument() function, which will basically do everything for you(after you copy/decode the file). Should look something like addNewDocument($doc_name,$mime_type,$file,0,$filesize,$user_owner,$patient_id,$category_id) which returns an array with some details if successful (see function header for details).
• Rather than use your getIdByDocumentCatName() function, remove your function and instead use the document_category_to_id() function in the library/documents.php script.

addpatientdocumentwithlink.php

• COMPLETED THIS APPEARS TO BE THE SAME EXACT FILE AS ABOVE???
• COMPLETED Setting $_SESSION['authGroup'] incorrectly. Check out how it is set in library/auth.inc and set it that way. (note I don't even see where this is used in your script).
• ACTIVE Should be using the documents class, which is rather simple(then you will support all the document related functionality, such as couchdb support, automatically). See the phimail_store() function in the library/direct_message_check.inc script for a nice example of how to do this.
• COMPLETED Use the notify_push global to ignore the device_token_badge and notification_res code elements.
• ACTIVE It also appears you are hard-coding the id_cat of 2 to be labs. Note it is better to hard-code the name of the folder that holds them rather than the id. For an example of this check out the Advanced Directives widget in the demographics.php script.

MEDMASTERPRO RESPONSE:

• This is not same as addpatientdocument.php. In addpatientdocument.php file, we get base64_encoded data and in this file we get link/url of file.
• Removed $_SESSION['authGroup'] variable.
• We can't use document class in api because we got data in base64_encoded format and then we decoded and create document.
• We don't need to check 'device_push_notification_service' on this page because it is applied in notification() function in functions.php script.
• Created new getIdByDocumentCatName() function in functions.php script and use name of the folder instead of hard-code 2.

REVIEWER RESPONSE (5/12/2013):

• Include the library/documents.php script and use the addNewDocument() function, which will basically do everything for you(after you copy/decode the file). Should look something like addNewDocument($doc_name,$mime_type,$file,0,$filesize,$user_owner,$patient_id,$category_id) which returns an array with some details if successful (see function header for details).
• Rather than use your getIdByDocumentCatName() function, remove your function and instead use the document_category_to_id() function in the library/documents.php script.

addpatientnotes.php

• COMPLETED Setting $_SESSION['authGroup'] incorrectly. Check out how it is set in library/auth.inc and set it that way. (note I don't even see where this is used in your script).
• COMPLETED Use the addPnote() function in library pnotes.inc script.
• COMPLETED Also, I noted you only seem to have functionality to send messages to oneself regarding a patient; note you can also send messages to other users regarding patients.

MEDMASTERPRO RESPONSE:

• Removed $_SESSION['authGroup'] variable.
• Used addPnote() function in library pnotes.inc script to add note.

addpatient.php

• COMPLETED Setting $_SESSION['authGroup'] incorrectly. Check out how it is set in library/auth.inc and set it that way. (note I don't even see where this is used in your script).
• COMPLETED Patient photo/images in OpenEMR are stored in the 'Patient Photograph' document category, which are shown in OpenEMR's patient summary screen. So, store your patient photos theres.
• ACTIVE You are inserting categories very incorrectly, which will break OpenEMR's current document screen. If you really need to add categories (which you may not need to since 'Patient Photograph' category is there by default), check out some of the previous sql upgrade scripts in the sql directory where we added some categories (for example, the Advance Directive categories); note how complicated it is, although it can definitely be done.
• ACTIVE For inserting the photes, which are essentially patient documents, use the documents class. Using it is rather simple(then you will support all the document related functionality, such as couchdb support, automatically). See the phimail_store() function in the library/direct_message_check.inc script for a nice example of how to do this.

MEDMASTERPRO RESPONSE:

• Removed $_SESSION['authGroup'] variable.
• Added patient photo in 'Patient Photograph' document category.
• We don't insert new category for patient photo.
• We can't use document class in api because we got data in base64_encoded format and then we decoded and create document.

REVIEWER RESPONSE (5/12/2013):

• Include the library/documents.php script and use the addNewDocument() function, which will basically do everything for you(after you copy/decode the file). Should look something like addNewDocument($doc_name,$mime_type,$file,0,$filesize,$user_owner,$patient_id,$category_id) which returns an array with some details if successful (see function header for details).
• Use the document_category_to_id() function in the library/documents.php script to check for the 'Patient Photograph' category (although you stated you are not looking for it or adding it above, note the code still does that).
• When adding a new category, here is an example of how to add a category from the sql upgrade script (sql/3_2_0-to-4_0_0_upgrade.sql):

INSERT INTO categories select (select MAX(id) from categories) + 1, 'Patient Photograph', , 1, rght, rght + 1 from categories where name = 'Categories';
UPDATE categories SET rght = rght + 2 WHERE name = 'Categories';
UPDATE categories_seq SET id = (select MAX(id) from categories);

addpayment.php

• COMPLETED Setting $_SESSION['authGroup'] incorrectly. Check out how it is set in library/auth.inc and set it that way. (note I don't even see where this is used in your script).
• ACTIVE My billing knowledge in OpenEMR is not very strong, but this appears good. In future, I may have ZH Healthcare look at it since they have developed a lot of OpenEMR's billing code.
• ACTIVE Modular issues (see 5/13/13 Reviewer Response).

MEDMASTERPRO RESPONSE:

• Removed $_SESSION['authGroup'] variable.
• Modified code to add copay in ar_activity and ar_session tables.

REVIEWER RESPONSE (5/12/2013):

• Need to avoid duplicating code (ie. copy/paste), so I have migrated the frontPayment() function to library/payment.inc.php. So, remove it from your script and use that instead (also placed parameters for the timestamp and auth for you to use).
• Also, for the rest of that code, can you point me to where you got it from (ie. was it copy/pasted from the library/payment.inc.php script or did you modify it). Asking because it may make sense to modularize some (or all) of this to avoid needing to duplicate this code, which is rather complicated.

classes.php

• The site variable will need to be dealt with at some point. Can do this later int he review process after have a better idea of the code flow.