Meaningful Use Measures:
At least 80% of all unique patients seen by the EP have demographics recorded as structured data.
Certification Criteria for EHR:
Enable a user to electronically record, modify, and retrieve patient demographic data including preferred language, insurance type, gender, race, ethnicity, and date of birth.
Tony,
This is a rather toxic dump. Did you look through (especially the upgrade script) and test this code.
Cursory review:
**database.sql
-to keep our sanity in the future please place the insert ‘layout options’ lines in the order they appear in the layout
-for list have seq go in 10 increments (just add 0 to end of each) to allow easy upgrade/mods of list in future
-why are previous entries not sorted alphabetically in list?
-for the option_ids, do not includes blanks (this will kill upgrade-mod options in future)
-for future, prefer option_id to be uncapped and abbreviated
**3_2_0-to-4_0_0_upgrade.sql
-what the heck are you doing with the If function calls. please read through rest of this script and the sql_upgrade.php to learn the functions. (you even have comments on these lines… geez…)
-for list have seq go in 10 increments (just add 0 to end of each) to allow easy upgrade/mods of list in future
-why are previous entries not sorted alphabetically in list?
-for the option_ids, do not includes blanks (this will kill upgrade-mod options in future)
-for future, prefer option_id to be uncapped and abbreviated
Brady,
None of those rules are documented in any place I can find. How would a programmer know those kinds of rules? I read the code and it not well documented and it a giant pile of if/then/else… Has anyone every of of a case statement? Nice and easy to follow. In any case I did not see any issue with two sql files submitted by Jeremy. Certainly some of your suggestions make sense, I would really appreciate it if guidelines like these were part of the developers pages on the wiki.
The upgrade worked without an issue on the Developer’s Appliance. I’ll ask Jeremy to rework it to meet your notes.
If you have specific RULES that you want followed for specific kinds of code post documentation regarding them, please. I don’t have time to guess what you might approve or disapprove and you don’t time to deal with code that doesn’t meet the bar.
We all want the best here, try to be gentle, even with me, though I can take it.
I don’t mean to be rude (sorry for that); I was just taken aback by the sql upgrade script, which obviously wasn’t tested fully, didn’t follow the template of other related code, and that something of that nature got into the cvs:
For example, here’s what happens if you run the upgrade script a second time (the point of the #If statements in the script are for it to be “safe” and not try to install stuff that is already there; these functions can be found in the sql_upgrade.php script)
Processing 3_2_0-to-4_0_0_upgrade.sql …
INSERT INTO list_options ( list_id, option_id, title, seq, is_default ) VALUES (‘language’, ‘Armenian’, ‘Armenian’, 3, 0)
ERROR: query failed: INSERT INTO list_options ( list_id, option_id, title, seq, is_default ) VALUES (‘language’, ‘Armenian’, ‘Armenian’, 3, 0)
Please just yank all these changes, and re-commit when they are fixed (via my above suggestions).
I agree more documentation would be useful on the policy page (http://www.openmedsoftware.org/wiki/Development_Policies), and we have been documenting as issues arise; I don’t have enough hours in the day to write a full openemr howto programming guide, but am happy to review code… (I’d be very happy if you’d document these issues on the policy wiki page so other developers can learn from this). A quick code review by us would of avoided these issues; if any questions regarding code we’re always happy to provide reviews(generally with new developers, the earlier the better to avoid wasted time). This has worked very well in the past for developers to learn the finer intricacies of openemr.
Really, I’m not offended, just annoyed the code like the sql_upgrade.php is not documented fully when it’s written or enhanced significantly and it doesn’t follow normal, recommended programming standards (like no 1 line ‘if’ statements).
Embedding a programming meta language as comments in an SQL file seems either crazy or genius, can’t tell which.
In any case how to construct the meta language and use it should have been carefully commented in the code, at least. You have to look in three places to piece together what each call is trying to accomplish. each reference to $match could have a comment describing what the match is intended to contain, like “table name”. or better, used a distinct variable name for each pattern matched. Much less chance of missing the point that way.
Enough soap boxing …
I’ll try and post something, but clearly I don’t know it well enough to get it right yet, so I am reluctant to document a standard I don’t know.
It’s genius… Rod wrote this, and once you figure it out, you’ll see the genius of it (analogous to the layouts engine). I was honored to add the #IfNotRow2D function though…
Isn’t part of the whole development thing to learn the machine that your working in. I learned about this sql/upgrade engine simply because it was required for me to actually create working and appropriate code. That’s generally the way development works, right?
Documentation of sql_upgrade.php would be a nice contribution, and I would suggest that doing it is easier and more constructive than complaining about it; the whole script is only 228 lines.
We do have documentation by example in the various upgrade scripts. Other developers didn’t seem to have any trouble with it. If you want it done to your own standards, allocate a budget for it. So far it’s been free.
Just a comment - I’m not looking to promote any more snarkiness in this thread than already exists, but the so-called examples look like regular MySQL comments in a .sql file to me. I thought the #If sections were odd, to be sure, but saw no reason to assume there was this embedded language in the comments.
I did discover that if any line has “-” in it, that entire line is ignored and the subsequent sql statements must get added to the query string as a result because in my testing, I saw my changes go through on a 3.2->4.0 upgrade. Take this for what it’s worth. I’m not about to suggest anybody fix it or plan for meta comments to go along with the meta language because I’m certain “good enough” is working for this.
I documented the existing regex matches’ behavior and placed a copy in sql_upgrade.php as well as the 3_2_0-to-4_0_0_upgrade.sql file for future reference. It makes more sense to me to have a note in the actual sql file rather than just sql_upgrade.php. I’ll stick a copy on the wiki as well.
Finally, I have a question about acceptable style in the upgrade scripts. I have what amounts to a table being altered, and several entries in the list_options and layout_options tables. There are 26 languages being added to the language list, the existing languages’ seq #s modified, 36 ethnicities being added to the ethrace list with the original 4 being updated as well. I’m also adding two new lists with list items and updating the layout of the demographics page. Since these changes are all related, is it acceptable to test the existence of single row from my inserts or do I need to break these down into smaller sections? If it should be broken down, to what extent would the community like to see this done?
Thanks Jeremy. I guess you’ll put your updates in the tracker?
I’d say it’s fine to put related changes in the same conditional group if you are sure that nobody would have previously applied any of them separately.
It’s all part of the learning curve. I’m a bit more ocd about it, and my suggestion is to put them in separate pertinent If’s (then following the standards of examples already in the upgrade file(s)). The modifications of the already existing layout items actually don’t need to be in an If statement (see lines 253-260 for good examples) . The modification of already existing list items(the ‘seq’ numbers) also doesn’t need to be surrounding in an If statement (see lines 132-133 in 3_0_1-to-3_1_0_upgrade.sql for example). The real goal of this script is to not break if run it a second time (so a good test is to run the upgrade script to ensure it works, and then run it again to ensure no errors).
