Skip to content

Conversation

jaycarlton
Copy link
Contributor

@jaycarlton jaycarlton commented Oct 24, 2019

Updated DUA content. Also broke a couple of things into separate classes.

Getting the formatting perfect is beyond the scope of this story, but I do need to see if someone can help me look for typos. The source document for the new DUA is here.

Copy link
Contributor

@s-rubenstein s-rubenstein left a comment

Choose a reason for hiding this comment

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

There's a number of whitespace issues, I offered some line break fixes, but you could also fix them by putting the explicit space character. I think &nbsp?

<div>By entering my initials next to each statement below, I agree to these terms:</div>
<InitialsAgreement onChange={(v) => this.setState({initialWork: v})} value={initialWork}>
My work may be logged and monitored by the <i>All of Us</i> Research Program to ensure
compliance with policies and procedures.
Copy link
Contributor

Choose a reason for hiding this comment

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

Before the ., , as well as the demonstration project charges

</InitialsAgreement>
<InitialsAgreement onChange={(v) => this.setState({initialSanctions: v})}
value={initialSanctions}>
I have read and understand the <i>All of Us</i> Research Program Sanctions for Privacy
Copy link
Contributor

Choose a reason for hiding this comment

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

The third initials is:

Access granted by the program in accordance with this agreement is exclusively for participation in _All of Us_ demonstration projects. At the conclusion of my participation in these activities, I understand that my access to the _All of Us_ data resources will be revoked and/or subject to customary access policies and procedures.

</div>
<div>
{showSanctionModal &&
<SanctionModal onClose={() => this.setState({showSanctionModal: false})}/>}
Copy link
Contributor

Choose a reason for hiding this comment

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

The sanctions information seems different. I am seeing:

, including but not limited to:
posting my name and affiliation on a publicly-accessible list of violators, and
notifying the National Institutes of Health or other federal agencies of my actions. 
I understand that failure to comply with these terms may also carry additional financial, legal, or other repercussions. 

Controlled tier) <i>All of Us</i> data.
</IndentedListItem>
<IndentedListItem>
Post the name and affiliation of the violator on a public <i>All of Us</i>
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a space issue here (same as before, with the placement of the newline affecting whitespace trimming)

@jaycarlton jaycarlton changed the title [RW-3694][risk=no] [RW-3694][risk=no] Updated DUA content Oct 25, 2019
@jaycarlton jaycarlton marked this pull request as ready for review October 25, 2019 20:16
@jaycarlton jaycarlton requested a review from andy7i October 25, 2019 20:18
Copy link
Contributor

@andy7i andy7i left a comment

Choose a reason for hiding this comment

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

a few text differences found so far

</IndentedOrderedList>
</p>
<p>Please read this agreement carefully and completely before signing.</p>
<SecondHeader>As “Authorized Demonstration User” of the <AoUTitle/> data, I
Copy link
Contributor

Choose a reason for hiding this comment

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

should be As an

<SecondHeader>As “Authorized Demonstration User” of the <AoUTitle/> data, I
will:</SecondHeader>
<IndentedUnorderedList>
<li>read and adhere to the <AoUTitle/> <a target='_blank' href={CORE_VALUES_URL}>core values</a>.
Copy link
Contributor

Choose a reason for hiding this comment

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

<IndentedUnorderedList>
If I become aware of any information that directly identifies one or more
participants, I will notify the <AoUTitle/> immediately
using the automatic notification system.
Copy link
Contributor

Choose a reason for hiding this comment

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

appropriate process.

also, make this a list item

I will <strong>NOT</strong> attempt to re-identify research
participants or their relatives.
<br/>
If I unintentionally re-identify participants through the process of my work, I will
Copy link
Contributor

Choose a reason for hiding this comment

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

also, make this a list item

Copy link
Contributor

Choose a reason for hiding this comment

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

and indent

for any external data, files, or software that I upload into my Workspace.</IndentedListItem>
<IndentedListItem>I will <strong>NOT</strong> upload data or files containing personally identifiable
information (PII).</IndentedListItem>
<IndentedListItem>PII means information that can be used to distinguish or trace the identity of an individual
Copy link
Contributor

Choose a reason for hiding this comment

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

indent this

@s-rubenstein
Copy link
Contributor

@andy7i should be good for another look.

Copy link
Contributor

@andy7i andy7i left a comment

Choose a reason for hiding this comment

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

Some content questions, a couple nitpicks, and a couple general things:

Code formatting:

  • unused imports
  • several lines are longer than the allowed length

File organization:

  • DUA files are in profile folder. Recommend moving to a new folder like app/pages/data-use-agreement

</li>
<li>share the results of my demonstration project and all contents of my Workbench with the <AoUTitle/>. </li>
<li>My workbench and its contents may be made public for the benefit of all authorized users.</li>
<li>honor the contribution of those who take part in <i>All of Us</i> to my work
Copy link
Contributor

Choose a reason for hiding this comment

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

honor the contribution to my work of those who take part in All of Us 

{showSanctionModal &&
<SanctionModal onClose={() => this.setState({showSanctionModal: false})}/>}
</div>
<DuaTextInput style={{marginTop: '0.5rem'}}
Copy link
Contributor

Choose a reason for hiding this comment

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

The doc has this line:

Authorized Demonstration User Signature

which we don't have. I'm not sure how we would add that in realistically. Might be worth letting them know.

onClick={() => this.submitDataUseAgreement(this.state.initialWork)}>Submit</Button>
</TooltipTrigger>
</FlexColumn>;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

the Terms and Definitions: section (at the bottom of the Alpha doc) is missing on our side.

participation in these activities, I understand that my access to the <i>All of Us</i> data
resources will be revoked and/or subject to customary access policies and procedures.
</InitialsAgreement>
<div><strong>I acknowledge that failure to comply with the terms of this agreement may result
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: a bit of margin on top would be nice

import {styles as headerStyles} from '../../components/headers';
import {TextInput} from '../../components/inputs';
import colors from '../../styles/colors';
import {reactStyles} from '../../utils';
Copy link
Contributor

Choose a reason for hiding this comment

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

unused imports and linter warnings to use absolute paths

height: '100%',
color: colors.primary,
},
h2: {...headerStyles.h2, lineHeight: '24px', fontWeight: 600, fontSize: '16px'},
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: using rem > px

<Button
style={{marginTop: '1rem', cursor: errors && 'not-allowed', padding: '0 1.3rem'}}
disabled={errors || submitting} data-test-id='submit-dua-button'
onClick={() => this.submitDataUseAgreement(this.state.initialWork)}>Submit</Button>
Copy link
Contributor

Choose a reason for hiding this comment

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

submitDataUseAgreement now takes submitDataUseAgreement(dataUseAgreementVersion, initials)

might need to merge in master to see this.

Copy link
Contributor

Choose a reason for hiding this comment

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

this looks ok actually

@@ -0,0 +1,216 @@
import {BolderHeader, BoldHeader, styles as headerStyles} from 'app/components/headers';
Copy link
Contributor

Choose a reason for hiding this comment

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

styles is unused



export const SanctionModal = (props) => {
return <Modal width={750}>
Copy link
Contributor

Choose a reason for hiding this comment

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

haven't tested this since I couldn't see it open locally.

Copy link
Contributor

Choose a reason for hiding this comment

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

that said, the width=750 bothers me

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is used, but don't want to pull it out as a release blocking thing.

</div>
<div>
{showSanctionModal &&
<SanctionModal onClose={() => this.setState({showSanctionModal: false})}/>}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see Sanctions shown in the alpha DUA doc. Remove?

Copy link
Contributor

@ericsong ericsong left a comment

Choose a reason for hiding this comment

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

Not sure if some of these comments need to be made. I indicated as such for the ones I'm unsure about and we should check with Karthik on those.

<IndentedListItem>
If I become aware of any information that directly identifies one or more
participants, I will notify the <AoUTitle/> immediately
using the automatic notification system.
Copy link
Contributor

Choose a reason for hiding this comment

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

"automatic notification system" should be "appropriate process"

<li>
<strong>NOT</strong> use <AoUTitle/> data or any external data, files, or software that I upload
into the Research Workbench for research that is discriminatory or stigmatizing of individuals, families,
groups, or communities. Please review the All of Us policy on stigmatizing research here.
Copy link
Contributor

Choose a reason for hiding this comment

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

<DuaTextInput placeholder='FIRST AND LAST NAME' style={{margin: '0 1ex'}}
onChange={(v) => this.setState({name: v})} value={name}
data-test-id='dua-name-input'/>
("Authorized User") have
Copy link
Contributor

Choose a reason for hiding this comment

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

bump

</div>
<div>By entering my initials next to each statement below, I agree to these terms:</div>
<InitialsAgreement onChange={(v) => this.setState({initialWork: v})} value={initialWork}>
My work may be logged, monitored, and audited by the <i>All of Us</i> Research Program to ensure
Copy link
Contributor

Choose a reason for hiding this comment

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

"My work may be logged" -> "My work will be logged"

disabled value={this.props.profileState.profile.username}/>
<DuaTextInput style={{marginTop: '0.5rem'}}
disabled value={this.props.profileState.profile.contactEmail}/>
<DuaTextInput style={{marginTop: '0.5rem'}}
Copy link
Contributor

Choose a reason for hiding this comment

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

The google doc has labels on these input fields. not sure if they're needed though?

disabled={errors || submitting} data-test-id='submit-dua-button'
onClick={() => this.submitDataUseAgreement(this.state.initialWork)}>Submit</Button>
</TooltipTrigger>
</FlexColumn>;
Copy link
Contributor

Choose a reason for hiding this comment

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

The google doc also has a page for "Terms and Definitions". Do we need to add that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Talked to Karthik, we do need that. Will add.

aggregate statistics that are more granular than buckets of 20 individuals without explicit approval from
the <AoUTitle/>.
</li>
<li><strong>NOT</strong> sell or distribute <AoUTitle/> data at any level of granularity for the purpose
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if needed but all of the "NOT"'s under the As “Authorized Demonstration User” of the All of Us Research Program data, I will: section has an underline

Copy link
Contributor

Choose a reason for hiding this comment

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

They don't. Jay checked with Dikshya on that

Copy link
Contributor

@ericsong ericsong left a comment

Choose a reason for hiding this comment

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

Content looks good.

Copy link
Contributor

@andy7i andy7i left a comment

Choose a reason for hiding this comment

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

1 fix then 👍

Notes:
Eric checked the content so I skimmed through the Terms and Conditions.
DUA files to be moved outside of this PR.


<SecondHeader>Terms and Definitions:</SecondHeader>
<IndentedUnorderedList>
<IndentedListItem>The <a href={{AOU_DEFINITION_URL}} target='_blank'><AoUTitle/></a> is a national longitudinal
Copy link
Contributor

Choose a reason for hiding this comment

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

this links to http://localhost:4200/[object%20Object] for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Already fixed! :D

@calbach
Copy link
Contributor

calbach commented Oct 28, 2019

Merging per slack discussion with Scout, tests finally caught up.

@calbach calbach merged commit e166fa1 into master Oct 28, 2019
@calbach calbach deleted the jaycarlton/RW-3694 branch October 28, 2019 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants