slim180 wrote on Wednesday, January 06, 2010:
Is there any reason why the 1st else block (near the top) is deleting the entire list and then re-adding it again? The list_options table has a dual primary key, so this can easily be fixed simply by using the UPDATE statement.
So, if there is something I’m not aware of, please speak up. If I fix this, would someone consider testing patch?
sunsetsystems wrote on Wednesday, January 06, 2010:
Well that is the inherent logic of the form. When you submit, the list is supposed to be made to match what is in the form.
You could do this by matching up each item, figuring out what was changed, doing the updates, inserting what was not already there, and deleting anything that remains - but what’s the payoff for all that trouble?
Of course it’s a different story with areas like patient demographics where concurrency issues come into play. But lists are not updated routinely.
slim180 wrote on Wednesday, January 06, 2010:
The code is just as simple if all the columns are updated even if the value is the same. Later, if you ever needed a trigger, it would know the difference (NEW <> OLD).
The update would keep the data integrity. It would help me out too. I have added a table to my own database and would like to use the list options.
sunsetsystems wrote on Wednesday, January 06, 2010:
I don’t mind if you change it. But don’t forget about deletes - the user can erase list items they don’t want.
There is another benefit, audit logging will make more sense.
tmccormi wrote on Wednesday, January 06, 2010:
Just so happens I have a customer request that will, likely, require triggers on everything …. 
-Tony www.mi-squared.com
slim180 wrote on Friday, January 08, 2010:
The insert, update, and delete statements will need to refer to a row id. That needs to replace the primary key and the primary key moved to a unique constraint:
ALTER TABLE list_options DROP PRIMARY KEY;
# Give this table a unique id
# MySQL will auto number existing records automatically (mysql Ver 14.14)
ALTER TABLE `list_options` ADD `id` BIGINT( 20 ) NULL AUTO_INCREMENT FIRST ,
ADD PRIMARY KEY ( `id` );
This adds an ID at the top. I checked the EMR v3.1.0 and all select SQL on this table refer to the column names (just as they should). So, changing the ‘select *’ order is fine.
sunsetsystems wrote on Friday, January 08, 2010:
I don’t get it. Why do you want an extra unique key?
slim180 wrote on Friday, January 08, 2010:
The Delete needs it. Right now, you delete by removing the list id. Well, if it is removed than the code does not know what to delete. You have to give everything an id to track it. This is also a standard way to join tables. This way, the list id is not copied everywhere, instead the id is referenced. So, they list_id and option live in a central location…
sunsetsystems wrote on Friday, January 08, 2010:
The pair (list_id, option_id) IS the unique id. That is its exact intended purpose. Use it exactly as you would a numeric ID. Just put the original values of these into a hidden form field, just as you would a numeric ID, that way you know what was deleted.
slim180 wrote on Saturday, January 09, 2010:
Ok, thanks for barring with me Rod. I’ll do my best to explain.
I want to add another filter on the reports. Your table is a good place to maintain the list items in the filter. So, in the end I’m needing a user modifiable list. Now I would need to refer to the items from a foreign key. This would not allow the user to delete the list item if it were referenced elsewhere. Naturally they can still.be modify.
The option goes into a report as a both a filter and a data item. A straight join (not a left join) need to be used for efficiency.
Now, when I starting putting this into place, we were surprised to see that we could not add or modify list items anymore because the code was running a delete violating the foreign key. So, this is why I posted a new plan to change this.
Maybe there is a better place in OpenEMR for this…
sunsetsystems wrote on Saturday, January 09, 2010:
Aha, that makes sense. As noted above, you can do what you want without changing the table structure. Let me know if you need further explanation.
slim180 wrote on Saturday, January 09, 2010:
Great - We need the best plan for the code and for the database though.
I thought the Id would be a nice addition because: the foreign key would not need cascading updates in the event that the user changed the Id to another value. When the id is referenced the foreign key does not need a cascading updates, it is automatic.
Also, the Id would cut the code and work done by the code by about half. The code would already know what to do: Insert (value with no id), Update any post with the id (changed or not), and Delete (user checked “delete” and post variable would indicate this for one or more ids).
A BigInt is a smaller piece of information when referenced from other tables.
Without the Id the code becomes messy. Two queries are needed to figure out if an insert, update, or delete should be done. It is not possible to know the difference between a rename of list_id and a removal of one list_id and addition of another.
sunsetsystems wrote on Saturday, January 09, 2010:
There are situations where users will want to replace a list entirely. See for example the export and import code in the Backup script. This kind of issue already exists with the “codes” table, where CPT and ICD and HCPCS codes are updated annually or more often. Putting foreign key constraints in at the database level is going to complicate these situations. Not saying this is necessarily a bad idea, but with all the other big changes going in right now I think this is a really bad time for it and you are underestimating the total amount of work required.
I don’t agree that the code becomes messy or impossible without a new primary key. Why don’t you write some code that saves the list using your numeric ID, and I’ll show you how to change it so that the new ID is not required.
bradymiller wrote on Sunday, January 10, 2010:
Rod,
That reminds me. Weren’t you gonna make the backup export/import stuff optional and default to off to avoid confusing the general users.
-brady
sunsetsystems wrote on Monday, January 11, 2010:
Brady, yes, I just checked in that change. Thanks for the reminder.