Metabase channel #43924

qnkhuat · 2024-06-10T10:37:49Z

Introducing a new module: Channel™.

See the tech doc: https://www.notion.so/metabase/Notification-high-level-design-9620a4a69db0494f806a146cc9174964?pvs=4 for a detailed design.

What this PR does:

Create a new channel module with 2 multimethod: render-notification and send!
Migrate slack and email to this module

This PR shouldn't generate any behavior changes, though you'll see a bunch of tests got updated because many of our tests were written when we still have: pulse -- which is a notification with one or more cards. So I had to update those tests, some are reworked to alert test, some are removed.

Walk through of this PR and repl demo: loom

Results

Dashboard subscriptions

Email

Before

After

Slack

Before

After

Alert

Email

Before

After

Slack

Before

After

Note on testing update:
you'll see a lot of changes are about adding alert_condition = rows to the Pulse, this is because previously we have the Pulse feature -- which is a notification with one or more cards, it's different from dashboard subscriptions in that it doesn't associate with a dashboard. or you can think of it like : Alert is a special case of Pulse where it has only one card.

Since this feature is deprecated, I've converted most of the pulse tests to alert tests to change from a pulse to an alert; it's simply by adding an alert_condition.

Milestone 1 of: #43822

replay-io · 2024-06-10T10:49:54Z

Status	Complete ↗︎
Commit	`a2e03a9`
Results	⚠️ 2 Flaky ✅ 2648 Passed

qnkhuat · 2024-06-12T05:04:20Z

balanced?

qnkhuat · 2024-06-12T05:15:00Z

src/metabase/pulse/util.clj

@@ -42,7 +42,8 @@
                                (process-query))
                              (process-query))]
          {:card   card
-           :result result}))
+           :result result
+           :type   :card}))


adding a :type here so execute-card this is considered a part, which is an internal data type that both alert and dashboard subscription usees for rendering.

qnkhuat · 2024-06-12T05:15:50Z

test/metabase/api/pulse_test.clj

-                                    :bcc?    true})
-               (mt/regex-email-bodies #"Daily Sad Toucans")))))))
-
-(deftest send-placeholder-card-test-pulse-test


this is an old use case with pulse, it's no longer needed because this test endpoint only works on dashboard, and on dashboard all cards are saved.

qnkhuat · 2024-06-12T05:16:10Z

test/metabase/api/pulse_test.clj

-;; This test follows a flow that the user/UI would follow by first creating a pulse, then making a small change to
-;; that pulse and testing it. The primary purpose of this test is to ensure tha the pulse/test endpoint accepts data
-;; of the same format that the pulse GET returns
-(deftest update-flow-test


same with above, it's an use case with the old pulse.

qnkhuat · 2024-06-12T05:39:51Z

test/metabase/pulse_test.clj

@@ -861,40 +752,6 @@
               processed))
        (is (= @titles ["a.png"]))))))

-(deftest multi-channel-test


we have many tests for alert send to email and slack, this is not needed.

qnkhuat · 2024-06-12T10:14:27Z

enterprise/backend/test/metabase_enterprise/sandbox/pulse_test.clj


-(deftest bcc-enabled-pulse-test


moved to metabase.channel.email_test

qnkhuat · 2024-06-12T10:15:10Z

src/metabase/channel/email.clj

99% of the code here and slack are moved from metabase.pulse

qnkhuat · 2024-06-12T10:16:01Z

src/metabase/email/messages.clj

    {:title                     card-name
     :titleUrl                  (urls/card-url card-id)
     :alertSchedule             (alert-schedule-text channel)
+     :notificationText          (if (nil? non-user-email)


this was a bug before, alert email for non-user doesn't have the manage subscriptions link.

qnkhuat · 2024-06-12T10:18:58Z

src/metabase/models/pulse.clj

@@ -293,7 +293,7 @@
  "Fetch a single *Pulse*, and hydrate it with a set of 'standard' hydrations; remove Alert columns, since this is a
  *Pulse* and they will all be unset."
  [pulse-or-id]
-  (some-> (t2/select-one Pulse :id (u/the-id pulse-or-id), :alert_condition nil)
+  (some-> (t2/select-one Pulse :id (u/the-id pulse-or-id))


in principle we shouldn't use this function anymore because there is no longer pulse, we should use retrieve-alert instead. But the problem is that alert has :card instead of :cards like pulse does, so lots of rendering code will break if we change to retrieve-alert.

I plan to rework this on the 2nd milestone, where we remove pulse alltogether

qnkhuat · 2024-06-12T10:54:16Z

src/metabase/channel/email.clj

@@ -0,0 +1,102 @@
+(ns metabase.channel.email


99% of the code here and slack are moved from metabase.pulse

calherries · 2024-06-13T04:00:14Z

This is a fairly big PR. So before I have a look, can you confirm what subset of the tech doc this PR completes? And can I read it as-is to understand the high-level design, or have the plans changed somewhat?

qnkhuat · 2024-06-13T04:51:14Z

@calherries This should partially address the Channel section in that tech doc. Specifically, this PR introduces 2 multimethods, render-notification and send!. Most of the changes are splitting the render + send logic from metabase.pulse and moving it to metabase.channel.*, so you should expect no behavior change.

qnkhuat · 2024-06-13T04:59:08Z

test/metabase/pulse_test.clj

+                       ((if (= :email channel-type)
+                          :channel/email
+                          :channel/slack)
+                        (pulse.test-util/with-captured-channel-send-messages!


if you look at how tests were written before you'll see 2 different approaches with email and slack.

For slack, the assertions are made against the result of (send-pulse!), this is done by having a bunch of redefs to make (send-pulse!) return the rendered-notification for slack only.

But for email, the test ignore the result of (send-pulse!), instead it checks results from the global (mt/inbox) atom.

I also think the recent flakes with pulse is because we were using this global inbox atom, it could be that parallel tests mutate the inbox while a test is running.

So I create a new pulse.test-util/with-captured-channel-send-messages! macro which will capture all calls to (channel/send!), this function should take a message that is already rendered and is ready to send. Which means: for email, it's a html with recipients info, for slack: It contains all the rendered attachments and text.

I also believe this will fix our flakes with pulse recently.

qnkhuat · 2024-06-13T04:59:58Z

test/metabase/pulse_test.clj

+              :email (thunk)
+              :slack (pulse.test-util/slack-test-setup! (thunk)))))))))
+
+(defn- tests!


this test helper uses redefs so it must end with !

qnkhuat · 2024-06-13T05:00:36Z

test/metabase/pulse_test.clj

-     (fn [_ _]
-       (is (= (rasta-pulse-email)
-              (mt/summarize-multipart-email #"Pulse Name"))))
+     (fn [_ [email]]


the result here is a list of emails, since this test only sends one we can take the first.

test/metabase/pulse_test.clj

piranha

IDK, aside from nitpicks - seems like this is exactly what is planned. :)

dev/src/dev/render_png.clj

enterprise/backend/test/metabase_enterprise/advanced_config/api/pulse_test.clj

src/metabase/channel/core.clj

qnkhuat added 3 commits June 10, 2024 13:41

adding the channel

538d488

pulse with new slack channel

3305b8e

pulse with email channels

9b9ce20

qnkhuat requested a review from camsaul as a code owner June 10, 2024 10:37

metabase-bot bot assigned qnkhuat Jun 10, 2024

metabase-bot bot added the .Team/BackendComponents also known as BEC label Jun 10, 2024

qnkhuat added the no-backport Do not backport this PR to any branch label Jun 10, 2024

pulse now use all channels api

d1cd4c2

qnkhuat changed the title ~~Channel~~ Metabase channel Jun 10, 2024

qnkhuat added 11 commits June 11, 2024 10:40

tidy up and support retry

ba0614b

fixing tests + retry sending

de5689e

pulse tests passed

02ab272

fix test failed to compile

17f8b34

test fixing...

36f750f

handle channel correctly

8ed0d00

remove debug code

1c805de

still fixing tests

2c82e37

test fixes :crossed-finger

03de60a

test fixes :crossed-finger 2

579a475

ci should be green now

578171e

qnkhuat commented Jun 12, 2024

View reviewed changes

qnkhuat added 4 commits June 12, 2024 12:40

Tidy

14df2a5

remove pulse test that no longer needed

09598d1

remove pulse test that no longer needed

607b81e

add attachments test for dashboard sub

7138fc6

qnkhuat commented Jun 12, 2024

View reviewed changes

src/metabase/channel/email.clj

Copy link

Contributor Author

qnkhuat Jun 12, 2024

Choose a reason for hiding this comment

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

99% of the code here and slack are moved from metabase.pulse

qnkhuat commented Jun 12, 2024

View reviewed changes

no more doall with for

be29d5d

qnkhuat commented Jun 12, 2024

View reviewed changes

Merge branch 'master' into channel

56998cb

qnkhuat commented Jun 13, 2024

View reviewed changes

test/metabase/pulse_test.clj Outdated Show resolved Hide resolved

Update test/metabase/pulse_test.clj

fcba1b8

darksciencebase mentioned this pull request Jun 13, 2024

[Notifications] Webhook channel for alert #43822

Open

qnkhuat mentioned this pull request Jun 18, 2024

Decouple channel and notification #43113

Closed

Merge branch 'master' into channel

6f67e2e

piranha approved these changes Jun 18, 2024

View reviewed changes

dev/src/dev/render_png.clj Outdated Show resolved Hide resolved

enterprise/backend/test/metabase_enterprise/advanced_config/api/pulse_test.clj Outdated Show resolved Hide resolved

src/metabase/channel/core.clj Outdated Show resolved Hide resolved

darksciencebase mentioned this pull request Jun 18, 2024

[Flaky Test]: two-cards-in-one-pulse-test #44333

Closed

qnkhuat added 3 commits June 19, 2024 11:26

Add an helper and make sure we don't send to archived channels

4f3b26f

final tidy

354b6c1

no inline def for tests

2342e92

qnkhuat enabled auto-merge (squash) June 19, 2024 04:26

qnkhuat added 3 commits June 19, 2024 11:27

helper docstring

caf79fa

test fixes

25b55cf

retrieve the disabled pulses too

a2e03a9

qnkhuat merged commit 613318f into master Jun 19, 2024
110 checks passed

qnkhuat deleted the channel branch June 19, 2024 09:25

This was referenced Jun 21, 2024

[Flaky Test]: above-goal-alert-test #44041

Closed

BE flake: metabase.pulse-test/multiple-recipients-test #43130

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metabase channel #43924

Metabase channel #43924

qnkhuat commented Jun 10, 2024 •

edited

replay-io bot commented Jun 10, 2024 •

edited

qnkhuat commented Jun 12, 2024

qnkhuat Jun 12, 2024

qnkhuat Jun 12, 2024

qnkhuat Jun 12, 2024

qnkhuat Jun 12, 2024

qnkhuat Jun 12, 2024

qnkhuat Jun 12, 2024

qnkhuat Jun 12, 2024

qnkhuat Jun 12, 2024

qnkhuat Jun 12, 2024

calherries commented Jun 13, 2024

qnkhuat commented Jun 13, 2024

qnkhuat Jun 13, 2024

qnkhuat Jun 13, 2024

qnkhuat Jun 13, 2024 •

edited

qnkhuat Jun 13, 2024 •

edited

piranha left a comment


		(deftest bcc-enabled-pulse-test

Metabase channel #43924

Metabase channel #43924

Conversation

qnkhuat commented Jun 10, 2024 • edited

Email

Slack

Email

Slack

replay-io bot commented Jun 10, 2024 • edited

qnkhuat commented Jun 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calherries commented Jun 13, 2024

qnkhuat commented Jun 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qnkhuat Jun 13, 2024 • edited

Choose a reason for hiding this comment

qnkhuat Jun 13, 2024 • edited

Choose a reason for hiding this comment

piranha left a comment

Choose a reason for hiding this comment

qnkhuat commented Jun 10, 2024 •

edited

replay-io bot commented Jun 10, 2024 •

edited

qnkhuat Jun 13, 2024 •

edited

qnkhuat Jun 13, 2024 •

edited