All Migration Script Consolidation#830
All Migration Script Consolidation#830andie23 wants to merge 55 commits intochore/find_patients_paginationfrom
Conversation
Migrate EMR data to MaHIS
…_to_mahis_data_migration Getting Mahis changes for harmonization
…cilities stuff in location and extra columns should use location attributes table this follows the openmrs data model design and does not introduce foreign tables maintaining the original structure
…/BHT-EMR-API into fix/merge_locations_x_facilities
modified: app/controllers/api/v1/locations_controller.rb modified: app/models/location.rb new file: app/models/location_attribute.rb modified: config/routes.rb new file: spec/factories/location_attributes.rb new file: spec/models/location_attribute_spec.rb new file: spec/requests/location_attribute_spec.rb
modified: app/services/facility_service.rb
…rvice Fix/location based facility service
Facility fix
…master_central_mahis_backend
…to master_central_mahis_backend
…into master_central_mahis_backend
…ies' into master_central_mahis_backend
andie23
changed the base branch from
development
to
chore/find_patients_pagination
There was a problem hiding this comment.
Pull request overview
This pull request consolidates migration scripts to align multiple EMR systems with a central MAHIS backend. The changes are intended for integration testing purposes and involve significant database schema modifications.
Key Changes:
- Merges location and facility data by creating location attributes and updating foreign key relationships
- Adds location_id tracking to transactional tables for multi-site support
- Updates authentication service to set location context during login
- Includes metadata remapping configuration and documentation
Reviewed changes
Copilot reviewed 53 out of 62 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
db/migrate/20251111144308_location_x_facilities.rb |
Main migration orchestrating facility-location consolidation with CSV loading and SQL execution |
db/migrate/load_csv_mapping.rb |
Ruby script to load facility-location mappings from CSV into temporary table |
db/migrate/merge_locations_x_facilities.sql |
SQL script creating location attributes and merging facility data into locations |
db/migrate/20251107064021_add_location_id_to_txn_tables.rb |
Adds location_id column to transactional tables for multi-site tracking |
db/csv/locations_x_facilities.csv |
Mapping data between 586 locations and facilities |
app/services/user_service.rb |
Updates login to use unscoped query and set location context |
app/models/user_role.rb |
Includes Locatable concern for location-aware behavior |
config/routes.rb |
Adds new route for location attribute API endpoint |
bin/metadata/txn_remap_config.yaml |
Configuration for metadata remapping during consolidation |
bin/dump_metadata.sh |
Updates metadata dump script to accept custom file paths |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ActiveRecord::Base.connection.disable_referential_integrity do | ||
| tables.each do |table| | ||
|
|
||
| # get table type | ||
| table_type = ActiveRecord::Base.connection.select_one <<~SQL | ||
| SHOW FULL TABLES LIKE #{ActiveRecord::Base.connection.quote(table['TABLE_NAME'])} | ||
| SQL | ||
|
|
||
| # skip if table is a view | ||
| next if table_type['Table_type'] == 'VIEW' | ||
|
|
||
| if table['TABLE_NAME'] == 'global_property' | ||
| # remove composite foreign key | ||
| ActiveRecord::Base.connection.execute <<~SQL | ||
| ALTER TABLE global_property DROP INDEX idx_global_property_property_location; | ||
| SQL | ||
| end | ||
|
|
||
| # remove foreign key | ||
| ActiveRecord::Base.connection.foreign_keys(table['TABLE_NAME']).each do |fk| | ||
| if fk.options[:column] == 'location_id' | ||
| puts "Removing foreign key #{fk.name} from table #{table['TABLE_NAME']}" | ||
| ActiveRecord::Base.connection.remove_foreign_key(table['TABLE_NAME'], fk.name) | ||
| end | ||
| end | ||
|
|
||
| ActiveRecord::Base.connection.execute <<~SQL | ||
| SET sql_mode = ''; | ||
| SQL | ||
|
|
||
| ActiveRecord::Base.connection.execute <<~SQL | ||
| UPDATE #{table['TABLE_NAME']} tl | ||
| INNER JOIN temp_facility_x_location_map flm ON tl.location_id = flm.facility_code | ||
| SET tl.location_id = flm.location_id | ||
| SQL | ||
|
|
||
|
|
||
| # change column type to integer | ||
| ActiveRecord::Base.connection.execute <<~SQL | ||
| ALTER TABLE #{table['TABLE_NAME']} | ||
| MODIFY COLUMN location_id INT | ||
| SQL | ||
|
|
||
| # update foreign key | ||
| ActiveRecord::Base.connection.execute <<~SQL | ||
| ALTER TABLE #{table['TABLE_NAME']} | ||
| ADD CONSTRAINT #{table['TABLE_NAME']}_location_id_fk | ||
| FOREIGN KEY (location_id) REFERENCES location (location_id) | ||
| SQL | ||
| end | ||
| end |
There was a problem hiding this comment.
The disable_referential_integrity block on lines 38-88 performs critical schema modifications without proper error handling or validation checks. If an error occurs during foreign key removal or column type changes, the database could be left in an inconsistent state. Consider adding validation checks and more granular error handling within this block.
| def load_csv_mapping | ||
| puts "Loading facility-location mapping from #{CSV_FILE_PATH}..." |
There was a problem hiding this comment.
Missing validation for CSV file existence. The script should check if CSV_FILE_PATH exists before attempting to read it. Without this check, the migration will fail with an unclear error if the CSV file is missing, which could be confusing during deployment.
db/migrate/load_csv_mapping.rb
Outdated
| values = batch.map { |m| "(#{ActiveRecord::Base.connection.quote(m[:facility_code])}, #{m[:location_id]}, #{ActiveRecord::Base.connection.quote(m[:location_name])}, #{ActiveRecord::Base.connection.quote(m[:location_district])}, #{ActiveRecord::Base.connection.quote(m[:facility_name])})" }.join(',') | ||
|
|
||
| ActiveRecord::Base.connection.execute <<~SQL | ||
| INSERT INTO temp_facility_x_location_map (facility_code, location_id, location_name, location_district, facility_name) | ||
| VALUES #{values} | ||
| ON DUPLICATE KEY UPDATE location_id = VALUES(location_id); | ||
|
|
||
| SQL |
There was a problem hiding this comment.
SQL injection vulnerability. Line 54 constructs SQL using string interpolation with ActiveRecord::Base.connection.quote(), but the column values are inserted directly into the VALUES clause. While quote() is used, the entire string concatenation approach is error-prone. Consider using parameterized queries or ActiveRecord's bulk insert methods instead for better security and maintainability.
| SET FOREIGN_KEY_CHECKS=0; | ||
| SET SQL_SAFE_UPDATES=0; |
There was a problem hiding this comment.
The migration disables SQL safe mode and foreign key checks globally (lines 1-2), which is risky. These settings affect the entire database session and could mask data integrity issues. Consider re-enabling these checks after critical operations are complete, or at minimum document why they must remain disabled for the entire script execution.
| def self.login(username, password) | ||
| user = User.where(username:).first | ||
| user = User.unscoped.where(username:).first | ||
| Location.current = Location.find(user.location_id) |
There was a problem hiding this comment.
Setting Location.current in the authentication service may not be thread-safe. In a multi-threaded environment (like Puma or concurrent request processing), this could lead to race conditions where one user's location affects another user's session. Consider using thread-local storage or request-specific context instead.
| ActiveRecord::Base.logger = Logger.new(STDOUT) | ||
| ActiveRecord::Base.logger.level = :debug | ||
|
|
||
| # execute 'source /home/brian/projects/BHT-EMR-API/db/migrate/load_csv_mapping.rb' |
There was a problem hiding this comment.
Commented-out code should be removed. The execute 'source ...' line on line 10 is commented out and should be deleted to improve code cleanliness, especially since the functionality is replaced by the load and load_csv_mapping calls on lines 11-12.
| class LocationXFacilities < ActiveRecord::Migration[7.0] | ||
| # set stdout logger for active record | ||
|
|
||
| def change |
There was a problem hiding this comment.
The migration is missing rollback logic. The change method should either be split into up and down methods or use reversible blocks to ensure the migration can be rolled back if needed. This is especially important for a migration that modifies table schemas and foreign keys across multiple tables.
…oved clarity and maintainability
Prevents Mysql2::Error when attempting to update foreign key columns in transactional tables that reference metadata tables with no diffs. The script now validates that temp_remap_* tables exist before running UPDATE queries, skipping columns where no mapping data is available. This fixes the error: "Table 'openmrs_dev.temp_remap_drug' doesn't exist"
Prevents Mysql2::Error when attempting to update foreign key columns in transactional tables that reference metadata tables with no diffs. The script now validates that temp_remap_* tables exist before running UPDATE queries, skipping columns where no mapping data is available. This fixes the error: "Table 'openmrs_dev.temp_remap_drug' doesn't exist"
6 participants
Context
This pull request consolidates all migrations scripts to align all EMR systems to one central MAHIS backend. This should only be used for integration testing