Cchit: cpoe

anonymous wrote on Saturday, January 09, 2010:

We have completed coding for Messages. The codes have been posted in the tracker for review. Note that the upgrade script is for testing use only and thus the funny name. You can also visit www.medbloom.com to see how it works. Use “test1” for both username and password.

Messages task has been moved from CDR to CPOE because it’s more for prescription orders authorization, lab orders authorization, and lab results signoff. When we started the work, we were thinking about patient reminders and lab results signoff. But we decided that it’s not a good idea to flood the Messages box with patient reminders. Note that the authorization and signoff pieces are not ready for use yet because the corresponding builds have not been done.
 
In terms of messaging, we assume that all messages are added to each patient’s Notes in Patient section. When you click on “Add New”, it’s treated as a new note. But if you open up any existing message, then any new message you add will be appended to the original note.

As for the (See All) and (Just Mine) options, we may want to add a security setting for each user. Not every user should be allowed to use (See All). What is the best way of doing this?
    
We will not remove Pt Notes/Auth in Miscellaneous section until Messages is ready for use. The Notes section in Patient section can be kept the way it is without making any change.

bradymiller wrote on Saturday, January 09, 2010:

hey,

Here’s code review for messages. Again, mostly from reading through patch and some testing on your demo:

interface/main/authorizations/authorizations.php
looks ok

interface/main/left_nav.php
**Internaionalize word ‘Letter’

interface/main/messages/messages.php
**Use openemr/library/formdata.inc.php functions (multiple reasons for centralizing this stuff; one is looking towards php6 compatibility) (specificically, I am only referring to line using the get_magic_quotes_gpc() function; note in formdata.inc.php, there are standard functions for only removing slashes (if magic quotes), and adding slashes)
**Change xl(’(Status)’) to (".xl(‘Status’).")

library/options.inc.php
looks ok

library/pnotes.inc
looks ok

For database scripts:
Add stuff to database.sql
***Are we being a bit greedy on the char limit for message status. Remember another user may want to set something much larger, since it’s customizable.
In the upgrade sql script add to sql/3_2_0-to-3_3_0_upgrade.sql
***For adding new list entries see lines 77-130 of openemr/sql/3_1_0-to-3_2_0_upgrade.sql for examples
***For adding new list see lines 6-69 of openemr/sql/3_1_0-to-3_2_0_upgrade.sql for examples (also, for the sequence number in list I’d suggest stating at 5 and going in increments of 5; this makes it easier to add additional ones in the future and order the way you want; avoiding the exact problem you had with ordering of the ‘Patient Note Types’ list)
*****Also for this list try to set a ordering id that puts it in the correct alphabetical place when listed in admin->list selector (simply look through database.sql to find an appropriate number)

-brady

bradymiller wrote on Saturday, January 09, 2010:

hey,
Also regarding the see mine, see all options. Agree see all shouldn’t even be an option; maybe the admin user (hence, protect with a ‘super’ acl). What does the rest of community think?
-brady

bradymiller wrote on Saturday, January 09, 2010:

Aron,
Check out this demo, since will likely decrease work you’ll need to do to refactor the soon to be new message/pt notes screen.
-brady

anonymous wrote on Saturday, January 09, 2010:

Please see the requirements posted here:
https://sourceforge.net/projects/openemr/forums/forum/202506/topic/3512713

bradymiller wrote on Sunday, January 10, 2010:

hey,

Thomas meant to post this:
http://www.openmedsoftware.org/wiki/Computer_Physician_Order_Entry

-brady

sunsetsystems wrote on Sunday, January 10, 2010:

The Messages GUI looks nice.  A couple of observations:

Before I click (See All), no messages are listed yet at the bottom right I see “<<   8 of 8   >>”.

In (See All) mode I see 8 messages in the Messages page, but only 2 in “Pt Notes/Auth”.  How come?

What is the difference between “Send message” and “Save message”?  I appended to a message and clicked “Save message” but couldn’t tell that it did anything.

Re the “Re:” dropdown - are you really going to list all patients there?  That doesn’t seem workable.  See the Event Editor popup which invokes a widget for selecting a patient.

The message edit panel needs a Cancel button.

Code comments to follow.

Rod
www.sunsetsystems.com

sunsetsystems wrote on Sunday, January 10, 2010:

Re messages.php:

Look at all of your “<a href” tags.  Most of these appear to need “onclick=‘top.restoreSession()’”.

Use formData(), don’t read GET and POST variables directly.

Don’t write to system-provided variables such as the $_GET array.  I know you probably copied that code from somewhere, but it just ain’t right.

Rod
www.sunsetsystems.com

anonymous wrote on Sunday, January 10, 2010:

The difference between the two sections are due to code and table changes. The programmer has not worked on Pt Notes/Auth, and we have expected that section to be removed (as discussed earlier).

The Save Message is designed for changing the status (e.g. from New to Read). We may want to allow a note to be appended as well. In this case, it is not sent to anyone.

anonymous wrote on Monday, January 18, 2010:

Please review the changed files. We have also added the following enhancements:

1. The link is now on Patient (instead of Type).

2. When you open an existing message, you are not allowed to change the Patient’s name.

3. The Status can now be changed without the use of  button.

bradymiller wrote on Tuesday, January 19, 2010:

hey,

Here’s a code review (I have not tested it yet; that will follow):

Why is other project(s) code in here? Makes it hard to review/test and will be hard to commit in modules also.

FOR FILE interface/main/messages/messages.php

1) Below code does not make sense and would break dealing with apostrophes if magic quotes is turned off. I am assuming your goal is just to remove the ‘escapes’ before sending it through the function, and you are avoiding adding the escapes for database insertion, so just use function strip_escape_custom() from formdata.inc.php to only strip escapes from input(will only do this if magic quotes is on). Please read the function carefully in library/formdata.inc.php(they are short and well explained) and ensure have understanding how escaping and magic quotes works. Test this stuff by placing apostrophes in the fields and ensure ir works when magic quotes is turned on and off (remember you need to restart apache when change this setting). If you are still confused, then please ask for a further explanation.
+        $note = formData(“note”);
+        $note = strip_escape_custom($note);

2) Don’t use translated terms in the algorithm (note this is dealt with automatically by the lists stuff). (note your below code would search for the translated term of ‘New’ in your list id which will not exist, since only english constants and not the translations are stored in the database; this is what gives openemr intra-clinic multi-lingual functionality; ie. a dutch doctor can select the sex of patient in their language and a spanish doctor would see the sex in their language while in actuality it’s stored in the database in english via the id).
$message_status = xl(‘New’); should just be $message_status = ‘New’;
Test this code by seeing how it works in another language. You’ll note that ti won’t work if New has a translation.

FOR FILE 3_2_0-to-3_3_0_upgrade.sql

1) Your code will not work and could potentially break current users databases whom are trying to upgrade (your using IfNotRow and not IfNotRow2D) at line 478 of patch. You need to test your upgrade scripts by both running it on a previous version database and on a current version database to ensure it installs correctly only when warranted. To code responsibly in OpenEMR, you are responsible for testing your upgrade scripts.

2) To properly add new lists see this very nice recent example here (use IfNotRow2D, which is safer than IfNotRow to do this): http://openemr.cvs.sourceforge.net/viewvc/openemr/openemr/sql/3_2_0-to-3_3_0_upgrade.sql?revision=1.5&view=markup )  and to add new items to list that already exist refer to my directions i gave you in my code review of your first patch above in this thread.

3) To further expand on my previous instruction in above thread:
-No need to use the max functions in the ‘seq’ column discovery
-For lists that already exist (adding new items):
-Manually set that where’d you like the new item to be ordered in the list; it’s ok to have the same ‘seq’ number as other entries (then ordering resorts to alphabetical).
-For lists that don’t exist:
-Instead of using max() function on title entry of list, give it a ‘seq’ number that will have it sort alphabetically (again, ok to have same ‘seq’ numbers as other list titles, and it will sort alphabetically if they are the same).
-If you don’t understand this stuff, then ask me more input here (I’ll direct you to examples in openemr as to why the ‘seq’ stuff is important).
-Also, start the ‘seq’ at 5 and increment by 5; this makes it much easier to add items in future version of OpenEMR.
-Also, ensure you organize your list and sequencing numbers correctly (either alphabetically or in the order you want them to show up in the ‘selector fields’). The ‘seq’ field is for ordering. Do not get in the habit of bypassing it via your options.inc.php alphabetization code (this was ok with the ‘Patient Note Types’ list since the ‘seq’ stuff is already screwed up in this list)

4) This is unrelated to the messages code, but do not make a new gender list. There is already one; look through the lists. Use that for your code. And when generate the selector, simply make the null entry equal to ‘All’ (don’t use both, since users may add more sex categories) in the ‘generate’ function. This is important, because then a user can add a new sex such as ‘transgender male’ onto the list, and not only will it show up in the demographics, but it will automatically work in your filtering algorithms for patient messaging and clinic alerts.

-brady

anonymous wrote on Tuesday, January 19, 2010:

I’m aware of the 3 common/merged files: left_nev.php, database.sql, and the upgrade script. We have multiple programmers working on multiple MUs. They share 1 test site and 1 demo site. Each MU can be worked on by more than 1 programmer. That’s why you see codes from other MUs. I can take them out before sending you the next patch.

bradymiller wrote on Friday, January 22, 2010:

hey,

Here’s review of your most recent message code:
http://sourceforge.net/tracker/?func=detail&aid=2928938&group_id=60081&atid=1245239

Some overall issues:
1) Re seems a bit short (maybe Regarding) in the message editor window
2) I’m Confused regarding ‘Done’ (when ‘Done’ the message doesn’t show up in Messages screen, but the ‘Done’ one are only ones that show up in authorizations screen; confusing?? ; I think I may be missing the point of ‘Done’)
3) Why clear textbox when change status(could be annoying if a user type messages and it disappears)?
4) The output of your notes breaks when a newline is in it (it prints out the \n character; these were dealt with fine in previous code)
5)Just to clarify, is openemr/interface/patient_file/summary/pnotes.php gonne be removed/replaced at some point? (right now it’s confusing that user goes to this screen when click link on openemr/interface/main/authorizations/authorizations.php)

Some lists issues:
1)If list already exist, then set for where you a want ordered in list, and ok to have duplicate seq numbers (so ‘Lab Results’->‘4’ ‘New Orders’->‘4’, and ‘Patient Reminder’->‘4’ should work out ok)
2)If new lists, again seq by 5,10,15,… (ie. for the new ‘Message Status’)
3)For the new list main entry, make the seq according to alphabetization of other main lists entries (ie. ‘Message Status’ should have same seq as ‘Occurrence’ list; then will sort correctly in the admin->Lists ‘Edit List’ selector; ie so message_status seq should be ‘10’)

Specific file reviews:
openemr/interface/main/left_nav.php
-clueless to what changes are, too much noise in diff.

openemr/interface/main/authorizations/authorizations.php
-be much more clear when you comment out code
-Do you feel strongly about adding periods in the constants? By doing this you are creating two new constants that will require translation; plus we can’t get rid of the old constants to ensure support of previous versions.

openemr/library/options.inc.php
ok, but lots of diff noise since not including Aron’s gui refactor stuff

openemr/library/pnotes.inc
seems ok

openemr/sql/3_2_0-to-3_3_0_upgrade.sql
lots of diff noise since not using up to date code
you didn’t test it correctly, or you would of noted missing IfNotRow2D for message_status list
see above discussion regarding lists

openemr/sql/database.sql
lots of diff noise since not using up to date code
see above discussion regarding lists
why are you messing with pc_apptstatus (or perhaps this are changes from somebody else)?

openemr/interface/main/messages/messages.php
I can’t see where you include library/formdata.inc.php
See my stuff discussed in previous review above in this thread, since you haven’t fixed either issues yet…
I’m guessing your newline bug issues is somewhere in here (discussed above)

As discussed above, there is a lot of noise in your files when i diffed to current SF teip (your code is getting behind). Also, your patch was unusable and unreadable; not sure what you did differently to create it)

-brady

bradymiller wrote on Friday, January 22, 2010:

hey,
Disregard my last comment regarding the patch being unreadable; now i realize it’s basically a patch from your last version that you posted which was helpful to see what you changed from last time.
-brady

anonymous wrote on Friday, January 22, 2010:

1. Not sure why you would want to diff to SF TIP as our codes have not gone through SVN yet. I’m rethinking the review process. Maybe it’s better to go through SVN first.

2. The diff noises are due to the previous codes merge. We took out the merged codes that are for other MUs as you have requested. As a result, you see the noises in the patch file.

3. The “Done” status is as designed. This helps a user cleans up the messages box without deleting the messages. We will check on the clearing of text when the status is changed.

4. The Pt Note/Authorization section will be removed as discussed earlier.

5. The list seems to be the major issue here. Will talk to you more.

6. formdata.inc.php - oops, I didn’t pass this one to the programmer. My mistake.

anonymous wrote on Friday, January 22, 2010:

7. messsage_status - I just checked with the programmer. This is a new list and so IfNotRow should be used (as per your instruction). Why do you say IfNotRow2D now?

anonymous wrote on Friday, January 22, 2010:

8. We added “.” to the end of the two sentences when editing the file: ‘You do not have access to view/edit this note’ and xl(‘Some authorizations were not displayed. Click here to view all’). This was done without knowing the impact. So adding the periods would cause problems.  

bradymiller wrote on Friday, January 22, 2010:

Thomas,
(simply answering by number of Thomas’s entries)
1) This is a Tony issue. It’s time for him to put SF stuff into the SVN (your files erase all the gui refactoring stuff).
2) The patch is actually fine; it’s totally unusable but is very useful to see what changes you  made from the last patch on this projects.
3) makes sense
4) makes sense
5) regarding lists, have your programmer look at my suggestions (i gave rec. seq entries to keep moving forward)
6) good
7) In previous review (see entry 2) I gave a better example of creating a list, and to use the IfNotRow2D function. He partially the followed instruction (created the parameters for the IfNotRow2D function correctly, however since he is still using IfNotRow with the new parameters this list will never be created on an upgrade and this is why I know your programmer did not test it). Have him read quickly through these functions in the sql_upgrade.php script and he’ll understand what i am talking about.
8) The way the translation engine works is 1) a script picks up all english constants (stuff in xl functions) within the codebase 2)these constants are place in a google docs translation spreadsheet 3)every couple weeks step 1 and 2 are repeated to place new english constants into spreadsheet (any changes inclusing punctuation is a new constant) 4)we never remove constants to ensure out tables are compatible with previous openemr versions. Right now we are at 3500 english constants. I’m  not saying don’t fix typos and punctuation. I’m just saying use judgment; if you feel it’s important, then do it. If you feels it’s not that big of a deal (ie. message rarely shown) then don’t do it. I don’t want to police this stuff, just want you guys to consider this when fixing/prettying up strings.

-brady

sunsetsystems wrote on Friday, January 22, 2010:

Well I’m trying to respond but can’t figure out the SF formatting requirements for code snippets.

Guess I’ll send email.

Rod
www.sunsetsystems.com

sunsetsystems wrote on Friday, January 22, 2010:

By the way, SF support agrees with me that there are some bugs with posting code snippets in the forums.  I have created ticket #8117 concerning this, which they say will be promptly escalated.

Rod
www.sunsetsystems.com