-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: peer review backend endpoints #20
Conversation
) | ||
data_cell = datadoc_logic.get_data_cell_by_id(data_cell_id, session=session) | ||
data_doc = data_cell.doc | ||
data_doc = process_query_execution( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name process_query_execution
sounds like it is executing the query, maybe process_query_execution_metadata
?
trackClick({ | ||
component: ComponentType.DATADOC_QUERY_CELL, | ||
element: ElementType.PEER_REVIEW_QUERY_BUTTON, | ||
aux: { | ||
lintError: this.state.hasLintError, | ||
sampleRate: this.sampleRate, | ||
}, | ||
}); | ||
|
||
const transformedQuery = await this.getTransformedQuery(); | ||
if (!transformedQuery) { | ||
return null; | ||
} | ||
|
||
const executionMetadata = | ||
this.sampleRate > 0 ? { sample_rate: this.sampleRate } : null; | ||
|
||
const peerReviewParams = { | ||
reviewer_ids: reviewerIds, | ||
external_recipients: externalRecipients, | ||
notifier_name: notifierName, | ||
review_request_reason: justification, | ||
}; | ||
|
||
const delayExecution = true; | ||
|
||
await this.props.createQueryExecution( | ||
transformedQuery, | ||
this.engineId, | ||
this.props.cellId, | ||
executionMetadata, | ||
peerReviewParams, | ||
delayExecution | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is quite similar to onRunButtonClick
, can we refactor a bit to reuse and serve both?
5f4b7e5
to
4e0f81d
Compare
524d8d5
to
7ca938b
Compare
4e0f81d
to
e365c81
Compare
7ca938b
to
1849b09
Compare
session=session, | ||
) | ||
# TODO: Implement notification logic for reviewers | ||
print(external_recipients) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove prints in next PR and add notification logic
Changes: refactored into onRunButtonClick() and onPeerReviewSubmit() to use helper executeQuery() |
1849b09
to
0ccbfc3
Compare
@@ -446,40 +446,93 @@ class DataDocQueryCellComponent extends React.PureComponent<IProps, IState> { | |||
} | |||
|
|||
@bind | |||
public async onRunButtonClick() { | |||
public async executeQuery(options: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we going to support the review request in the adhoc page as well? or we can have some pointer to let user to use datadocs if they see those errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added for adhoc page as well
peerReviewParams, | ||
delayExecution: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't delayExecution
kind of unnecessary? it will be always true if peerReviewParams is not null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, just added delayExecution flag in case we have future feature that require delay execution, removed for now
e365c81
to
62e53a1
Compare
0ccbfc3
to
b515d87
Compare
62e53a1
to
79a01da
Compare
b515d87
to
6fded45
Compare
6fded45
to
9d933b0
Compare
Changes
create_query_execution()
to handle peer review workflow and delayed executions