-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding db column for satellite_lab_name and UI changes for using Satellite Name instead of Id #143
base: master
Are you sure you want to change the base?
Conversation
dishahpatel
commented
Apr 6, 2025
- Adding new columns for satellite lab name to the user and patient tables
- Updating the user creation and user update forms to include Satellite Lab Name
- Updating the patient creation form to display satellite lab name instead of satellite lab id
- Updating the patient update form to display satellite lab name instead of satellite lab id
- Updating the field name to Satellite Lab Name when viewing a patient profile
@@ -0,0 +1 @@ | |||
ALTER TABLE `patient` ADD COLUMN `satellite_lab_name` varchar(45) DEFAULT NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding the name field to each table, I think it would be better to add a new table that stores the satellite lab ID and name, in the lab database. So something like
CREATE TABLE IF NOT EXISTS `satellite_labs` (
`id` int(11) unsigned NOT NULL AUTO_INCREMENT,
`name` VARCHAR(40) NOT NULL
);
Then you would need a function like
function get_satellite_lab_name($lab_db_name, $satellite_lab_id) {
// change to database $lab_db_name,
// query satellite_labs table
// change back to database previously used
// return value
}
As well as an associated function to write or update a satellite lab row.
What do you think? There is definitely a drawback here in that an extra query would be involved, as well as a switch of the database connection. But I think that avoiding duplicating the data per-user and per-patient would be worth it, with the additional advantage that we can store metadata on satellite labs this way too.
|
||
# Retrieves all user IDs corresponding to $LIS_SATELLITE_LAB_USER from the user table | ||
$saved_db = DbUtil::switchToGlobal(); | ||
$query_string = "SELECT satellite_lab_id FROM user WHERE satellite_lab_name = '$satellite_lab_name'"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$query_string = "SELECT satellite_lab_id FROM user WHERE satellite_lab_name = '$satellite_lab_name'"; | |
$query_string = "SELECT satellite_lab_id FROM user WHERE satellite_lab_name = '$satellite_lab_name' LIMIT 1"; |
query_associative_one
I don't believe enforces the query only returns one thing.
|
||
# Retrieves all user IDs corresponding to $LIS_SATELLITE_LAB_USER from the user table | ||
$saved_db = DbUtil::switchToGlobal(); | ||
$query_string = "SELECT satellite_lab_id FROM user WHERE satellite_lab_name = '$satellite_lab_name'"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this kind of highlights a possible issue - get_satellite_lab_id_by_name
implicitly uses the User
table in blis_revamp
as the source of truth.
So, imagine we have a backup of a lab. When we restore a backup, we restore the blis_[lab id]
database, not blis_revamp
, because we don't want to overwrite the existing usernames and passwords for other labs, and other shared data, because eg. a country director might be importing backups for many labs. But this also means we'll be importing a patient
table filled with a lot of satellite_lab_id
s that don't (necessarily) line up to the IDs and names in the user
table.
I don't think we have any labs where this would be an issue at the moment. But it does make the backup/restore issue more complicated, and it would be a point towards making the "source of truth" of lab IDs and names in the blis_[lab id]
database (eg. maybe use the patient table for these functions if you want to keep satellite_lab_name
as a column on both patient and user?)
$selected_satellite_lab_name = $patient->satelliteLabName; | ||
foreach ($satellite_lab_names as $satellite_lab_name) { | ||
if ($satellite_lab_name == $selected_satellite_lab_name) { | ||
echo "<option value='{$satellite_lab_name}' selected>{$satellite_lab_name}</option>"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation here would allow for a user to submit any value in the form, not just the ones that are in the dropdown (for instance by modifying the HTML to use a different string for the name.)
It would be better to submit the numerical ID as the value and the server can determine what the lab name is from that. Yes you can submit a different ID than what the dropdown intended, but that is also the purpose of the dropdown!
@@ -39,7 +39,7 @@ function update_lab_user() | |||
|
|||
|
|||
// Begin email address test | |||
var email_regex = new RegExp(/^(([^<>()[\]\\.,;:\s@\"]+(\.[^<>()[\]\\.,;:\s@\"]+)*)|(\".+\"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/i); | |||
var email_regex = new RegExp(/^(([^<>()[\]\\.,;:\s@\"]+(\.[^<>()[\]\\.,;:\s@\"]+)*)|(\".+\"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What did this change? (well I see what it changed, but why?)