Overview:
1) The concept is very nice. Still some basics to attend to (see below specifics), but will be a nice addition to the codebase when ready (also need to ensure no functionality from the precription report (and dispensations part of it) is lost).
interface/main/left_nav.php:
--move the !$GLOBALS['disable_prescriptions'] flag from the menu into your report (ie. it should only be turning off the prescription report, not all the others)
interface/reports/clinical_reports.php:
--missing these recent additions from interface/reports/prescriptions_report.php: http://github.com/bradymiller/openemr/commit/cf07394c4f4eb1f6facefd9a217f5db6be66d84f http://github.com/openemr/openemr/commit/212bc5a8137c4ce8dc98e3a2e50123c762ca6b54
--throw an error if age is not an integer
-- line 127, translate 'All Facilities'
--Place the !$GLOBALS['disable_prescriptions'] flag at line 170 (the select list item for prescriptions)
--Translate all title in the Option selector (Diagnosis etc.)
--Translate 'Unassigned' in the gender selector
--Do not grab the gender lists directly, use the generate_select_list() (we have this new function rather than the previously used function, generate_form_field()) function in library/options.inc.php. grep code to see instances of its use. Extremely important to never grab a list directly or you lose auto validation and internationalization. Note that function will replace about 10 or so lines of your code there.
--Translate ethnicity slector 'Unassigned'
--Again, use generate_select_list() for ehtnicity list
--Why are you setting your main variables multiple time ($age_from etc.). One time at top of script should suffice.
--What's with the hard-coded dates in r.date_modified >=2010-01-01
--NEVER use mysql native function calls (mysql_query($sqlstmt) or mysql_fetch_array); only use functions in library/sql.inc see other code for a multitude of correct examples to do a simple sql query: http://www.openmedsoftware.org/wiki/Development_Policies#MySQL_connections
--very nice use of generate_display_field() though, I was starting to lose hope :) (should also do this for the ehtnicity and gender output)
thanks,
-brady
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I went the the functionality part of this with Thomas as well. Here's the basic results
1) Diagnosis needs to be gotten from encounter data as well as medical issues, may want to be able to report those separately
2) The term Procedures should be Service Codes to be consistent, as the term Procedures are now used for Lab Orders and other kinds of CPOE type functions
3) Some of the other functionality is not actually working reliably on his test system (or mine).
--Tony
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
An important aside is none of the search functioning is working; I could get nothing to return for nay search (except that all pt get returned when click a 'diagnosis' search).
My contributions are:
----INCORPORATED NEW SECURITY METHODS ((set global reverse magic quotes flag, set no fake globals flag, use sql binding/placemakers(stop sql-injection), use htmlspecialchars(stop xss attacks)) Note no need for formdata.inc.php functions anymore when use this stuff.
----FIXED incorrect demographics2.php links
----CHANGED FULL PHP tags, <?php instead of <?
----USED generate_display_field() for output of a list item.
----USED dropdown_facility function
Code and testing was very solid. I committed your report to sourceforge with following change:
1) Added back the Rx report since this has some specific functionality for the Dispensary (Lot numbers etc.)
2) Added an additional commit that fixed two very minor bugs.
Thanks for the contribution (also please update the wiki NIST testing page)
-brady
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I have downloaded this patch for review - Tony
Thomas, here's a code review:
Overview:
1) The concept is very nice. Still some basics to attend to (see below specifics), but will be a nice addition to the codebase when ready (also need to ensure no functionality from the precription report (and dispensations part of it) is lost).
interface/main/left_nav.php:
--move the !$GLOBALS['disable_prescriptions'] flag from the menu into your report (ie. it should only be turning off the prescription report, not all the others)
interface/reports/clinical_reports.php:
--missing these recent additions from interface/reports/prescriptions_report.php:
http://github.com/bradymiller/openemr/commit/cf07394c4f4eb1f6facefd9a217f5db6be66d84f
http://github.com/openemr/openemr/commit/212bc5a8137c4ce8dc98e3a2e50123c762ca6b54
--throw an error if age is not an integer
-- line 127, translate 'All Facilities'
--Place the !$GLOBALS['disable_prescriptions'] flag at line 170 (the select list item for prescriptions)
--Translate all title in the Option selector (Diagnosis etc.)
--Translate 'Unassigned' in the gender selector
--Do not grab the gender lists directly, use the generate_select_list() (we have this new function rather than the previously used function, generate_form_field()) function in library/options.inc.php. grep code to see instances of its use. Extremely important to never grab a list directly or you lose auto validation and internationalization. Note that function will replace about 10 or so lines of your code there.
--Translate ethnicity slector 'Unassigned'
--Again, use generate_select_list() for ehtnicity list
--Why are you setting your main variables multiple time ($age_from etc.). One time at top of script should suffice.
--What's with the hard-coded dates in r.date_modified >=2010-01-01
--NEVER use mysql native function calls (mysql_query($sqlstmt) or mysql_fetch_array); only use functions in library/sql.inc see other code for a multitude of correct examples to do a simple sql query:
http://www.openmedsoftware.org/wiki/Development_Policies#MySQL_connections
--very nice use of generate_display_field() though, I was starting to lose hope :) (should also do this for the ehtnicity and gender output)
thanks,
-brady
I went the the functionality part of this with Thomas as well. Here's the basic results
1) Diagnosis needs to be gotten from encounter data as well as medical issues, may want to be able to report those separately
2) The term Procedures should be Service Codes to be consistent, as the term Procedures are now used for Lab Orders and other kinds of CPOE type functions
3) Some of the other functionality is not actually working reliably on his test system (or mine).
--Tony
Patient Lists 2
Procedure report is kept unchanged, while a new Services Codes report has been added.
Thomas,
An important aside is none of the search functioning is working; I could get nothing to return for nay search (except that all pt get returned when click a 'diagnosis' search).
To deal with other code issues, I've contributed quite a bit to your code, and put the patch here. To see what changes I made from yours, can see my commit in my github branch here:
http://github.com/bradymiller/openemr/commits/thomas_report_3
Specifically my commit is:
http://github.com/bradymiller/openemr/commit/9c0ea0d0b6594b71f956211f70bc98bd22649385
My contributions are:
----INCORPORATED NEW SECURITY METHODS ((set global reverse magic quotes flag, set no fake globals flag, use sql binding/placemakers(stop sql-injection), use htmlspecialchars(stop xss attacks)) Note no need for formdata.inc.php functions anymore when use this stuff.
----FIXED incorrect demographics2.php links
----CHANGED FULL PHP tags, <?php instead of <?
----USED generate_display_field() for output of a list item.
----USED dropdown_facility function
Thread and links to examples to explain the new security implementations can be found here:
http://sourceforge.net/projects/openemr/forums/forum/202506/topic/3530656
Stuff you still need to do:
--Incorporate this recent additions from interface/reports/prescriptions_report.php:
http://github.com/openemr/openemr/commit/212bc5a8137c4ce8dc98e3a2e50123c762ca6b54
--Please explain real quick the need/use for the date range stuff (I'm confused by it)
--HAVE THE SEARCHING ACTUALLY WORK
-brady
Patient Lists 3
pasted review if this code here:
http://sourceforge.net/projects/openemr/forums/forum/202506/topic/3702210
Resubmission
PatientList - Adhering NIST Standards
Hi,
Patient List Inconsistencies are resolved and have attached the patch for the same. Patientlist Github source https://github.com/visolve-selvi/openemr/commit/7c6a1b26e869051694ac9596a69a6f839baa5b74
Thanks
ViCarePlus Team
Wow,
Code and testing was very solid. I committed your report to sourceforge with following change:
1) Added back the Rx report since this has some specific functionality for the Dispensary (Lot numbers etc.)
2) Added an additional commit that fixed two very minor bugs.
Thanks for the contribution (also please update the wiki NIST testing page)
-brady