Loris
Loris copied to clipboard
aces
[Candidate_parameters] Add Not Applicable option to consent
Brief summary of changes
~This PR makes the date of consent only required when consent is 'yes'. If the consent is 'no', users can still enter a date if they choose, but no error message is thrown and they are able to save if they do not enter dates. If they do enter dates, they still should not be able to save unless the dates match.~
This PR adds the Not Applicable option for consent always instead of only when it existed before. In this case, dates can not be entered.
Testing instructions (if applicable)
See text plan
Link(s) to related issue(s)
This is a CCNA override - https://github.com/aces/CCNA/pull/3822
@miladheshmati if I'm not mistaken this item has been discussed and a "no" consent date is as important as a "yes" consent date. consider scenarios where someone answered "yes" but then changed it to "no" it's very important to log the dates where the consent changed. Also, even a "no" consent on it's own is very important to log when it was given.
What is your rational in removing this ?
I'm with @ridz1208 on this. The date of consent refusal (answering "no") is probably more important than the date of granting consent (answering "yes"). Why was this implemented on CCNA? It seems like a really bad idea.
Update according to decision made in August of last year: This PR does not need a change and should be tested / reviewed as is.
Update according to decision made in August of last year: This PR does not need a change and should be tested / reviewed as is.
@CamilleBeau, So I should just fix it so that the tests that are failing now to pass?
Update according to decision made in August of last year: This PR does not need a change and should be tested / reviewed as is.
@CamilleBeau, So I should just fix it so that the tests that are failing now to pass?
Yes please :)
@driusan when I try to access the logs for the failed tests, it says "The logs for this run have expired and are no longer available." Is there a way that I can rerun the tests to see the problem?
@driusan @CamilleBeau, Thank you. The tests are passed and the PR is ready to be reviewed.
Can we document the use case / reason in the module's README file? It still seems counter productive and it would be good to have an easily available explanation in the specs rather than having to dig through meeting minutes to find it.
@miladheshmati See above comment. Can you update the module's README with the reasoning for this?
@miladheshmati See above comment. Can you update the module's README with the reasoning for this?
@driusan, this PR adds an option to the consent status to be able to set it to Not Applicable, In the README file, there is no part talking about the options, Should I add it to "Configurations" section maybe?
Hm.. you're right, there's no good section to put in in under our standard module spec headings. I would add a subheading under Purpose. I think that would make more sense than configurations since there's nothing to configure.
Hm.. you're right, there's no good section to put in in under our standard module spec headings. I would add a subheading under Purpose. I think that would make more sense than configurations since there's nothing to configure.
@driusan, are you going to add it to this PR? I can do it if you help me with the title.
@miladheshmati See above comment. Can you update the module's README with the reasoning for this?
@driusan, this PR adds an option to the consent status to be able to set it to Not Applicable, In the README file, there is no part talking about the options, Should I add it to "Configurations" section maybe?
@miladheshmati I'm confused, I don't see the not applicable option being added to the dropdown of consent options? how do you set it to not applicable? also, the parts where you are changing the code have a bunch of code comments that are not being modified, are those still accurate or are we just adding confusion here?
Here are minutes from the last conversation had about this
Santiago Torres
[12:08 PM](https://mcin.slack.com/archives/C05NZ20DU9H/p1692720494791199)
Hi All, just to follow-up on the discussion with regards to adding the date for the 'NA' option for a given consent. I guess my understanding of this is that this would be a cohort wide status, so not actually a decision made by the participant, hence no date for the 'NA' decision (The date would have been externally established in the IRB).
[12:08](https://mcin.slack.com/archives/C05NZ20DU9H/p1692720506595209)
That is, a given participant could be part of larger study/project that would normally include consent 'x', but the participant is part of a cohort/subproject that are 'NA' for this consent 'x'.
[12:08](https://mcin.slack.com/archives/C05NZ20DU9H/p1692720525762509)
If so, this is not a decision made by the participant, rather a condition of the cohort to which the participant belongs to, so implicitly being excluded/NA for the consent. In some ways this loops back to the consent falling under 'Project' vs 'Sub-project' idea. If the consent is based on 'Sub-project', then the consent may simply not be displayed for the given cohort and 'NA' is not even necessary.
ridz
[7:51 PM](https://mcin.slack.com/archives/C05NZ20DU9H/p1692834670754939)
Just to prove
[@charliehenrib](https://mcin.slack.com/team/U1NMF5MDK)
wrong, I won't argue this any further. If it makes sense for studies to not sign the date where the consent was deemed not applicable then so be it.
:laughing:
1
charliehenrib
[7:53 PM](https://mcin.slack.com/archives/C05NZ20DU9H/p1692834784840899)
I'm with Santiago with this one :)
[7:53](https://mcin.slack.com/archives/C05NZ20DU9H/p1692834810877019)
So no date... Good luck sleeping at night
[@ridz](https://mcin.slack.com/team/U0JJ6DYRX)
ridz
[7:53 PM](https://mcin.slack.com/archives/C05NZ20DU9H/p1692834824564149)
who am I to argue with democracy
[7:54](https://mcin.slack.com/archives/C05NZ20DU9H/p1692834844383589)
I will sleep better knowing that I will make you regret this decision when the situation arises
[7:54](https://mcin.slack.com/archives/C05NZ20DU9H/p1692834893678499)
I may have conceded, but I will not forget
[7:55](https://mcin.slack.com/archives/C05NZ20DU9H/p1692834955361219)
On my grave you will read "here lies Rida - I told you so - Abou Haidar"
What is CCNA doing now if the override was removed but the code wasn't changed?
@driusan I mean.... I told you so just doesn't quite say it anymore... they reverted to logic that makes sense :stuck_out_tongue:
- here lies Rida - I told you so - Abou Haidar
@ridz1208 Sorry, as discussed on slack this change does still need to be made. Should proceed with merge
@CamilleBeau was the change restored on CCNA ? Just asking to see how rigorously tested this need to be (if it's actively used on CCNA maybe someone from CCNA can give it a good test)?
@CamilleBeau was the change restored on CCNA ? Just asking to see how rigorously tested this need to be (if it's actively used on CCNA maybe someone from CCNA can give it a good test)?
@ridz1208 This change still exists on CCNA and wasn't removed, the change that was mistakenly (?) removed on CCNA was the not_applicable option being added. I can retest this though since a lot of time has passed!
@CamilleBeau the first few comments reject the removal of a required date for a no consent. I think the title of the PR is misleading.
both yes and no consents should require a date, I don't think there was a discussion on this. the only discussion was around the addition of the NA option (with no date by the request of Charlie and Santiago)
https://github.com/aces/Loris/pull/8809#pullrequestreview-1871423996
@CamilleBeau the first few comments reject the removal of a required date for a no consent. I think the title of the PR is misleading.
both yes and no consents should require a date, I don't think there was a discussion on this. the only discussion was around the addition of the NA option (with no date by the request of Charlie and Santiago)
Gotcha. I renamed the PR and modified the description. If I understand correctly, this means we just need to add the N/A option. It is already the case that dates are not required for N/A, but are required for yes / no.
@CamilleBeau sorry I don't remember the technical part of this PR fully, aren't there required changes for making the date required or not based on the selection. And had we made a call on being able from going from yes to NA or NA to yes or No to NA ...
@SantiagoTG @charliehenrib may have an opinion there
@CamilleBeau sorry I don't remember the technical part of this PR fully, aren't there required changes for making the date required or not based on the selection. And had we made a call on being able from going from yes to NA or NA to yes or No to NA ...
@SantiagoTG @charliehenrib may have an opinion there
The code is already suited so that dates can not be entered for N/A, so there were no changes needed for this.
Currently a date or withdrawal date is not required for going from yes or no to NA, which I think makes sense because it doesn't seem explicitly like a withdrawal.