Skip to content
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

[candidate] Create Sex table and remove Sex enum #9025

Merged
merged 24 commits into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ changes in the following format: PR #1234***
#### Features
- Add OpenID Connect authorization support to LORIS (PR #8255)

#### Updates and Improvements
- Create new `sex` table to hold candidate sex options, and change Sex and ProbandSex columns of `candidate` table to a varchar(255) datatype that is restricted by the `sex` table (PR #9025)

#### Bug Fixes
- Fix examiner site display (PR #8967)
- bvl_feedback updates in real-time (PR #8966)
Expand Down
19 changes: 16 additions & 3 deletions SQL/0000-00-00-schema.sql
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,15 @@ CREATE TABLE `language` (
INSERT INTO language (language_code, language_label) VALUES
('en-CA', 'English');

CREATE TABLE `sex` (
`SexID` int(10) unsigned NOT NULL AUTO_INCREMENT,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What purpose does this column have? It is not being used as a foreign key by any other tables and the other column in this table already has a not null/unique constraint and is being used as a foreign key into this table.

Removing it would solve the "what should it be named" debate in the thread below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, this column is not useful. I don't mind removing it

`Sex` varchar(255) NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hhhhm, my previous comment was just a little thought I had during our meeting, but if I am to really review this pull request I guess I'll try to be more exhaustive.

After giving it more thought, I am still not fully satisfied with the field names. I think having Sex in these names when we are already in the sex table is a little redundant. For instance, I'd really prefer to write sex.ID and sex.Name rather than sex.SexID and sex.Sex in my SQL requests, especially since the first one may look like a foreign key and the second is quite cryptic (imo). So I strongly advocate for the names ID and Name for the field names.

Regarding capitalization, the database sure is messy but I think it would be a good idea to maintain a consistent convention in at least the new tables. I am under the impression that this convention is unofficially singular snake_case for the table names and PascalCase for the field names. I am fine with that and this PR matches this convention, but I am still new and this impression of mine may be wrong, so can someone with more database experience than me confirm or deny it ?

Finally, regarding the pointing of sex foreign keys on ID or Name, I do not have a strong opinion as both have their pros and cons. I think using ID as a foreign key would probably be conceptually cleaner, but also require to update the current database and queries as well as require more joins in the latter. Using Name is certainly easier and will lead to simpler queries.

It would probably have been easier if I had mentionned all that earlier so sorry for the late reply ! In any case, I'd like someone with more database experience than me to also review this pull request.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ERROR 3780 (HY000) at line 155: Referencing column 'Sex' and referenced column 'Sex' in foreign key constraint 'FK_candidate_sex_1' are incompatible.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding capitalization, the database sure is messy but I think it would be a good idea to maintain a consistent convention in at least the new tables. I am under the impression that this convention is unofficially singular snake_case for the table names and PascalCase for the field names. I am fine with that and this PR matches this convention, but I am still new and this impression of mine may be wrong, so can someone with more database experience than me confirm or deny it ?

That seems to match is the official policy documented in docs/SQLModelingStandard.md but it's so weakly enforced that I had to go look it up.

Copy link
Contributor

@maximemulder maximemulder Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems to match is the official policy documented in docs/SQLModelingStandard.md but it's so weakly enforced that I had to go look it up.

Damn I looked at the documentation but somehow missed this file, thanks for pointing it to me. I forgot my glasses at home today so I guess I'm gonna use that as my excuse 😅

PRIMARY KEY (`SexID`),
UNIQUE KEY `Sex` (`Sex`)
) ENGINE=InnoDB AUTO_INCREMENT=1 DEFAULT CHARSET=utf8mb4 COMMENT='Stores sex options available for candidates in LORIS';

INSERT INTO sex (Sex) VALUES ('Male'), ('Female'), ('Other');

CREATE TABLE `users` (
`ID` int(10) unsigned NOT NULL auto_increment,
`UserID` varchar(255) NOT NULL default '',
Expand Down Expand Up @@ -151,7 +160,7 @@ CREATE TABLE `candidate` (
`DoB` date DEFAULT NULL,
`DoD` date DEFAULT NULL,
`EDC` date DEFAULT NULL,
`Sex` enum('Male','Female','Other') DEFAULT NULL,
`Sex` varchar(255) DEFAULT NULL,
`RegistrationCenterID` integer unsigned NOT NULL DEFAULT '0',
`RegistrationProjectID` int(10) unsigned NOT NULL,
`Ethnicity` varchar(255) DEFAULT NULL,
Expand All @@ -166,7 +175,7 @@ CREATE TABLE `candidate` (
`flagged_other_status` enum('not_answered') DEFAULT NULL,
`Testdate` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
`Entity_type` enum('Human','Scanner') NOT NULL DEFAULT 'Human',
`ProbandSex` enum('Male','Female','Other') DEFAULT NULL,
`ProbandSex` varchar(255) DEFAULT NULL,
`ProbandDoB` date DEFAULT NULL,
PRIMARY KEY (`CandID`),
UNIQUE KEY `ID` (`ID`),
Expand All @@ -175,9 +184,13 @@ CREATE TABLE `candidate` (
KEY `CandidateActive` (`Active`),
KEY `FK_candidate_2_idx` (`flagged_reason`),
KEY `PSCID` (`PSCID`),
KEY `FK_candidate_sex_1` (`Sex`),
KEY `FK_candidate_sex_2` (`ProbandSex`),
CONSTRAINT `FK_candidate_1` FOREIGN KEY (`RegistrationCenterID`) REFERENCES `psc` (`CenterID`),
CONSTRAINT `FK_candidate_2` FOREIGN KEY (`flagged_reason`) REFERENCES `caveat_options` (`ID`) ON DELETE RESTRICT ON UPDATE CASCADE,
CONSTRAINT `FK_candidate_RegistrationProjectID` FOREIGN KEY (`RegistrationProjectID`) REFERENCES `Project` (`ProjectID`) ON UPDATE CASCADE
CONSTRAINT `FK_candidate_RegistrationProjectID` FOREIGN KEY (`RegistrationProjectID`) REFERENCES `Project` (`ProjectID`) ON UPDATE CASCADE,
CONSTRAINT `FK_candidate_sex_1` FOREIGN KEY (`Sex`) REFERENCES `sex` (`Sex`) ON DELETE RESTRICT ON UPDATE CASCADE,
CONSTRAINT `FK_candidate_sex_2` FOREIGN KEY (`ProbandSex`) REFERENCES `sex` (`Sex`) ON DELETE RESTRICT ON UPDATE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

CREATE TABLE `session` (
Expand Down
16 changes: 16 additions & 0 deletions SQL/New_patches/2024-01-29-create-sex-table.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
CREATE TABLE `sex` (
`SexID` int(10) unsigned NOT NULL AUTO_INCREMENT,
`Sex` varchar(255) NOT NULL,
PRIMARY KEY (`SexID`),
UNIQUE KEY `Sex` (`Sex`)
) ENGINE=InnoDB AUTO_INCREMENT=1 DEFAULT CHARSET=utf8mb4 COMMENT='Stores sex options available for candidates in LORIS';

INSERT INTO sex (Sex) VALUES ('Male'), ('Female'), ('Other');

ALTER TABLE candidate
MODIFY COLUMN sex varchar(255) DEFAULT NULL,
MODIFY COLUMN ProbandSex varchar(255) DEFAULT NULL,
ADD KEY `FK_candidate_sex_1` (`Sex`),
ADD KEY `FK_candidate_sex_2` (`ProbandSex`),
ADD CONSTRAINT `FK_candidate_sex_1` FOREIGN KEY (`Sex`) REFERENCES `sex` (`Sex`) ON DELETE RESTRICT ON UPDATE CASCADE,
ADD CONSTRAINT `FK_candidate_sex_2` FOREIGN KEY (`ProbandSex`) REFERENCES `sex` (`Sex`) ON DELETE RESTRICT ON UPDATE CASCADE;
6 changes: 1 addition & 5 deletions modules/candidate_list/jsx/candidateListIndex.js
Original file line number Diff line number Diff line change
Expand Up @@ -282,11 +282,7 @@ class CandidateListIndex extends Component {
name: 'sex',
type: 'select',
hide: this.state.hideFilter,
options: {
'Male': 'Male',
'Female': 'Female',
'Other': 'Other',
},
options: options.Sex,
},
},
{
Expand Down
1 change: 1 addition & 0 deletions modules/candidate_list/php/candidate_list.class.inc
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ class Candidate_List extends \DataFrameworkMenu
'cohort' => $cohort_options,
'participantstatus' => $participant_status_options,
'useedc' => $config->getSetting("useEDC"),
'Sex' => \Utility::getSexList(),
];
}

Expand Down
1 change: 1 addition & 0 deletions modules/candidate_parameters/ajax/getData.php
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ function getProbandInfoFields()
'ageDifference' => $ageDifference,
'extra_parameters' => $extra_parameters,
'parameter_values' => $parameter_values,
'sexOptions' => \Utility::getSexList(),
];

return $result;
Expand Down
15 changes: 10 additions & 5 deletions modules/candidate_parameters/jsx/ProbandInfo.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
import React, {Component} from 'react';
import PropTypes from 'prop-types';
import Loader from 'Loader';
import {
FormElement,
StaticElement,
SelectElement,
ButtonElement,
DateElement,
TextareaElement,
} from 'jsx/Form';

/**
* Proband Info Component.
Expand All @@ -16,11 +24,7 @@ class ProbandInfo extends Component {
super(props);

this.state = {
sexOptions: {
Male: 'Male',
Female: 'Female',
Other: 'Other',
},
sexOptions: {},
Data: [],
formData: {},
updateResult: null,
Expand Down Expand Up @@ -66,6 +70,7 @@ class ProbandInfo extends Component {
formData: formData,
Data: data,
isLoaded: true,
sexOptions: data.sexOptions,
});
},
error: (error) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ class CandidateQueryEngine extends \LORIS\Data\Query\SQLQueryEngine
"Sex",
"Candidate's biological sex",
$candscope,
new \LORIS\Data\Types\Enumeration('Male', 'Female', 'Other'),
new \LORIS\Data\Types\StringType(255),
kongtiaowang marked this conversation as resolved.
Show resolved Hide resolved
new Cardinality(Cardinality::SINGLE),
),
new DictionaryItem(
Expand Down
5 changes: 1 addition & 4 deletions modules/genomic_browser/jsx/tabs_content/cnv.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,7 @@ class CNV extends Component {
filter: {
name: 'Sex',
type: 'select',
options: {
Male: 'Male',
Female: 'Female',
},
options: options.Sex,
},
},
{
Expand Down
5 changes: 1 addition & 4 deletions modules/genomic_browser/jsx/tabs_content/methylation.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,7 @@ class Methylation extends Component {
filter: {
name: 'Sex',
type: 'select',
options: {
Male: 'Male',
Female: 'Female',
},
options: options.Sex,
},
},
{
Expand Down
5 changes: 1 addition & 4 deletions modules/genomic_browser/jsx/tabs_content/profiles.js
Original file line number Diff line number Diff line change
Expand Up @@ -168,10 +168,7 @@ class Profiles extends Component {
filter: {
name: 'Sex',
type: 'select',
options: {
Male: 'Male',
Female: 'Female',
},
options: options.Sex,
},
},
{
Expand Down
5 changes: 1 addition & 4 deletions modules/genomic_browser/jsx/tabs_content/snp.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,7 @@ class SNP extends Component {
filter: {
name: 'Sex',
type: 'select',
options: {
Male: 'Male',
Female: 'Female',
},
options: options.Sex,
},
},
{
Expand Down
1 change: 1 addition & 0 deletions modules/genomic_browser/php/views/cnv.class.inc
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ class CNV
'Sites' => $sites,
'Cohorts' => $cohorts,
'Platform' => $platform_options,
'Sex' => \Utility::getSexList(),
];
}

Expand Down
1 change: 1 addition & 0 deletions modules/genomic_browser/php/views/methylation.class.inc
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ class Methylation
'genotyping_platform',
'Name'
),
'Sex' => \Utility::getSexList(),
];
}

Expand Down
1 change: 1 addition & 0 deletions modules/genomic_browser/php/views/profiles.class.inc
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class Profiles
$this->_formElement = [
'Sites' => $sites,
'Cohorts' => $cohorts,
'Sex' => \Utility::getSexList(),
];
}

Expand Down
1 change: 1 addition & 0 deletions modules/genomic_browser/php/views/snp.class.inc
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ class SNP
'Sites' => $sites,
'Cohorts' => $cohorts,
'Platform' => $platform_options,
'Sex' => \Utility::getSexList(),
];
}

Expand Down
13 changes: 5 additions & 8 deletions modules/new_profile/php/new_profile.class.inc
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,11 @@ class New_Profile extends \NDB_Form
$ageMin = $config->getSetting('ageMin');
$dobFormat = $config->getSetting('dobFormat');
$edc = $config->getSetting('useEDC');
$sex = [
'Male' => 'Male',
'Female' => 'Female',
'Other' => 'Other',
];
$pscidSet = "false";
$minYear = (isset($startYear, $ageMax)) ? $startYear - $ageMax : null;
$maxYear = (isset($endYear, $ageMin)) ? $endYear - $ageMin : null;
$sex = \Utility::getSexList();

$pscidSet = "false";
$minYear = (isset($startYear, $ageMax)) ? $startYear - $ageMax : null;
$maxYear = (isset($endYear, $ageMin)) ? $endYear - $ageMin : null;

// Get sites for the select dropdown
$user_list_of_sites = $user->getCenterIDs();
Expand Down
17 changes: 17 additions & 0 deletions php/libraries/Utility.class.inc
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,23 @@ class Utility
return $result;
}

/**
* Returns list of sex options in the database
*
* @return array An associative array of sex values.
*/
static function getSexList(): array
{
$factory = NDB_Factory::singleton();
$DB = $factory->database();

$query = "SELECT Sex FROM sex";

$result = $DB->pselectCol($query, []);

return array_combine($result, $result);
}

/**
* Returns a list of sites in the database
*
Expand Down
19 changes: 9 additions & 10 deletions src/StudyEntities/Candidate/Sex.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,7 @@ class Sex implements \JsonSerializable
/* @var string */
public $value;

private const VALID_VALUES = array(
'Male',
'Female',
'Other',
);
private $validValues = [];

/**
* Calls validate() immediately.
Expand All @@ -43,10 +39,12 @@ class Sex implements \JsonSerializable
*/
public function __construct(string $value)
{
if (!self::validate($value)) {
$this->validValues = \Utility::getSexList();

if (!self::validate($value, $this->validValues)) {
throw new \DomainException(
'The value is not valid. Must be one of: '
. implode(', ', self::VALID_VALUES)
. implode(', ', $this->validValues)
);
}
$this->value = $value;
Expand All @@ -55,13 +53,14 @@ public function __construct(string $value)
/**
* Ensures that the value is well-formed.
*
* @param string $value The value to be validated
* @param string $value The value to be validated
* @param array $laidValues The restricted optional values of $value
*
* @return bool True if the value format is valid
*/
public static function validate(string $value): bool
public static function validate(string $value, array $validValues): bool
{
return in_array($value, self::VALID_VALUES, true);
return in_array($value, $validValues, true);
Copy link
Contributor

@kongtiaowang kongtiaowang Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getSexList will return array(
'Male' => 'Male',
'Female' => 'Female',
'Other' => 'Other'
) so you need to change to [return in_array($value, array_values($validValues), true);]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kongtiaowang Made that change but still having trouble unfortunately!

}

/**
Expand Down
4 changes: 2 additions & 2 deletions tools/CouchDB_Import_Demographics.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class CouchDBDemographicsImporter
],
'Sex' => [
'Description' => 'Candidate\'s biological sex',
'Type' => "enum('Male', 'Female', 'Other')"
'Type' => "varchar(255)"
],
'Site' => [
'Description' => 'Site that this visit took place at',
Expand Down Expand Up @@ -324,7 +324,7 @@ function _updateDataDict()
if ($config->getSetting("useProband") === "true") {
$this->Dictionary["Sex_proband"] = [
'Description' => 'Proband\'s biological sex',
'Type' => "enum('Male','Female', 'Other')"
'Type' => "varchar(255)"
];
$this->Dictionary["Age_difference"] = [
'Description' => 'Age difference between the candidate and ' .
Expand Down
Loading