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

feat: daily stacked and spent sats notification #1756

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

Soxasora
Copy link
Member

@Soxasora Soxasora commented Dec 22, 2024

Description

Daily stacked and spent sats summary addressing #1751

It features a brief summary of how much a user has stacked (including rewards) and spent in 24h.

Data is collected from the user_stats_days view following the America/Chicago timezone, if the record is still not available past the midnight for other timezones, the latest record available is pulled (2 day interval1, limited to 1).

SN-worker triggers a push notification every midnight

The notification itself is clickable and links to the full Satistics page

Screenshots

Notification
image

Settings
image

Additional Context

#1750 will not conflict with this new type of notification, we would need to give it a filter category though!

Checklist

Are your changes backwards compatible? Please answer below:

It needs migrations to allow toggling and scheduler support but other than that these are only additions!

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:

6, data collection happens unequivocally from user_stats_days, the notification also has a fallback to the previous one if newer data isn't available. Push notifications are not tested yet but follows the same design as the others, worker correctly starts and finish the job at midnight

For frontend changes: Tested on mobile, light and dark mode? Please answer below: The displayed notification follows the same design language as the others

Did you introduce any new environment variables? If so, call them out explicitly here: No

TODO:

  • push notification
  • cleanup
  • fix notification is not always available (timezone)
  • slimmer query
  • refactor DailyStats to SatSummary
  • settings toggle for daily stats
  • notification component
  • notification resolver

edit:

Footnotes

  1. this also serves as a 1-day fallback before removing the notification in case there's no sat activity

@Soxasora Soxasora marked this pull request as ready for review December 23, 2024 18:22
@Soxasora Soxasora marked this pull request as draft December 23, 2024 19:10
@Soxasora Soxasora marked this pull request as ready for review December 23, 2024 19:11
@Soxasora Soxasora marked this pull request as draft December 24, 2024 11:46
@Soxasora Soxasora marked this pull request as ready for review December 24, 2024 11:46
Copy link
Member Author

Choose a reason for hiding this comment

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

realized I didn't push this before, sorry

@Soxasora Soxasora added the feature new product features that weren't there before label Dec 29, 2024
Copy link
Member

@huumn huumn left a comment

Choose a reason for hiding this comment

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

This looks nice. People are going to like this.

I'll QA and UI review tomorrow.

@Soxasora
Copy link
Member Author

Soxasora commented Dec 30, 2024

Sometimes, dailySatSummary runs before views-days updates the user_stats_days materialized view, causing it to fail in obtaining new data. To ensure the availability of new data, I decided to pair dailySatSummary with views-days since they both run at midnight anyway, I hope it's okay.

Edit:
While working on iOS push notifications, I realized that I didn't include DAILY_STATS tag inside immediate push notifications, this should do it

@huumn
Copy link
Member

huumn commented Dec 31, 2024

Heads up: I'm going to create a better seed to test this. I won't review until tomorrow.

Sometimes, dailySatSummary runs before views-days updates the user_stats_days materialized view, causing it to fail in obtaining new data. To ensure the availability of new data, I decided to pair dailySatSummary with views-days since they both run at midnight anyway, I hope it's okay.

This is an okay way to do this if we are dependent on those views afaict.


I think there may be a more general UX issue with this approach though and it's tricky to solve. By depending on user_stats_days, this misses the rewards and territory revenue which are computed just after midnight. I think people will expect the notification to include those recent numbers instead of the ones from earlier in the day. For example:

As is, and correct me if I'm wrong, after midnight on 1/1:

  1. this summary notification is shown with data from 12/31/2024 (including rewards/revenue earned from 12/30)
  2. rewards and territory revenue are computed for 12/31/2024, on 1/1/2025, and notifications are shown
  3. stacked/spent from (2) won't be summarized until 1/2/2025

The above won't block my approval and payment. It looks like you did all the important parts so I should have an easy time doing whatever is required to fix this ux thing.

@Soxasora
Copy link
Member Author

By depending on user_stats_days, this misses the rewards and territory revenue which are computed just after midnight.

I noticed this behavior the time I tested territory fees, I was conflicted on whether doing something about it or leave it as-is because territory revenue was being given after the midnight, so I just waited for your opinion on it.

I considered:

  • Subtracting msats_reward, msats_revenue from user_stats_days.msats_stacked with a LEFT JOIN from SubAct and Earn to get the latest data

@huumn
Copy link
Member

huumn commented Dec 31, 2024

I think the move might be to create a new view that's computed at 12:15a just for this purpose. I'm not sure there's a better way.

@Soxasora
Copy link
Member Author

Soxasora commented Dec 31, 2024

Agree, it would also be its own pgboss job because we wouldn't need views-days anymore

edit: also I noticed that I have 2 commits in the future because I was testing with time, didn't rebase because it could mess with your tree

@huumn
Copy link
Member

huumn commented Dec 31, 2024

edit: also I noticed that I have 2 commits in the future because I was testing with time, didn't rebase because it could mess with your tree

lol I was wondering why my comments on github weren't showing up. I think you found a github bug. github trusts git a lot.

@huumn
Copy link
Member

huumn commented Jan 1, 2025

Sorry, been slacking. I'm currently working on a seed for this.

@Soxasora Soxasora marked this pull request as draft January 7, 2025 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature new product features that weren't there before
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants