You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/08/03 16:07:27 UTC

[GitHub] [incubator-superset] JasonD28 opened a new pull request #10506: fix: refactored SQL-based alerts to not pass sqlalchemy objects as args

JasonD28 opened a new pull request #10506:
URL: https://github.com/apache/incubator-superset/pull/10506


   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   Updated the code to remove the passing of sqlalchemy objects as arguments to functions and instead replace them with plain type arguments.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   ### TEST PLAN
   <!--- What steps should be taken to verify the changes -->
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] bkyryliuk merged pull request #10506: fix: refactored SQL-based alerts to not pass sqlalchemy objects as args

Posted by GitBox <gi...@apache.org>.
bkyryliuk merged pull request #10506:
URL: https://github.com/apache/incubator-superset/pull/10506


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10506: fix: refactored SQL-based alerts to not pass sqlalchemy objects as args

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10506:
URL: https://github.com/apache/incubator-superset/pull/10506#discussion_r464523711



##########
File path: superset/tasks/schedules.py
##########
@@ -551,10 +552,17 @@ def schedule_alert_query(  # pylint: disable=unused-argument
 
     if report_type == ScheduleType.alert:
         if is_test_alert and recipients:
-            deliver_alert(schedule, recipients)
+            deliver_alert(schedule.id, schedule.slice.id, schedule.label, recipients)
             return
 
-        if run_alert_query(schedule):
+        if run_alert_query(
+            schedule.id,

Review comment:
       Instead of passing in every param needed from schedule, maybe it's cleaner to only pass in the schedule id, then you can get the schedule object within the function and reference all the params there.
   
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] bkyryliuk commented on a change in pull request #10506: fix: refactored SQL-based alerts to not pass sqlalchemy objects as args

Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #10506:
URL: https://github.com/apache/incubator-superset/pull/10506#discussion_r464741076



##########
File path: superset/tasks/schedules.py
##########
@@ -612,10 +614,12 @@ def deliver_alert(alert: Alert, recipients: Optional[str] = None) -> None:
     _deliver_email(recipients, deliver_as_group, subject, body, data, images)
 
 
-def run_alert_query(alert: Alert) -> Optional[bool]:
+def run_alert_query(alert_id: int) -> Optional[bool]:

Review comment:
       @JasonD28 let's start adding tests




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org