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