feat: add Pushpad provider for web push notifications #4235

collimarco · 2023-09-29T16:14:08Z

What change does this PR introduce?

Add Pushpad provider for web push notifications.

Why was this change needed?

Closes #4218

collimarco · 2023-10-12T11:57:22Z

@parveshsaini @BiswaViraj @scopsy

Can you please consider merging or let me know something?

2 weeks have passed and I see that other providers (posted later) are already merged.

Since it costed me a lot of work, I would appreciate if you can look at it.

Thanks

BiswaViraj · 2023-10-12T14:33:20Z

@collimarco Sorry for the delay 🙏🏼 ,
I will review this in few hours.
For now can you please, fix the merge conflicts

collimarco · 2023-10-12T16:06:22Z

@BiswaViraj

Thanks for the reply!

It seems that the only conflicts are the package files (some generated automatically)... Maybe it's better to fix them when you are merging this PR (to avoid further conflicts)?

Otherwise please let me know what exact steps I should perform

BiswaViraj · 2023-10-12T16:11:49Z

@collimarco, just saw the spell check CI test is also failing, please fix it by adding the words to the https://github.com/novuhq/novu/blob/next/.cspell.json file

BiswaViraj · 2023-10-12T16:17:22Z

packages/application-generic/package.json

@@ -72,6 +72,7 @@
    "@novu/netcore": "^0.19.0",
    "@novu/nodemailer": "^0.19.0",
    "@novu/one-signal": "^0.19.0",
+    "@novu/pushpad": "^0.16.3",


Suggested change

"@novu/pushpad": "^0.16.3",

"@novu/pushpad": "^0.20.0-alpha.1",

After fixing the merge conflicts, update this version to ^0.20.0-alpha.1

@collimarco , I think this change was removed after the rebase, can you update this and then run npm run setup:project to generate the new pnpm-lock file

@BiswaViraj Ok, done! Do you need anything else for merging?

BiswaViraj · 2023-10-12T16:20:03Z

providers/pushpad/README.md

+## Usage
+
+```javascript
+    FILL IN THE INITIALIZATION USAGE


Please add an example usage here

BiswaViraj · 2023-10-12T16:20:59Z

providers/pushpad/package.json

@@ -0,0 +1,82 @@
+{
+  "name": "@novu/pushpad",
+  "version": "0.16.3",


Suggested change

"version": "0.16.3",

"version": "0.20.0-alpha.1",

BiswaViraj · 2023-10-12T16:23:05Z

providers/pushpad/package.json

+    "pnpm": "^7.26.0"
+  },
+  "dependencies": {
+    "@novu/stateless": "0.16.3",


Suggested change

"@novu/stateless": "0.16.3",

"@novu/stateless": "^0.20.0-alpha.1",

BiswaViraj · 2023-10-12T16:26:28Z

providers/pushpad/src/lib/pushpad.provider.spec.ts

+import { PushpadPushProvider } from './pushpad.provider';
+
+test('should trigger pushpad library correctly', async () => {
+  const provider = new PushpadPushProvider({
+    apiKey: 'test-token',
+    appId: '012345',
+  });
+});


Please add a test case where the response is mocked,
For reference, you can check other providers spec files

BiswaViraj · 2023-10-12T16:29:30Z

apps/web/public/static/images/providers/dark/pushpad.svg

Add dark Square version of the logo here.

BiswaViraj · 2023-10-12T16:30:30Z

apps/web/public/static/images/providers/light/pushpad.svg

add light square version of logo here.

collimarco · 2023-10-12T17:37:59Z

@BiswaViraj

Thanks for the suggestions! I made the requested changes.

For the test I see that other providers do this for example:

  const spy = jest
    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
    // @ts-expect-error
    .spyOn(provider.oneSignal, 'createNotification')

However the Pushpad library uses new Pushpad.Notification({}) and not something like pushpadInstance.createNotification(). This means that it is hard to mock / stub in this case. I also admit that I am mainly a Ruby developer and I don't have much experience with tests in TypeScript. Do you have any suggestion on how to test that?

BiswaViraj · 2023-10-12T19:21:00Z

@collimarco , umm, I see what you mean, it will be difficult, in that case, you can try to mock the provider's sendMessage

  const spy = jest
    .spyOn(provider, 'sendMessage')
    .mockImplementation(async () => {
      // eslint-disable-next-line @typescript-eslint/no-explicit-any
      return {} as any;
    });

collimarco · 2023-10-12T19:50:42Z

@BiswaViraj Yes it's hard to test.

Do you mean something like this?

import { PushpadPushProvider } from './pushpad.provider';

test('should trigger pushpad library correctly', async () => {
  const provider = new PushpadPushProvider({
    apiKey: 'test-token',
    appId: '012345',
  });

  const spy = jest
    .spyOn(provider, 'sendMessage')
    .mockImplementation(async () => {
      // eslint-disable-next-line @typescript-eslint/no-explicit-any
      return {} as any;
    });
});

That would mock the entire method and nothing is tested... that is why I omitted the test.

Otherwise the only solution would be to extract this code to a separate method:

    const notification = new Pushpad.Notification({
      project: this.pushpad,
      body: options.content,
      title: options.title,
    });

And then stub the method that instantiates the new notification.

However, again, all the testing would be against a mock notification, and that is not really useful.

LetItRock · 2023-10-13T08:39:31Z

hey @collimarco! 👋 thanks a lot for your contribution to Novu, we really appreciate that 🤝

A few suggestions from my side about the mocking. Basically in your tests, you need to cover the logic that is in the PushpadPushProvider.sendMessage and verify that when the options.target exists then the notification.deliverTo is called, otherwise notification.broadcast.

In order to do that you have to mock the pushpad module and provide the mock implementation for the Pushpad.Notification.

Something like this should work:

jest.mock('pushpad', () => {
  class MockNotification {
    async deliverTo(target, callback) {
      callback(null, { id: 12345 });
    }

    async broadcast(callback) {
      callback(null, { id: 54321 });
    }
  }

  return {
    ...jest.requireActual('pushpad'),
    Notification: MockNotification,
  };
});

Then in your tests, you can create the instance of the PushpadPushProvider and call the sendMessage function and then verify if the id returned is the one that is from the MockNotification.deliverTo or MockNotification.broadcast.

Hope this helps ;)
P.S. Please also update your branch with the latest changes.

collimarco · 2023-10-13T09:12:36Z

@LetItRock Thanks for the suggestion, I appreciate it!

I was already working on this and found a similar solution: can you check if it's ok? (test is passing)

66ce7d4

Otherwise if you prefer your code I can change it.

P.S. Please also update your branch with the latest changes.

I only see the package files conflicting: can you please fix that when you merge the branch?

Otherwise, how do you recommend to proceed? Do you want me to make a git merge origin/master on my feature branch?

LetItRock · 2023-10-13T09:58:27Z

I was already working on this and found a similar solution: can you check if it's ok? (test is passing)

yes, this approach also works 🙌 but please just add one more test that will cover the case when the broadcast function is called ;)

P.S. Please also update your branch with the latest changes.

I only see the package files conflicting: can you please fix that when you merge the branch?

Otherwise, how do you recommend to proceed? Do you want me to make a git merge origin/master on my feature branch?

you can follow the suggested steps here: https://stackoverflow.com/a/7244456/2383407, the branch that you have to merge/rebase is next

collimarco · 2023-10-13T10:23:23Z

@LetItRock Yes, no problem.

But I have a doubt about the Novu architecture that is blocking me: is options.target always set to some user IDs (never empty)? Or it can be null to send a broadcast message to everyone? Or you use an empty array [] when you want to send to everyone?

LetItRock · 2023-10-13T10:55:15Z

@LetItRock Yes, no problem.

But I have a doubt about the Novu architecture that is blocking me: is options.target always set to some user IDs (never empty)? Or it can be null to send a broadcast message to everyone? Or you use an empty array [] when you want to send to everyone?

If you check the target types, it says that it's string[] meaning you shouldn't expect it to be null.
The target is something that we call "device tokens" that are stored on the subscriber of the notification. The Novu logic that is calling the sendMessage will make sure that it will be never called with [] nor null.

…d by novu

collimarco · 2023-10-13T11:48:44Z

@LetItRock @BiswaViraj

I have:

removed Pushpad broadcast notifications since Novu doesn't support it
completed the tests
rebased my feature on upstream/next

Please merge it or let me know something.

Thanks

collimarco · 2023-10-13T13:03:45Z

If you want to try the Pushpad provider directly with Novu you can create a free account:

Go to Pushpad and create an account (only name and email are required for sign up, no credit card)
Create a new sender (e.g. Name: "My sender")
Create a new project (e.g. Sender: "My sender", Name: "Example project", Website: "example.com")
Go to project settings to get the project ID (called AppID in Novu)
Go to your account settings -> Access tokens and create a new token (called ApiKey in Novu)
You should be able to send a notification to a recipient from Novu (e.g. target = "myuser567")
If you go to your Pushpad project and click Notifications -> Details you should see that the notification is sent to the user with id = "myuser567"

LetItRock · 2023-10-13T21:46:52Z

If you want to try the Pushpad provider directly with Novu you can create a free account:

Go to Pushpad and create an account (only name and email are required for sign up, no credit card)

Create a new sender (e.g. Name: "My sender")

Create a new project (e.g. Sender: "My sender", Name: "Example project", Website: "example.com")

Go to project settings to get the project ID (called AppID in Novu)

Go to your account settings -> Access tokens and create a new token (called ApiKey in Novu)

You should be able to send a notification to a recipient from Novu (e.g. target = "myuser567")

If you go to your Pushpad project and click Notifications -> Details you should see that the notification is sent to the user with id = "myuser567"

hey 👋 I was able to successfully integrate Novu with a Pushpad.
But unfortunately in the activity feed, I do see that the ID is returned as null, check the screenshot below. It's happening because the code in the PushpadPushProvider.sendMessage is not "awaiting" for the notification.deliverTo callback result. In order to return the ID appropriately, you need to wrap notification.deliverTo code in the promise and "await" for the result. Also please include in the test the check for the id field.

collimarco · 2023-10-14T06:11:09Z

@LetItRock

Thanks!

I have fixed the error with this commit:
e556773

Let me know

LetItRock · 2023-10-17T15:40:08Z

@LetItRock

Thanks!

I have fixed the error with this commit: e556773

Let me know

Looks good, thanks! 🙌 let's see if the pipelines will pass :)

LetItRock · 2023-10-17T16:05:54Z

@collimarco can you please pull the latest and resolve the conflicts? thanks! 🤝

collimarco · 2023-10-17T16:42:44Z

@LetItRock Thanks for approving this!

I already rebased everything the other day, spending a lot of time, then the branch next moved forward, and now I would need to rebase again... It's really a waste of time.

Can you please rebase it when you actually merge it? Otherwise if I do the work again and you don't merge immediately it's useless...

LetItRock · 2023-10-18T13:39:12Z

@LetItRock Thanks for approving this!

I already rebased everything the other day, spending a lot of time, then the branch next moved forward, and now I would need to rebase again... It's really a waste of time.

Can you please rebase it when you actually merge it? Otherwise if I do the work again and you don't merge immediately it's useless...

ok, we will handle it, thanks! ;)

LetItRock · 2023-10-23T22:13:58Z

hey @collimarco! 👋 can you please give me permission to push to your fork?

collimarco · 2023-10-24T11:10:28Z

@LetItRock I have added you to the forked repository with write access. Let me know if you need anything else.

collimarco · 2023-10-26T09:10:03Z

@LetItRock Thanks for merging this!

Do you know when this will become available in the next release and in your cloud version?

When it becomes publicly available and released, I am planning to publish a blog post or documentation on our website.

LetItRock · 2023-10-27T08:52:00Z

@LetItRock Thanks for merging this!

Do you know when this will become available in the next release and in your cloud version?

hey @collimarco 👋 thank you too for the contribution 🤝
We've just deployed the cloud version v0.21.0; Pushpad integration is included.

When it becomes publicly available and released, I am planning to publish a blog post or documentation on our website.

that's cool, let us know about that ;)

collimarco · 2023-10-27T08:57:28Z

@LetItRock Sure, I'll let you know :)

collimarco · 2023-11-12T11:56:51Z

@LetItRock

let us know about that ;)

Here's the blog post 🎉 https://pushpad.xyz/blog/sending-web-push-notifications-from-novu-notification-center

github-actions bot added @novu/web @novu/shared providers community labels Sep 29, 2023

collimarco mentioned this pull request Sep 29, 2023

🚀 Feature: Pushpad provider for web push notifications #4218

Closed

2 tasks

BiswaViraj self-assigned this Oct 12, 2023

BiswaViraj requested changes Oct 12, 2023

View reviewed changes

collimarco requested a review from BiswaViraj October 13, 2023 09:55

collimarco added 8 commits October 13, 2023 13:33

feat: add pushpad provider novuhq#4218

59de9c1

fix: add Pushpad to spell check

7410fdd

fix: add example usage for Pushpad provider

82ec24d

fix: add square logo for Pushpad provider

4a74b79

fix: update novu and package version for Pushpad provider

8cb9820

fix: update version for pushpad provider in package.json

f9d5892

fix: refactor code and test it for pushpad provider

192beb3

fix: remove code for pushpad broadcast notifications that are not use…

5b53c0c

…d by novu

collimarco force-pushed the next branch from 4b34daf to 5b53c0c Compare October 13, 2023 11:41

fix: add pushpad to package.json and regenerate pnpm-lock file

fca0bcc

fix: await for the notification.deliverto callback result in pushpad

e556773

LetItRock approved these changes Oct 17, 2023

View reviewed changes

Merge branch 'next' into pushpad/next

0b29a52

LetItRock and others added 6 commits October 24, 2023 17:24

chore(web): update the push providers order

9eb618c

Merge branch 'next' into pushpad/next

a517a39

Merge branch 'next' into pushpad/next

067d996

chore: fixed lock file

12377b8

Merge branch 'next' into pushpad/next

40482b4

Merge branch 'next' into next

7b0ba16

LetItRock merged commit 7405994 into novuhq:next Oct 25, 2023
24 of 27 checks passed

collimarco mentioned this pull request Oct 31, 2023

Add Pushpad provider to README.md #4720

Merged

collimarco mentioned this pull request Nov 7, 2023

Add Pushpad provider documentation novuhq/docs#344

Merged

	"@novu/pushpad": "^0.16.3",
	"@novu/pushpad": "^0.20.0-alpha.1",

	"@novu/stateless": "0.16.3",
	"@novu/stateless": "^0.20.0-alpha.1",

feat: add Pushpad provider for web push notifications #4235

feat: add Pushpad provider for web push notifications #4235

Conversation

collimarco commented Sep 29, 2023

What change does this PR introduce?

Why was this change needed?

collimarco commented Oct 12, 2023

BiswaViraj commented Oct 12, 2023

collimarco commented Oct 12, 2023

BiswaViraj commented Oct 12, 2023

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

collimarco commented Oct 12, 2023

BiswaViraj commented Oct 12, 2023

collimarco commented Oct 12, 2023

LetItRock commented Oct 13, 2023

collimarco commented Oct 13, 2023 • edited Loading

LetItRock commented Oct 13, 2023

collimarco commented Oct 13, 2023

LetItRock commented Oct 13, 2023

collimarco commented Oct 13, 2023

collimarco commented Oct 13, 2023

LetItRock commented Oct 13, 2023 • edited Loading

collimarco commented Oct 14, 2023

LetItRock commented Oct 17, 2023

LetItRock commented Oct 17, 2023

collimarco commented Oct 17, 2023

LetItRock commented Oct 18, 2023

LetItRock commented Oct 23, 2023

collimarco commented Oct 24, 2023

collimarco commented Oct 26, 2023

LetItRock commented Oct 27, 2023

collimarco commented Oct 27, 2023 • edited Loading

collimarco commented Nov 12, 2023

collimarco commented Oct 13, 2023 •

edited

Loading

LetItRock commented Oct 13, 2023 •

edited

Loading

collimarco commented Oct 27, 2023 •

edited

Loading