OpenEMR / Code Review / #243 Care Coordination Module

Brady Miller - 2014-08-23

Hi,

I directed one of the "up for grabs" demos to this code to make it easy for testers:
http://www.open-emr.org/wiki/index.php/Development_Demo#192.168.1.134

I am unable to register any of the modules; get the following error when click the Register button "Failure: Module name already exists"

-brady
OpenEMR

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

ZH Healthcare - 2014-08-25

Hi,

I have removed the modules from the application.config.php file. Can you update the demo code.

GitHub Path:
https://github.com/zhhealthcare/openemr/commit/f0140177d7cb58bc221d898820f32334396af59b

Thanks & Regards
ZH HealthCare

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Brady Miller - 2014-08-25

Hi,

Demo is working now. Won't the mechanism to install modules break for a multisite installation? Have you tested it to work with multisite installation?

Please announce this code on the developer forum since only a couple of us follow the tracker items.

thanks,
-brady

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

ZH Healthcare - 2014-08-25

Hi,

We have removed the checking. Instead, adding the module name to the config is restricted if its already exists. Can you update the demo code.

GitHub Path:
https://github.com/zhhealthcare/openemr/commit/de8c1cad17cbd4d33d1bc585e14eb1fa03e0beb0

Thanks & Regards
ZH HealthCare

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Brady Miller - 2014-08-28

Hi,

The demo updates every day automatically from your github branch (CareCoordiantionV1)

Please post a request to review/test this code on the forums (note only a couple peoper monitor the tracker and hundreds monitor the forums). For a code review of this size and importance good to post in both tracker and forum.

-brady
OpenEMR

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Brady Miller - 2014-09-05

Hi,

This commit is huge with hundreds of files. I'm going to place some items here in the tracker simply for code review organization sake.

List of files changed in native OpenEMR:
interface/patient_file/summary/add_edit_issue.php
interface/super/edit_list.php
interface/usergroup/user_admin.php
interface/usergroup/usergroup_admin.php
interface/usergroup/usergroup_admin_add.php
library/options.inc.php
myportal/soap_service/server_med_rec.php
sites/default/sqlconf.php
sql/4_1_2-to-4_1_3_upgrade.sql
sql/database.sql
version.php

Attached the diff for the sql/database.sql and sql/4_1_2-to-4_1_3_upgrade.sql .

4_1_2-to-4_1_3_upgrade.sql.diff

database.sql.diff

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Brady Miller - 2014-09-05

1.There should be no changes to the sites/default/sqlconf.php file

2.sql/database.sql
----should have 31 varchar in facility_code column (NOT just 20) (beiscause option_id in list_options holds 31)
----For seq in the new Religion item in layout_options, place it at end of list in Stats tab (this is fine since not a usual question in the states and then avoid conflicting with other seq items).
----For all the lists items in list_options, you need to set a seq and increment the seq by 10's (this allows flexibility in the future)
----For all the lists items in list_options, need to ensure option_id is never more than 31 characters.
----Also suggest that the fields that hold an item a option_id stay at 31.
----What is official name of coding used in notes field in the marital list? (you will use this below in edit_list.php)
----What is official name of coding used in notes field in the drug_route list? (you will use this below in edit_list.php)
----What is official name of coding used in notes field in the religious_affiliation list? (you will use this below in edit_list.php)
----What is official name of coding used in notes field in the personal_relationship list? (you will use this below in edit_list.php)
----What is official name of coding used in notes field in the race list? (you will use this below in edit_list.php)
----What is official name of coding used in notes field in the ethnicity list? (you will use this below in edit_list.php)
----Why did you add redundant items to drug_route (the Oral, Subcutaneous and transdermal, which are already there)
----What is the Care Coordination item in address book used for? (I want to make sure you set the option_value correctly)
----How are erx_diagnosis and erx_diagnosis_name in prescriptions table being populated? (I note it's collected in EncounterccdadispatchTable.php, but do not see where it's populated)
----How are order_title, code_suffix, and profile_title in procedure_results table being used? (order_title and profile_title are not being used at all and code_suffix is not populated anywhere but collected in EncounterccdadispatchTable.php)
----Does the ccda table only support couchdb, or will it also work without couchdb?
----How is list_codes table populated? (I note it's collected in EncounterccdadispatchTable.php, but do not see where it's populated)
----How is procedure_subtest_result table populated? (I note it's collected in EncounterccdadispatchTable.php, but do not see where it's populated)
----Did you forget to add the CCDA in categories?

3.sql/4_1_2-to-4_1_3_upgrade.sql
----Deal with all related questions/comments in above sql/database/.sql review
----When modify drug_route use the title rather than the option_id
----For adding codes to notes in marital, recommend just using a IfRow2D command
----the #IfRow2D layout_options field_id religion form_id DEM is not needed since done later on
----Will defer rest of review on this until sql/database.sql is revised

4.library/options.inc.php
---- What happens if the $lres_inactive query doesn't return anything??
---- The activity flag is note working for the multiselect list (which is what race is set to). Note there are also many other datatypes there that use lists from list_options, although just having it work for single select and multi select lists should suffice.

5.interface/super/edit_list.php
----For each of the lists that is using a official code in the Notes column, need to rename to the column here: https://github.com/openemr/openemr/blob/master/interface/super/edit_list.php#L873

6.interface/usergroup/user_admin.php
----xl() around "Select Type"

7.interface/usergroup/usergroup_admin_add.php
----xl() around "Select Type"

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Brady Miller - 2014-09-05

Looking forward to code review responses and next revision.
-brady
OpenEMR

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Brady Miller - 2014-09-05

Hi ZH,

I posted the code review above. Note I focused on the database changes and the native openemr changes (I also did a lot of grepping to sort out how database changes impacted code in native openemr and zend openemr code).

-brady
OpenEMR

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Brady Miller - 2014-11-27

This post is for organization sake:
Pertinent forum:
https://sourceforge.net/p/openemr/discussion/202506/thread/b515b5bd

Pertinent ongoing demo(ZH CareCoordiantionV2 branch):
http://www.open-emr.org/wiki/index.php/Development_Demo#192.168.1.134

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

ZH Healthcare - 2015-06-18

Brady,

We have made some fixes and committed the code. Please check. There are two commits. First commit is the library files need for Care Coordination module. And the second one is the Care Coordination module files.

GitHub Path:
https://github.com/zhhealthcare/openemr/commit/450184b9c1863b9bc27a4183521735b205b0d191

Thanks & Regards
ZHHealthCare

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Brady Miller - 2015-06-19

Awesome,

Will place this at the top of my code review queue. Here's a demo that points to your new branch:
http://www.open-emr.org/wiki/index.php/Development_Demo#192.168.1.134

-brady

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Brady Miller - 2015-06-30

Starting to look at this. It's rather massive :) Nice to see you've addressed most of my previous issues. Will likely take me about a week to complete the review.
-brady

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Tony McCormick - 2015-07-07
  
  I believe we already have these items via the EMRDirect interface, right?
  
  On Tue, Jun 30, 2015 at 12:28 AM, Brady Miller bradymiller@users.sf.net
  wrote:
  
  170.314(e)(1) - View, download, and transmit to 3rd party (MU2))
  
  170.314(e)(3) - Secure messaging (MU2))
  
  Tony McCormick, CTO
  www.mi-squared.com
  Support: 866-735-0897, Direct: 713-574-6709
  My Calendar: http://bit.ly/XznvDo
  ... we have to slow down a bit to be able to speed up a lot... (me)
  
  --
  Please be aware that e-mail communication can be intercepted in
  transmission or misdirected. Please consider communicating any sensitive
  information by telephone. The information contained in this message may be
  privileged and confidential. If you are NOT the intended recipient, please
  notify the sender immediately with a copy to hipaa-security@mrsb-ltd.com and
  destroy this message.
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

ZH Healthcare - 2015-07-07

Tony,

These items are done from Patient Portal using EMR Direct service.

ZHHealthCare

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Tony McCormick - 2015-07-07

Got it. Thanks for the info.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Brady Miller - 2015-07-10

Just letting you know that the review is still ongoing. Hopefully done by the end of this weekend.
-brady

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Brady Miller - 2015-07-14

Hi,
Here's a review of the database stuff, OpenEMR script changes, and the added libraries. The Zend code scripts reviews will take longer, but the above stuff is much more important to work out on the integration front and can be revised while I review the Zend scripts (my quick glancing at the Zend code scripts is that things look solid, but haven't completed the detailed review yet of these). Onward and upward :)
-brady

interface/patient_file/summary/add_edit_issue.php
--For Severity, can there be an Unassigned? If not, recommend not defaulting to Fatal(ie. guessing lots of living patients would end up with Fatal allergies). To be consistent recommend placing Life Threatening Severity below Severe, and then placing Fatal below Life Threatening Severity.
--Don't comment out html and php code, just remove it.
--What happens to upgraded users that have entered in reactions which are not on the list? Can you think of a way to make this more palatable for upgraders? Also for the reactions list, recommend making the ID something readable (like nausea, hives, etc.).

interface/patient_file/summary/stats_full.php
--Don't comment out php code, just remove it.
--What happens to upgraded users that have entered in reactions which are not on the list? Can you think of a way to make this more palatable for upgraders? Also for the reactions list, recommend making the ID something readable (like nausea, hives, etc.).

interface/super/edit_list.php
--Ensure you incorporated this into most recent codebase on next revision. The module to add the Flow Board (Patient Tracker) made some changes here.
--What is Notes title supposed to be for the Reactions list?
--What is Notes title supposed to be for the Marital Status list?
--What is Notes title supposed to be for the Industry list?
--What is Notes title supposed to be for the Occupation list?
--What is Notes title supposed to be for the Immunization Manufacturer list?
--What is Notes title supposed to be for the Immunization Completion Status list?

interface/usergroup/user_admin.php
--For the two generate_select_list() changes, remove the e parameter from the xl() calls.(this should fix the bug in that gui)

interface/usergroup/usergroup_admin.php
--On last line edited, why did you remove trim() call from the update_password() call?

interface/usergroup/usergroup_admin_add.php
--For the one generate_select_list() change, remove the e parameter from the xl() call.(this should fix the bug in that gui)

library/log.inc
--Rather than change sqlInsertClean_audit() to sqlQueryNoLog(), change them to the sqlStatementNoLog() function instead.

library/options.inc.php
--ok

myportal/soap_service/server_med_rec.php
--ok

myportal/soap_service/server_side.php
--Is following code debugging code (ie. should it be removed?)
$fh12 = fopen(sys_get_temp_dir() . '/scriptLog2.txt', 'a');
fwrite($fh12, 'getDirectAddressList' . print_r($data, 1) . PHP_EOL);
fclose($fh12);

sites/default/sqlconf.php
--This file should not be modified

sql/4_2_0-to-4_2_1_upgrade.sql
--Be more strict when adding a new lists. For example:
-#IfNotRow2D list_options list_id lists option_id religious_affiliation
--For race list, need to follow how this was done for languages list example:
https://github.com/zhhealthcare/openemr/blob/CareCoordinationV3/sql/4_1_2-to-4_2_0_upgrade.sql#L297-L307
(my advice is a quick perl script to pump this repetitive code to paste into the upgrade script which will be very long; trying to do this by hand would likely take a long time)
--For the drug_route list, when modifying a column use the title_id (not the option_id) to avoid modifying wrong places (in case upgraders have changed the option_id codes).
--When adding new drug_list items, agree with the mechanism youi are using (using IfNotRow2Dx2() ), but start seq at 20 and increment it by 10 for the new entries.
--Have two same blocks of code with "IfMissingColumn lists severity_al"
--I'd remove this block "IfRow2D layout_options field_id religion form_id DEM", since doing this already here "IfMissingColumn patient_data religion". And to avoid messing up a prior customization by a user (where a user has added a religion entry), suggest making the name a bit different, like "religion_choice" or something like that.
--What are the different option_value settings in abook_type meant for?
--What are the external_id stuff meant for?
--Remove "#IfMissingColumn ccda emr_transfer" and "#IfMissingColumn ccda type" and simply add these when add the ccda table
--For the lists what is the meaning of setting the "is_default" setting for a list title entry?
--What happens to upgraded users that have entered in Occupations which are not on the list? Can you think of a way to make this more palatable for upgraders?
--why dropping the pid index in the patient_access_offsite table?
--Don't set a character set in tables (patient_portal_menu)
--For "#IfNotRow3D list_options list_id outcome option_id 1 codes 413322009", instead use the title_id and if it's a snomed code, then prepend it with SNOMED-CT:
--What is form_observation, form_care_plan, and form_functional_cognitive_status ?
--Why are new procedure tables needed(procedure_subtest_result table and order_title table)?

sql/database.sql
--Ensure everything here is in 4_2_0-to-4_2_1_upgrade.sql and vice versa. For example, noted no external_provider and external_org in abook_type list here. Also noted no Cancer_Diagnosis_* lists here.
--Ensure don't hard code the charset for the tables (some are set to latin1).

version.php
--ok

The supporting libraries commit:
Overall Issues:
--Isn't the "interface/modules/zend_modules/vendor" meant for OpenEMR modules that aren't Zend. Why are you placing libraries there?
--Why the ".gitignore" files?

Brings in following libraries:
SFTP
--This looks like a generic php library that is not dependent on Zend. Wouldn't this be best in the openemr/library/SFTP directory

composer
--Give me a quick description on why this is used, version, license.

dompdf
--Give me a quick description on why this is used, version, license.

doctrine
--Give me a quick description on why this is used, version, license.

Last edit: Brady Miller 2015-07-14

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

ZH Healthcare - 2015-07-14

Brady,

We will get back to you with the fixes.

Thanks & Regards
ZHHealthCare

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Brady Miller - 2015-07-16

Hi,

Here is the review of the Zend scripts:
OVERALL ISSUES:
--Remove commented out code
--When save temporary files, where are they being saves to. And are they being removed after being used? (especially the ccda stuff)
--When submit links/forms, note session stuff needs to be dealt with:
----http://www.open-emr.org/wiki/index.php/OpenEMR_System_Architecture#PHP_Sessions_and_Browser_Windows

interface/modules/zend_modules/module/Application/src/Application/Model/ApplicationTable.php
--$leaging vs $leading typo?
--use escape_limit() function (in library/formdata.inc.php) to escape the limitStart and limitEnd (as you note, they can't be binded)
--why are you doing strtolower()? (just wondering if there is a specific reason)
--Can you use the generate_id() function (in library/sql.inc) rather than creating the generateSequenceID() function?

interface/modules/zend_modules/module/Application/src/Application/Plugin/Phimail.php
--Do not copy/paste functions(then they need to be maintained in two separate places and difficult to follow for developers). Are these functions just copy/pasted?

interface/modules/zend_modules/module/Application/view/application/index/auto-suggest.phtml
--Missing a couple $this->escapeHtml calls (line with "previous" and at bottom $patient_id and $dob)
--Missing one translation call ("Previous")

interface/modules/zend_modules/module/Application/view/application/sendto/send.phtml
--html escape when echo $key_ccda
--missed html escape on $component
--Don't include trailing colon for translation constants (Direct Address:, Reason for Referral:)
--why commented out the translated labels to the radio buttons at bottom of script? If using the values to show these, then they need to be translated, which means can't use the values on the script that catches this.

interface/modules/zend_modules/module/Carecoordination/README.md
--looks like is a junk file

interface/modules/zend_modules/module/Carecoordination/src/Carecoordination/Model/CarecoordinationTable.php
--noted lots of inserts into list_options. I am assuming this is to cover things (like drug form ie. tabs) that are not yet on the list. Is this correct? Or is other stuff being put into the list_options.
--For getCodes(), does the query really need to exclude inactive entries (if using for mapping, such as ethnicity, seems like would still want to map the code even if it's not active)

interface/modules/zend_modules/module/Carecoordination/src/Carecoordination/Model/Configuration.php
--Labels not translated

interface/modules/zend_modules/module/Carecoordination/src/Carecoordination/Model/EncountermanagerTable.php
--use escape_limit() function (in library/formdata.inc.php) to escape the $data['limit_start'] and $data['results'] in the query.
--bind $pid parameter in the query
--Can you give me an example of what you store in ccda_data of ccda table for a file that is not stored in couchdb

interface/modules/zend_modules/module/Carecoordination/table.sql
--What is this??? Isn't this already covered in the database.sql and sql upgrade script?

interface/modules/zend_modules/module/Carecoordination/view/carecoordination/carecoordination/revandapprove.phtml
--Html escape all variables going to html(use your custom ZH function that you used in code above).
--Translate all labels.
--Don't translate things that should not be translated.
--Probably should run this script through a ZH internal review to clean up above issues; guessing was scripted by a newer zh developer (it's a nice script, just lots of issues with the translations and html escaping).

interface/modules/zend_modules/module/Carecoordination/view/carecoordination/carecoordination/upload.phtml
--Translate "Do You Really Want to Continue?"
--Translate "Abort"
--As above script, should run this script through a ZH internal review to clean up issues with translations and html escaping.

interface/modules/zend_modules/module/Carecoordination/view/carecoordination/carecoordination/view.phtml
--As above scripts, should run this script through a ZH internal review to clean up issues with translations and html escaping.

interface/modules/zend_modules/module/Carecoordination/view/carecoordination/encountermanager/index.phtml
--As above scripts, should run this script through a ZH internal review to clean up issues with translations and html escaping.(for example, a patients full names is getting translated...)

interface/modules/zend_modules/module/Carecoordination/view/carecoordination/setup/index.phtml
--As above scripts, should run this script through a ZH internal review to clean up issues with translations and html escaping.

interface/modules/zend_modules/module/Ccr/view/ccr/ccr/index.phtml
--As above scripts, should run this script through a ZH internal review to clean up issues with translations and html escaping.(this one has less issues than above scripts, but again noted that patient name has been translated...)

interface/modules/zend_modules/module/Ccr/view/ccr/ccr/revandapprove.phtml
--As above scripts, should run this script through a ZH internal review to clean up issues with translations and html escaping.(this one has less issues than above scripts, but for example the immunization note text and lab results are being translated...)

interface/modules/zend_modules/module/Documents/src/Documents/Controller/DocumentsController.php
--Where are documents getting saved when upload a document? What is an example of wht url in documents table looks like? Can they be seein in the OpenEMR document viewer? Would using the current OpenEMR upload function be more robust like you do in above care coordination code(see the OpenEMR function you used when getting an incoming document from phimail in you above care coordination code)?

interface/modules/zend_modules/module/Documents/src/Documents/Plugin/Documents.php
--See what needs to be done when collecting documents in the native openemr code.

interface/modules/zend_modules/module/Documents/table.sql
--What is this SELECT 1 for?

interface/modules/zend_modules/module/Immunization/src/Immunization/Model/ImmunizationTable.php
--use escape_limit() function (in library/formdata.inc.php) to escape the $form_data['limit_start'] and $form_data['results'] in the query.

interface/modules/zend_modules/module/Immunization/view/immunization/immunization/index.phtml
--missing some translations (again translating patient name...)
--don't include traling spaces or a colon in the translation constant "Total Number of Immunizations : "

interface/modules/zend_modules/module/Syndromicsurveillance/src/Syndromicsurveillance/Model/SyndromicsurveillanceTable.php
--use escape_limit() function (in library/formdata.inc.php) to escape the $start and $end in query.

interface/modules/zend_modules/module/Syndromicsurveillance/view/syndromicsurveillance/syndromicsurveillance/index.phtml
--html escape $row_code['id'] and $row_provider['value']
--shouldn't previous, next and last be translated?
--Again translated patient name...

interface/modules/zend_modules/public/index.php
--Did the code that you removed there go anywhere else (especially the security flags $sanitize_all_escapes and $fake_register_globals=false)???
--This initially seems a bit worrisome that you have several different mechanisms of authorization here. Can you exlain each one?

interface/modules/zend_modules/public/js/scripts/immunization.js
-- As an aside, your js_xl() function is very cool.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Brady Miller - 2015-07-16

Look forward to the next revision :)
thanks,
-brady

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Brady Miller - 2015-07-26

One thing that was brought to my attention regarding my review. Ignore my issue with the vendor directory.
-brady

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

ZH Healthcare - 2015-07-27

Sure. We are working on other comments. We will be able to submit the next revision by this week.

ZHHealthCare

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

ZH Healthcare - 2015-07-29

Brady,

You have asked a question on how to handle the existing reaction and occupations added through text box. Can we not just write a script to add those values to the existing list? We will run the script while executing the sql upgrade. I think that would solve the problem. What do you say?

Thanks & Regards
ZHHealthCare

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Brady Miller - 2015-07-29

Hi ZH,
That sounds like a good plan.
-brady

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Care Coordination Module

Group

Searches

Help

#243 Care Coordination Module

Related

Discussion