jklann
commented
8 years ago Thanks for your work Keith. I reviewed a lot of your changes and I think many of them are particular to your i2b2 setup and so shouldn't be in the master branch. There is some good stuff in there though - I hope you have time to clean it up!
I do want to understand the security issue because that seems very important. As I understand it, the problem is when the CDM schema/user wants to get to the i2b2 metadata schema/user's tables - there has to be a new grant whenever a metadata table is altered or dropped/created - is that right? So this isn't an issue within the CDM schema, right? (You didn't make it seem that way, just making sure.)
Also, would it be possible/proper to put the grant commands in the the transform, as a stored procedure? Or, because it needs to be executed by the metadata user (as opposed to the CDM user) will that not work?
Thanks. Also I'll be at AMIA through Friday so it might take me a few days to get back to you.
Jeff,
Please review these changes from both the Oracle and MSSQL folders, and merge with master if you feel its relevant.
I've seen sites that use separate databases for all of the i2b2 cells, and some that use the same database. So I did not apply the MSSQL permission changes like I did in Oracle. I kept the logic the way you had it, but applied the [dbo] across the board to keep consistency with the schema prefix ('dbo').
Regarding the two new columns in PCORNET_MED (table) in CDM that is referenced in the PCORNET_MED synonym in the CDM database, I made some minor changes. I realize that the i2p-transform repo contains a file to apply the PCORNET_MED columns in i2p-transform / Oracle / PCORI_MEDS_SCHEMA_CHANGE_ora.sql. It's probably okay to have this file, but from a person reading code, simpler is usually better. I think the actual script files should have documentation in them (both the ontology repo and transformation repo), so someone reading the code can clearly see that they point to one another. Mixing an ALTER TABLE command with a transformation will require extra work from an Oracle perspective, because when you update a table's schema, the security needs to be re-applied for a procedure in CDM to reference a synonym that references a table in a different schema. That's why I applied the two columns. In the i2p-transform, I'll put conditional logic to avoid an exception. Also, the developer may update NDC and Rx CUI codes multiple times, so that becomes a lot of work to re-apply security. I'll be suggesting to isolate the ALTER commands in my next pull request. There is also new file called i2p-transform_permissions.sql. This contains all of the security changes that are referenced in your notes in the *.md file (README) in the i2p-transform in the 'Preparing the PopMedNet database'. If a code change is required, there should probably be a file to make those code changes rather than just having it in the documentation. That's why I'm suggesting to add this file.
I also noticed that there are discrepencies in the length of the NDC column. In the dispensing table, it uses 11. In your transform update, 11 is hard coded, but it sets 12 as the max length. According to the VA's software, called Vista, you can have 16 (and sometimes more) characters. But other medical documentation says there are 11 max. I guess it's something to research. Probably not a big deal, but something to look at.
Also, I'll do another pull request on the Oracle scripts in i2p-transform so this pull request is done at the same time as the other one, so someone cloning both repositories gets both of them. I did not review the MSSQL scripts in i2p-transform for CDM v3, but hopefully you can make them consistent.
Let me know if you'd like to discuss.
Keith