Skip to content

feat: toggle run all notifications - #1162

Merged
czgu merged 3 commits into
pinterest:masterfrom
lilyli9:external/toggle-run-all-notifications
Feb 14, 2023
Merged

czgu merged 3 commits into
pinterest:masterfrom
lilyli9:external/toggle-run-all-notifications

Conversation

@lilyli9

@lilyli9 lilyli9 commented Feb 14, 2023

Copy link
Copy Markdown
Contributor

Added a toggle to the run all confirmation page so that users can choose whether they want a notification to be sent upon run completion. By default, the notification and toggle will be enabled to match previous behavior of Querybook where a notification will always be sent to the user upon run completion. The type of notification sent is still based on "notification preference" in user settings.

Screen Shot 2023-02-13 at 4 00 06 PM

Comment thread querybook/server/datasources/datadoc.py Outdated

@register("/datadoc/<int:id>/run/", methods=["POST"])
def adhoc_run_data_doc(id):
def adhoc_run_data_doc(id, sendNotification=True):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

use camel casing send_notifications
also please default it as False


interface IProps {
defaultNotification: boolean;
handleNotificationChange: (notification: boolean) => void;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

onNotificationChange

const [notification, setNotification] = useState(defaultNotification);

const internalNotificationChange = useCallback(
(value) => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: (value: boolean) =>

onChange={internalNotificationChange}
/>

<span data-balloon-pos={'up'} className="ml4">

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

data-balloon-pos={'up'}
but there is no tooltip to show?

@lilyli9
lilyli9 requested a review from czgu February 14, 2023 16:55

@czgu czgu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

amazing!

@czgu
czgu merged commit 53a70e1 into pinterest:master Feb 14, 2023
rohan-sh1 pushed a commit to CAI-TECHNOLOGIES/cai-ext-db-explorer that referenced this pull request Apr 11, 2023
* feat: toggle run all notifications

* chore: rename var change default send notification

* fix: rename all handleNotificationChange
aidenprice pushed a commit to arrowtail-precision/querybook that referenced this pull request Jan 3, 2024
* feat: toggle run all notifications

* chore: rename var change default send notification

* fix: rename all handleNotificationChange
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.

2 participants