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

More push notification types #530

Merged
merged 14 commits into from
Oct 4, 2023

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented Sep 28, 2023

Closes #372

  • earning cowboy hat
  • losing cowboy hat
  • daily reward
  • invites
  • deposits
  • referrals
  • job updates

videos

deposits:

2023-09-29.00-33-35.mp4

earning & losing cowboy hats:

2023-10-04.20-58-38.mp4

daily rewards:

2023-10-04.21-07-47.mp4

@ekzyis ekzyis marked this pull request as draft September 28, 2023 22:16
@ekzyis ekzyis force-pushed the 372-more-push-notification-types branch 3 times, most recently from 4ae3a78 to e39b366 Compare September 28, 2023 23:16
worker/streak.js Outdated Show resolved Hide resolved
@ekzyis ekzyis force-pushed the 372-more-push-notification-types branch from e39b366 to 7fd0a3f Compare October 3, 2023 15:59
@ekzyis
Copy link
Member Author

ekzyis commented Oct 4, 2023

This is done except for job updates. For push notifications on Job updates, we need to check forupdates here.

Unfortunately, I will most likely not be able to finish this until next week since I'll be in Berlin tomorrow.

So I would say let's review this as is and merge since I think job update push notifications are not relevant for 99% of stackers.

Also, I wasn't able to test invites and referrals. For referrals, I think there is still this bug where it doesn't work for lightning or nostr. And with invites, I get redirected to /_next/data/development/invites/clncacp0p0001p8wdfr2zgytk.json?id=clncacp0p0001p8wdfr2zgytk&type=lightning after signup for some reason.

@ekzyis ekzyis marked this pull request as ready for review October 4, 2023 22:00
@ekzyis
Copy link
Member Author

ekzyis commented Oct 4, 2023

Ahh, I missed the release nvm then haha

@huumn
Copy link
Member

huumn commented Oct 4, 2023

You missed it just by a hair. I checked back at least 5 times because I kept seeing notifications.

We can still deploy it soon if it only needs your stamp of approval.

@ekzyis
Copy link
Member Author

ekzyis commented Oct 4, 2023

We can still deploy it soon if it only needs your stamp of approval.

Good to go from me, I can't approve my own PR in Github

@huumn
Copy link
Member

huumn commented Oct 4, 2023

Looks really well done! Great job!

@huumn huumn merged commit 425220d into stackernews:master Oct 4, 2023
1 check passed
Comment on lines +165 to +168
sendUserNotification(earner.userId, {
title: `you stacked ${numWithUnits(msatsToSats(earnings), { abbreviate: false })} in rewards`,
tag: 'EARN'
}).catch(console.error)
Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, there is one notification per user and type and not one notification per user only. So only the last notification shows up since the others before get replaced.
I have to sum the rewards up in the serviceworker (or sum them up here and only send one push notification) and I can include the rewards per type in the body then.

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.

More push notification types
2 participants