Skip to content
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

fix: Correct payload to unbreak open link when clicking welcome notification #1207

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

chamons
Copy link
Contributor

@chamons chamons commented Nov 18, 2024

https://app.asana.com/0/1208719710991963/1208760940956731/f

This issue boiled down to the "Open link when clicking welcome notification" feature not working, when you clicked the notification is opened the 'default' URL, not the option set.

After doing a bunch of digging, I was able to confirm that we were saving that setting and then servcing it to the SDK via the sync API.

I was then able to trace it through:

  1. src/shared/helpers/EventHelper.ts (onSubscriptionChanged_showWelcomeNotification)
  2. src/shared/helpers/MainHelper.ts (MainHelper.showLocalNotification)

There we shovel that url into a dataPayload that we shovel into a service worker call.

That comes out the other side when you click the notifiction in:

  1. src/sw/serviceWorker/ServiceWorker.ts (onNotificationClicked)

There we cast via:

const osNotification = event.notification.data as IOSNotification;

IOSNotification is defined to have:

readonly launchURL?: string;

However, the struct we created had:

    const dataPayload = {
      data,
      url,
      buttons: buttons
        ? convertButtonsToNotificationActionType(buttons)
        : undefined,
    };

url != launchURL

Fixing the payload made it start working.

Systems Affected

  • WebSDK
  • Backend
  • Dashboard

Validation

Tests

Info

I only manually tested this. If we need automated tests, please show me where to get started.

Checklist

  • All the automated tests pass or I explained why that is not possible
  • I have personally tested this on my machine or explained why that is not possible
  • I have included test coverage for these changes or explained why they are not needed

Programming Checklist
Interfaces:

  • Don't use default export
  • New interfaces are in model files

Functions:

  • Don't use default export
  • All function signatures have return types
  • Helpers should not access any data but rather be given the data to operate on.

Typescript:

  • No Typescript warnings
  • Avoid silencing null/undefined warnings with the exclamation point

Other:

  • Iteration: refrain from using elem of array syntax. Prefer forEach or use map
  • Avoid using global OneSignal accessor for context if possible. Instead, we can pass it to function/constructor so that we don't call OneSignal.context

Screenshots

image

image

Info

Checklist

  • I have included screenshots/recordings of the intended results or explained why they are not needed

Related Tickets

https://app.asana.com/0/1208719710991963/1208760940956731/f


This change is Reviewable

…ication

https://app.asana.com/0/1208719710991963/1208760940956731/f

This issue boiled down to the "Open link when clicking welcome notification"
feature not working, when you clicked the notification is opened the 'default'
URL, not the option set.

After doing a bunch of digging, I was able to confirm that we were saving that setting
and then servcing it to the SDK via the sync API.

I was then able to trace it through:

1) src/shared/helpers/EventHelper.ts (onSubscriptionChanged_showWelcomeNotification)
2) src/shared/helpers/MainHelper.ts (MainHelper.showLocalNotification)

There we shovel that url into a dataPayload that we shovel into a service worker call.

That comes out the other side when you click the notifiction in:

3) src/sw/serviceWorker/ServiceWorker.ts (onNotificationClicked)

There we cast via:

const osNotification = event.notification.data as IOSNotification;

IOSNotification is defined to have:

```
readonly launchURL?: string;
```

However, the struct we created had:

```
    const dataPayload = {
      data,
      url,
      buttons: buttons
        ? convertButtonsToNotificationActionType(buttons)
        : undefined,
    };
```

url != launchURL

Fixing the payload made it start working.
@chamons chamons requested review from jkasten2 and rgomezp November 18, 2024 20:26
Copy link
Contributor

@rgomezp rgomezp left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work!

@chamons chamons merged commit 1d15945 into main Nov 18, 2024
4 checks passed
@chamons chamons deleted the fix_notification_button_launch_url branch November 18, 2024 22:25
@jkasten2 jkasten2 mentioned this pull request Nov 19, 2024
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.

4 participants