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

[Consent] New module #7583

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

CamilleBeau
Copy link
Contributor

@CamilleBeau CamilleBeau commented Sep 24, 2021

Brief summary of changes

#7363
New module for consent and eConsent. See "README.md" for more information

  • Have you updated related documentation?

Testing instructions (if applicable)

Run patches & make sure to compile before testing.
See TestPlan

@zaliqarosli zaliqarosli self-requested a review September 27, 2021 18:52
Copy link
Contributor

@zaliqarosli zaliqarosli left a comment

Choose a reason for hiding this comment

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

initial comments

SQL/New_patches/2021-09-22-ConsentModule.sql Outdated Show resolved Hide resolved
`ConsentGroupID` int(10) unsigned NOT NULL,
`CenterID` int(10) unsigned DEFAULT NULL,
`ConsentDisplayID` int(10) unsigned NOT NULL,
CONSTRAINT `FK_consent_group_consent_display_rel_1` FOREIGN KEY (`ConsentGroupID`) REFERENCES `consent_group` (`ConsentGroupID`) ON DELETE CASCADE ON UPDATE CASCADE,
Copy link
Contributor

Choose a reason for hiding this comment

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

would be good to specifiy what the primary keys are here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most rel tables have every field as a primary key. However, in this case CenterID can not be included in the primary key since it is nullable. It also cannot be (ConsentGroupID,ConsentDisplayID) because it's possible that these columns will need to hold the same values with a different CenterID. Do you have a suggestion for what the primary key should be? I think it would be good to have a constraint that ensures no duplicate rows are entered in this table but I'm having some trouble implementing it

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds like the PK combination needs to be (ConsentGroupID,ConsentDisplayID,CenterID). Is there a reason why CenterID is nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Null CenterID is default, i.e. it represents all sites. The CenterID would not be null if there is a site-specific change in the consent form. For example, if all sites have the same consent form except for site A, then there would be a row with a null CenterID linking to the general consent form, and a row with the CenterID for site A linking to the site-specific consent form.

SQL/New_patches/2021-09-22-ConsentModule.sql Outdated Show resolved Hide resolved

-- Relates different sites and consent groups to their
-- respective displays
CREATE TABLE `consent_display_rel` (
Copy link
Contributor

Choose a reason for hiding this comment

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

so a consent group can have multiple displays and display IDs can also be attached to different consent groups right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct!

Copy link
Contributor

Choose a reason for hiding this comment

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

can you give me an example of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, for CTU, the REB for UBC has approved different wording in the eConsent form than other sites. There are also different acknowledgements that the candidate has to submit. These are all written in the z-json, so a different file is written for sites that have unique needs. This table points to the right row in consent_display that holds the name of the zjson file relevant to that site. It is possible that multiple sites will have the same form, whereas others have a different form, so multiple sites may need to link to the same display. Null centerID indicates the default (we discussed this in one of our meetings). However, I don't see a reason why different consent groups would need the same display although I suppose if it's a generic display where people want wording such as "Please enter consent for the following:", then it is possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

as far as i can understand, it looks like there is a many to many relationship between consent group and displays, when there should maybe be a 1 to many in the direction of 1 consent group to many displays (based on site). is this something you could address if you haven't already?

Comment on lines +21 to +22
`CandidateID` int(6) NOT NULL,
`ConsentGroupID` integer unsigned 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.

I wonder if this should be DirectConsentID as the primary key, that is referenced as a foreign key from the candidate_consent_rel table i.e. add DirectConsentID as a column in the candidate_consent_rel table.

did we already discuss that option and shut it down for a reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I don't think we discussed that option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just thought about this a bit more. I'm not sure that candidate_consent_rel is the best place to have a DirectConsentID since entries in the candidate_consent_rel are not necessarily eConsent related, and also the table is not organized by consent group. I feel like it complicates the use of the tables

Copy link
Contributor

@zaliqarosli zaliqarosli Nov 8, 2021

Choose a reason for hiding this comment

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

DirectConsentID here would be for the direct_consent table, not the candidate_consent_rel table

@maltheism maltheism added the Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch label Oct 17, 2022
@CamilleBeau CamilleBeau added the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Dec 1, 2022
@CamilleBeau CamilleBeau added Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch and removed Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch labels Dec 6, 2022
@CamilleBeau CamilleBeau removed Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix labels Dec 21, 2023
$query = "SELECT ConsentID, Name, Status, DateGiven, DateWithdrawn, Label
FROM candidate_consent_rel cc JOIN consent c USING (ConsentID)
WHERE CandidateID=:cid";
$query = "SELECT
Copy link
Contributor

Choose a reason for hiding this comment

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

@CamilleBeau please update the unittest in the /var/www/loris/test/unittests/CandidateTest.php line 1052 to 1065
$result = [
['ConsentID' => 1,
'Name' => 'name1',
'Status' => 'done',
'DateGiven' => 'today',
'DateWithdrawn' => 'tomorrow',
'Label' => 'label',
'Comment' => 'comment'
]
];

    $this->_dbMock->expects($this->once())
        ->method('pselectWithIndexKey')
        ->with(
            $this->stringContains(
                "SELECT ConsentID, Name, Status, DateGiven, DateWithdrawn, Label, Comment"
            )
        )
        ->willReturn($result);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks so much for your help @kongtiaowang !! Unfortunately I made this change but am still getting a similar failure in the tests

@kongtiaowang
Copy link
Contributor

@CamilleBeau
kongtiaowang#383
the last two commits fixed the issue

@CamilleBeau
Copy link
Contributor Author

@kongtiaowang Thanks for your help, I added the changes from those 2 commits but I'm still getting the error in test suite

@ridz1208 ridz1208 added the Priority: High PR or issue should be prioritised over others for review and testing label Jul 2, 2024
@@ -213,6 +213,9 @@ DROP TABLE IF EXISTS `psc`;
DROP TABLE IF EXISTS `visit_project_cohort_rel`;
DROP TABLE IF EXISTS `visit`;
DROP TABLE IF EXISTS `project_cohort_rel`;
DROP TABLE IF EXISTS `consent_display_rel`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing "candidate_consent_history" in the drop table list.

@@ -2297,8 +2297,9 @@ CREATE TABLE `candidate_consent_rel` (
`CandidateID` int(6) NOT NULL,
`ConsentID` integer unsigned NOT NULL,
`Status` enum('yes','no', 'not_applicable') DEFAULT NULL,
`DateGiven` date DEFAULT NULL,
`DateWithdrawn` date DEFAULT NULL,
`DateGiven` datetime DEFAULT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

rm line 2308-2359 "candidate_consent_history" create table twice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High PR or issue should be prioritised over others for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants