[Candidate_parameters, Core] Normalizing Consent in LORIS #3504

zaliqarosli · 2018-02-20T19:54:03Z

This PR transforms the current implementation of consent into a normalized structure. Current behaviour on the front-end shouldn't be affected.

Changes targeted:

create normalized tables
implement SQL modelling standard
move data into new tables / update calls to DB
move config settings from config.xml to DB

Discussions required:

on options for statuses of consent (to enum or not to enum)
on default status and coalescing when querying consent in DQT

Moving forward:

The ajax scripts are a little hacky in order to be compatible with changes in the back-end while keeping the front-end untouched. The next step would be to clean up how consent is used in the front-end and fix up the ajax scripts in parallel.

See also: #3263, #3315, #3348

How to test:

Test front-end Candidate Parameters->Consent Status (pre-PR)
Source SQL patch - check tables and settings are created
Run Import_Data_Normalise_Consent.php - check new tables are populated when successful
Make backup of participant_status and consent_info_history and drop deprecated columns/tables in DB as instructed by above script
Test front-end on PR branch - there should be no changes in functionality/display
Test consent status tab only shows when ‘Use Consent’ in Config module enabled
Run CouchDB_Import_Demographics.php
Query demographics instrument in DQT - check results are what you expect from DB

…_normalise_consent

ridz1208 · 2018-02-21T15:20:30Z

@llevitis just putting #3315 here so it doesn't get forgotten

zaliqarosli · 2018-02-21T20:50:52Z

tools/Import_Data_Normalise_Consent.php

+foreach ($consents as $key=>$consent) {
+    // Do consent columns exist in old table?
+    $consentName = $consent['name'];
+    $columnQuery = 'SHOW COLUMNS FROM participant_status LIKE "' . $consentName . '"';


@ridz1208 at the moment, only the columns with status information is being checked for existence in participant_status. do you think this is enough or should we also be checking for the existence of the _date and _withdrawal columns too?

the check shouldbe done on all columns since you dont want to assume that they exist... just to be sure.

ridz1208 · 2018-02-21T20:55:07Z

tools/Import_Data_Normalise_Consent.php

+    // Do consent columns exist in old table?
+    $consentName = $consent['name'];
+    $columnQuery = 'SHOW COLUMNS FROM participant_status LIKE "' . $consentName . '"';
+    $columnExists = $db->pselect($columnQuery, array());


You might wan to use the database column exist function !!

xlecours · 2018-02-20T20:19:37Z

SQL/0000-00-00-schema.sql

+CREATE TABLE `candidate_consent_type_history` (
+  `CandidateConsentHistoryID` int(10) unsigned NOT NULL AUTO_INCREMENT,
+  `PSCID` varchar(255) NOT NULL,
+  `ConsentName` varchar(255) NOT NULL,


This should probably be a foreign key on consent_type. right?

@xlecours similar to the old consent_info_history table, this table should only be a log of data submitted on the front-end, unaffected by changes in data of the other consent tables, but with more meaningful fields for users reading the history on the front-end

Ok. The primary goal of this PR is to remove the consent from config.xml and not to redesign the way we log events.

@xlecours a secondary goal, which I think is just as important, is to implement the new SQL modelling standard, hence the renovation of the existing consent history table

PapillonMcGill · 2018-02-22T14:08:08Z

SQL/Archive/2017-11-01-use_consent_to_database.sql

+  `CandidateID` int(6) NOT NULL,
+  `ConsentTypeID` int(2) NOT NULL,
+  `Status` enum('yes', 'no', 'not_answered') NOT NULL,
+  `DateGiven` date DEFAULT NULL,


as per https://github.com/aces/Loris/blob/19.0-dev/docs/SQLModelingStandard.md (Which should be propagated to minor),
ENUM are not permitted.

Could use this occasion to add a table "option" with "TableName", "AttributeName", "Value" attributes. This would create the table to replace all ENUM within the DB. Don't forget the (artificial) primary key.

Im not sure about this design... I actually told zaliqa to keep it as an enum until we discuss how to tackle it.

Sever places will use the yes/no options in loris and im not sure if those options should also include "not answered" or other options.

Id like to bring this up at the loris meeting so it can be discussed with everyone

yepp, let's put it through as enum for now until a standard for all options in the database is decided on

Excellent idea, we could create a class for that so it be easy on projects to use the feature. I should work on that after I'm finish with visit

the reason for yes/no enums in LORIS is that MySQL mutilates "boolean" to tinyint(1) and changes true/false to 1/0.

Maybe we should document that and update the SQL standards? (not answered doesn't make sense though)

yeah i agree, i wanted to talk about the not answered option at tomorrow's meeting

PapillonMcGill · 2018-02-22T14:10:34Z

SQL/Archive/2017-11-01-use_consent_to_database.sql

+  CONSTRAINT `PK_candidate_consent_type_rel` PRIMARY KEY (`CandidateID`,`ConsentTypeID`),
+  CONSTRAINT `FK_candidate_consent_type_rel_CandidateID` FOREIGN KEY (`CandidateID`) REFERENCES `candidate` (`CandID`),
+  CONSTRAINT `FK_candidate_consent_type_rel_ConsentTypeID` FOREIGN KEY (`ConsentTypeID`) REFERENCES `consent_type` (`ConsentTypeID`)
+) ENGINE=InnoDB DEFAULT CHARSET=utf8;


FK should containt "on cascade" and "on delete" clause

@PapillonMcGill did you mean "on update" and "on delete"? i dont think there's an "on cascade"

@zaliqarosli i would assume so, and on that note, you should update the delete_candidate php script to delete the fields from the tables you added

@zaliqarosli yes, on delete and on update. Look like I write faster than I think. Must be in need of vacation ;-)

@ridz1208 should we maybe fix #3348 first or update it to include consent tables once this PR goes through? i feel like changes to delete_candidate script should be separate from this consent PR. thoughts?

PapillonMcGill · 2018-02-22T14:11:50Z

SQL/Archive/2017-11-01-use_consent_to_database.sql

+  `DateWithdrawn` date DEFAULT NULL,
+  `EntryStaff` varchar(255) DEFAULT NULL,
+  `EntryDate` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
+  CONSTRAINT `PK_candidate_consent_type_history` PRIMARY KEY (`CandidateConsentHistoryID`)


varchar() should be after date and timestamp field.
no ENUM

could you explain why the varchar comes after the date and timestamp?

@zaliqarosli For efficiency, if DB engine want to read a field placed after, it as to traverse the varchar() until the end. Can't jump directly to the required field start as the offset from row start in not fixed.
I'm preparing a presentation on that for an ADM.

ahh okay i think i know what you mean. looking forward to the adm!

PapillonMcGill · 2018-02-22T15:21:03Z

php/libraries/Utility.class.inc

+    static function getConsentList()
+    {
+        $DB =& Database::singleton();
+


should be
$factory = NDB_Factory::singleton();
$db = $factory->database();

@PapillonMcGill could you explain this to me please? I've seen both used and not sure what the difference is

@PapillonMcGill i do see that the other functions in Utility follow the way you mentioned

@zaliqarosli For more info, should ask @xlecours. I saw that he made a PR #3507 for that. I just wanted to do all uniformly.
Suspect it as to do with uniformity to simplify testing

PapillonMcGill · 2018-02-22T15:23:07Z

tools/CouchDB_Import_Demographics.php

        $fieldsInQuery = "SELECT c.DoB,
-                                c.CandID, 
+                                 c.CandID, 
                                c.PSCID, 


extra space!

PapillonMcGill · 2018-02-22T15:24:08Z

tools/CouchDB_Import_Demographics.php

-							c.CandID, 
+                            c.DoB, 
+		            c.CandID, 
                            c.PSCID, 


extra space at end of line 147
missing space at line 148

@PapillonMcGill thanks for your review! will make these changes :)

PapillonMcGill · 2018-02-22T18:22:50Z

SQL/Archive/2017-11-01-use_consent_to_database.sql

+  `EntryDate` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
+  `EntryStaff` varchar(255) DEFAULT NULL,
+  CONSTRAINT `PK_candidate_consent_type_history` PRIMARY KEY (`CandidateConsentHistoryID`)
+) ENGINE=InnoDB DEFAULT CHARSET=utf8;


still some varchar() before date and timestamp,
Nullable should be after not null

i think i need more coffee

zaliqarosli · 2018-02-22T18:47:16Z

SQL/Archive/2017-11-01-use_consent_to_database.sql


 CREATE TABLE `candidate_consent_type_history` (
  `CandidateConsentHistoryID` int(10) unsigned NOT NULL AUTO_INCREMENT,
+  `EntryDate` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,


@PapillonMcGill did i get it right this time? lol

xlecours · 2018-02-22T19:53:38Z

php/libraries/Candidate.class.inc

+        $factory = NDB_Factory::singleton();
+        $db      = $factory->database();
+
+        $query       = "SELECT *


Please specify the columns.

They are no all used

If some get changed in the schema, it is not obvious that it was used here?

If this need to join other tables, it will gather all those new table's columns

xlecours · 2018-02-22T19:53:50Z

php/libraries/Utility.class.inc

+    {
+        $DB =& Database::singleton();
+
+        $query = "SELECT * FROM consent_type";


Please specify the columns.

xlecours · 2018-02-22T19:54:21Z

tools/Import_Data_Normalise_Consent.php

+  } else {
+      // Check for zero dates
+      $psData = $db->pselect(
+                  'SELECT * FROM participant_status WHERE ' . $consentName . ' IS NOT NULL OR ' . $consentName . ' != ""',


Please specify the columns.

xlecours · 2018-02-22T19:54:30Z

tools/Import_Data_Normalise_Consent.php

+
+  // Get all data where the consent status has a value
+  $psData = $db->pselect(
+              'SELECT * FROM participant_status WHERE '


Please specify the columns.

xlecours · 2018-02-22T19:54:38Z

tools/Import_Data_Normalise_Consent.php

+
+// Select consent history and import into new history table
+$consentHistory = $db->pselect(
+    'SELECT * FROM consent_info_history',


Please specify the columns.

i already said I AGREE !

@xlecours thank you for your input!! i made the changes for all the other queries but for this one, the field names in the old history table are dynamically set by the consent types. I'll specify the predicted column names here but it'll be in a concatenated query

xlecours · 2018-02-22T19:55:54Z

modules/candidate_parameters/ajax/getData.php

-
-        array_push($commentHistory, $unformattedComments);
+    $historyData = $db->pselect(
+        "SELECT * 


Please specify the columns.

zaliqarosli · 2018-02-23T01:08:21Z

tools/Import_Data_Normalise_Consent.php

+// Select consent history and import into new history table
+$historyFieldsQuery = 'SELECT CandID, entry_staff, data_entry_date';
+foreach ($consentType as $consentName => $consentLabel) {
+    $historyFieldsQuery .= ', ' . $consentName . ', ' . $consentName . '_date, ' . $consentName . '_withdrawal';


@xlecours let me know what you think

driusan

some preliminary review, but this is too big to review in one sitting. Can you try and split this up into multiple smaller self-contained PRs?

The schema+import script would make a good start for a first one without touching any other code.

driusan · 2018-02-20T20:53:39Z

SQL/0000-00-00-schema.sql

+CREATE TABLE `candidate_consent_type_rel` (
+  `CandidateID` int(6) NOT NULL,
+  `ConsentTypeID` int(2) NOT NULL,
+  `Status` enum('yes', 'no', 'not_answered') NOT NULL,


No enums in the core tables is the new rule. yes, no, not_answered NOT NULL also doesn't really make sense. It should just be yes, no, NULLABLE, and (at that point it's just a boolean, not an enum, which makes more sense)

driusan · 2018-02-20T20:54:14Z

SQL/0000-00-00-schema.sql

+  `PSCID` varchar(255) NOT NULL,
+  `ConsentName` varchar(255) NOT NULL,
+  `ConsentLabel` varchar(255) NOT NULL,
+  `Status` enum('yes','no','not_answered') NOT NULL,


to enum or not to enum appears to be a point of contention.. to be determined hopefully tomorrow

driusan · 2018-02-20T20:55:16Z

SQL/0000-00-00-schema.sql

+  `DateGiven` date DEFAULT NULL,
+  `DateWithdrawn` date DEFAULT NULL,
+  `EntryStaff` varchar(255) DEFAULT NULL,
+  `EntryDate` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,


The history should never be updated, why does this have an ON UPDATE clause?

as suggested by @PapillonMcGill and the new SQL modelling standard

driusan · 2018-02-20T20:56:30Z

SQL/0000-00-03-ConfigTables.sql

-INSERT INTO ConfigSettings (Name, Description, Visible, AllowMultiple, DataType, Parent, Label, OrderNumber) SELECT 'allowPrenatalTimepoints', 'Determines whether creation of timepoints prior to Date of Birth is allowed', 1, 0, 'boolean', ID, 'Allow Prenatal Timepoints', 21 FROM ConfigSettings WHERE Name="study";
-INSERT INTO ConfigSettings (Name, Description, Visible, AllowMultiple, DataType, Parent, Label, OrderNumber) SELECT 'ImagingUploaderAutoLaunch', "Allows running the ImagingUpload pre-processing scripts", 1, 0, 'boolean', ID, 'ImagingUploader Auto Launch',22 FROM ConfigSettings WHERE Name="study";
-INSERT INTO ConfigSettings (Name, Description, Visible, AllowMultiple, DataType, Parent, Label, OrderNumber) SELECT 'citation_policy', 'Citation Policy for Acknowledgements module', 1, 0, 'textarea', ID, 'Citation Policy', 23 FROM ConfigSettings WHERE Name="study";
-INSERT INTO ConfigSettings (Name, Description, Visible, AllowMultiple, DataType, Parent, Label, OrderNumber) SELECT 'CSPAdditionalHeaders', 'Extensions to the Content-security policy allow only for self-hosted content', 1, 0, 'text', ID, 'Content-Security Extensions', 24 FROM ConfigSettings WHERE Name="study";


Why is all this changed?

@ridz1208 wanted to update the order of the statements

driusan · 2018-02-20T20:57:55Z

modules/candidate_parameters/ajax/formHandler.php

+    $consentDetails   = Utility::getConsentList(); 
+
+    // Get list of consents for candidate
+    $candidateConsent = Candidate::getConsent($candID);


This isn't (and shouldn't be) defined as a static function below

what do you mean? could you be more explicit please?

driusan · 2018-02-20T21:02:58Z

php/libraries/Candidate.class.inc

+     * @return array List of consents and their associated values for this candidate.
+     *               The keys of the arrays are the IDs of the consents
+     */
+    function getConsent($candID)


It doesn't make sense for any functions in the Candidate class to take a candID as a parameter. It's the candidate class, it already knows what it's candID is.

This should probably have a consent type parameter instead though, shouldn't it?

driusan · 2018-02-20T21:03:53Z

php/libraries/Candidate.class.inc

+        $factory = NDB_Factory::singleton();
+        $db      = $factory->database();
+
+        $query       = "SELECT *


Select * should be avoided in favour of explicit column names, otherwise the code is going to be brittle with schema changes.

oh yes, @xlecours pointed that out for other statements, i missed these ones

driusan · 2018-02-20T21:04:12Z

php/libraries/Utility.class.inc

+    {
+        $DB =& Database::singleton();
+
+        $query = "SELECT * FROM consent_type";


Same comment, avoid SELECT *

@driusan cool, thanks for all your comments. i'll make the changes and work on sending out smaller, more contained PRs

driusan · 2018-02-20T21:04:40Z

php/libraries/Utility.class.inc

+        foreach ($result as $row) {
+            $list[$row["ConsentTypeID"]] = $row;
+            unset($list[$row["ConsentTypeID"]]["ConsentTypeID"]);
+        }


Didn't @ridz1208 recently add a variant of pselect to make this easier?

@ridz1208 is he talking about pselectCol? i forgot what the above foreach statement does

i think pselectcolwithindex something

Here's a reference to the PR #3428 - The function is called 'pselectWithIndexKey()'

driusan · 2018-02-26T15:41:29Z

SQL/Archive/2017-11-01-use_consent_to_database.sql

+  `CandidateID` int(6) NOT NULL,
+  `ConsentTypeID` int(2) NOT NULL,
+  `Status` enum('yes', 'no', 'not_answered') NOT NULL,
+  `DateGiven` date DEFAULT NULL,


the reason for yes/no enums in LORIS is that MySQL mutilates "boolean" to tinyint(1) and changes true/false to 1/0.

Maybe we should document that and update the SQL standards? (not answered doesn't make sense though)

PapillonMcGill · 2018-03-05T13:23:25Z

SQL/0000-00-00-schema.sql

+  `ConsentTypeID` int(2) NOT NULL AUTO_INCREMENT,
+  `Name` varchar(255) NOT NULL,
+  `Label` varchar(255) NOT NULL,
+  CONSTRAINT `PK_consent_type` PRIMARY KEY (`ConsentTypeID`),


Do these 2 varchar() field need 255 chars?
What is the practical / forecast max length for the name and label?
Would 15 or 20 for Name and 20 or 30 for Label be sufficient?

New PR #3532 if you could redirect reviews there

PapillonMcGill · 2018-03-05T13:51:09Z

tools/CouchDB_Import_Demographics.php

-                            c.DoB,
-							c.CandID, 
+                            c.DoB, 
+                            c.CandID, 


extra space at end of line

ridz1208 and others added 30 commits November 7, 2017 16:12

base

4448434

candidate_params

c5b8fc4

sql

9de9c0e

Resolve conflict

13d8dd2

Update schema and patch to follow modeling standard

5de0dc2

Fix front end and ajax call

b04cd66

Merge branch 'minor' of https://github.com/aces/loris into 2017_11_01…

5344499

…_normalise_consent

Update getConsent functions

acd5246

Attemp implement vertical tab

73035cd

Remove comments

5083022

Update schema and add patch

e3397dd

Update ajax

b679b25

Remove changes to jsx

39aad5d

Update SQL

46dd6d0

Update ajax

0dffc99

Update ajax

f63acfb

Restore original Tabs.js

cc03312

Remove any changes to js

9e5c957

Fix bug when submitting

2fd2640

Define history array

87761ba

Update null check

71a9736

Fix typo

026b858

Fix typo

0fadedc

Fix typo

63c5469

Remove default null for enum

478c173

Start data transfer script

39d6a91

Update import script

37cda35

Latest version

dd48461

First final edit of script

747b0f2

Fix some typos

1827a26

Fix demographics query

70df708

zaliqarosli removed the State: Needs work PR awaiting additional work by the author to proceed label Feb 21, 2018

Add date of consent to query

d190c8e

zaliqarosli commented Feb 21, 2018

View reviewed changes

ridz1208 requested changes Feb 21, 2018

View reviewed changes

xlecours requested changes Feb 21, 2018

View reviewed changes

Add check for date, withdrawal columns in table

a4375be

PapillonMcGill suggested changes Feb 22, 2018

View reviewed changes

PapillonMcGill reviewed Feb 22, 2018

View reviewed changes

tools/CouchDB_Import_Demographics.php

$fieldsInQuery = "SELECT c.DoB,

c.CandID,

c.CandID,

c.PSCID,

Copy link

Contributor

PapillonMcGill Feb 22, 2018

Choose a reason for hiding this comment

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

extra space!

PapillonMcGill reviewed Feb 22, 2018

View reviewed changes

zaliqarosli added 2 commits February 22, 2018 12:41

Fix spaces and alignment

332947a

Make requested changes to SQL and Utiliy function

3b1a6fc

PapillonMcGill reviewed Feb 22, 2018

View reviewed changes

Implement SQL standard

56e1f46

zaliqarosli commented Feb 22, 2018

View reviewed changes

xlecours requested changes Feb 22, 2018

View reviewed changes

zaliqarosli added 2 commits February 22, 2018 19:04

Specify column names in queries

2272f74

phpcs

5abb68c

zaliqarosli commented Feb 23, 2018

View reviewed changes

zaliqarosli mentioned this pull request Feb 23, 2018

using tosql now generate only 1 file with ordered instruction #3348

Merged

driusan requested changes Feb 26, 2018

View reviewed changes

zaliqarosli added State: Needs work PR awaiting additional work by the author to proceed State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed labels Feb 26, 2018

zaliqarosli mentioned this pull request Mar 2, 2018

[Candidate_parameters, Core] Normalizing Consent SQL 1/2 #3532

Merged

4 tasks

PapillonMcGill reviewed Mar 5, 2018

View reviewed changes

tools/CouchDB_Import_Demographics.php

c.DoB,

c.CandID,

c.DoB,

c.CandID,

Copy link

Contributor

PapillonMcGill Mar 5, 2018

Choose a reason for hiding this comment

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

extra space at end of line

zaliqarosli closed this Mar 5, 2018

zaliqarosli deleted the normalise_consent branch April 24, 2019 17:45

[Candidate_parameters, Core] Normalizing Consent in LORIS #3504

[Candidate_parameters, Core] Normalizing Consent in LORIS #3504

Uh oh!

Conversation

zaliqarosli commented Feb 20, 2018 • edited by HenriRabalais Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ridz1208 commented Feb 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zaliqarosli Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PapillonMcGill Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zaliqarosli Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zaliqarosli Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

zaliqarosli commented Feb 20, 2018 •

edited by HenriRabalais

Loading

zaliqarosli Feb 22, 2018 •

edited

Loading

PapillonMcGill Feb 22, 2018 •

edited

Loading

zaliqarosli Feb 22, 2018 •

edited

Loading

zaliqarosli Feb 22, 2018 •

edited

Loading