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.

Sign up for GitHub

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

Jump to bottom

optimsitic and new comment/admire push notifications #1159

Merged
merged 4 commits into from
Aug 21, 2023
Merged

optimsitic and new comment/admire push notifications #1159

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
247cfb2
optimsitic and new comment/admire push notifications
benny-conn Aug 17, 2023
c1610d4
truncate long names
benny-conn Aug 17, 2023
187b324
Merge branch 'main' into benny/new-push-notifications
benny-conn Aug 17, 2023
aae74be
wrong join var
benny-conn Aug 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions db/gen/coredb/query.sql.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions db/queries/core/query.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,9 @@ SELECT c.* FROM galleries g, unnest(g.collections)
-- name: GetTokenById :one
select * from tokens where id = $1 and displayable and deleted = false;

-- name: GetTokenMediaByTokenId :one
select tm.* from tokens join token_medias tm on tokens.id = tm.token_id where tokens.id = $1 and tokens.displayable and not tokens.deleted and not tm.deleted;
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure tm.token_id is the on-chain "token ID," not the token's actual DBID, right? I think you want tokens.token_media_id = tm.id.


-- name: GetTokenByIdBatch :batchone
select * from tokens where id = $1 and displayable and deleted = false;

Expand Down
64 changes: 60 additions & 4 deletions service/notifications/notifications.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ type pushLimiter struct {
comments *limiters.KeyRateLimiter
admires *limiters.KeyRateLimiter
follows *limiters.KeyRateLimiter
tokens *limiters.KeyRateLimiter
}

func newPushLimiter() *pushLimiter {
Expand All @@ -57,6 +58,7 @@ func newPushLimiter() *pushLimiter {
comments: limiters.NewKeyRateLimiter(ctx, cache, "comments", 5, time.Minute),
admires: limiters.NewKeyRateLimiter(ctx, cache, "admires", 1, time.Minute*10),
follows: limiters.NewKeyRateLimiter(ctx, cache, "follows", 1, time.Minute*10),
tokens: limiters.NewKeyRateLimiter(ctx, cache, "tokens", 1, time.Minute*10),
}
}

Expand Down Expand Up @@ -101,6 +103,18 @@ func (p *pushLimiter) tryFollow(ctx context.Context, sendingUserID persist.DBID,
}
}

func (p *pushLimiter) tryTokens(ctx context.Context, sendingUserID persist.DBID, tokenID persist.DBID) error {
key := fmt.Sprintf("%s:%s", sendingUserID, tokenID)
if p.isActionAllowed(ctx, p.tokens, key) {
return nil
}

return errRateLimited{
limiterName: p.follows.Name(),
senderID: sendingUserID,
}
}

func (p *pushLimiter) isActionAllowed(ctx context.Context, limiter *limiters.KeyRateLimiter, key string) bool {
canContinue, _, err := limiter.ForKey(ctx, key)
if err != nil {
Expand Down Expand Up @@ -493,7 +507,7 @@ func createPushMessage(ctx context.Context, notif db.Notification, queries *db.Q
},
}

if notif.Action == persist.ActionAdmiredFeedEvent {
if notif.Action == persist.ActionAdmiredFeedEvent || notif.Action == persist.ActionAdmiredPost {
admirer, err := queries.GetUserById(ctx, notif.Data.AdmirerIDs[0])
if err != nil {
return task.PushNotificationMessage{}, err
Expand All @@ -507,11 +521,16 @@ func createPushMessage(ctx context.Context, notif db.Notification, queries *db.Q
return task.PushNotificationMessage{}, fmt.Errorf("user with ID=%s has no username", admirer.ID)
}

message.Body = fmt.Sprintf("%s admired your activity", admirer.Username.String)
ac := "activity"
if notif.Action == persist.ActionAdmiredPost {
ac = "post"
}

message.Body = fmt.Sprintf("%s admired your %s", admirer.Username.String, ac)
return message, nil
}

if notif.Action == persist.ActionCommentedOnFeedEvent {
if notif.Action == persist.ActionCommentedOnFeedEvent || notif.Action == persist.ActionCommentedOnPost {
comment, err := queries.GetCommentByCommentID(ctx, notif.CommentID)
if err != nil {
return task.PushNotificationMessage{}, err
Expand All @@ -530,7 +549,12 @@ func createPushMessage(ctx context.Context, notif db.Notification, queries *db.Q
return task.PushNotificationMessage{}, fmt.Errorf("user with ID=%s has no username", commenter.ID)
}

message.Body = fmt.Sprintf("%s commented on your activity", commenter.Username.String)
ac := "activity"
if notif.Action == persist.ActionCommentedOnPost {
ac = "post"
}

message.Body = fmt.Sprintf("%s commented on your %s", commenter.Username.String, ac)
return message, nil
}

Expand Down Expand Up @@ -558,6 +582,32 @@ func createPushMessage(ctx context.Context, notif db.Notification, queries *db.Q
message.Body = body
return message, nil
}
if notif.Action == persist.ActionNewTokensReceived {

tm, err := queries.GetTokenMediaByTokenId(ctx, notif.Data.NewTokenID)
if err != nil {
return task.PushNotificationMessage{}, err
}
name := tm.Name
if name == "" {
to, err := queries.GetTokenById(ctx, notif.Data.NewTokenID)
if err != nil {
return task.PushNotificationMessage{}, err
}
name = to.Name.String
}
Comment on lines +587 to +598
Copy link
Collaborator Author

@benny-conn benny-conn Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not exactly happy with this. Just like token_metadata and media we probably want to remove the duplicate fields of name and description from the tokens table and just rely on the token_media. There are more implications to this given that a token should have a name immediately upon sync. This would mean that upon sync every token would have its token_medias column created immediately and partially filled and then token processing can continue working on it async.

I know we have also been talking about joining token_medias on tokens in general. Maybe we can do some sort of coalesce with the first of these two fields then? Still don't like the duplication though...

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, yeah, the duplication here is kind of annoying. Agreed that you could coalesce them and write a more specific query that hits both tables and just returns a name instead of all token media columns. Filling in some of token_medias immediately also sounds A-OK to me, as long as it's obvious which fields are guaranteed to be immediately useful and which fields are being processed. But I also think it's fine to leave it as-is if it's not a problem anywhere else atm. Your call!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I will make a follow-up, maybe fix it week, for creating token_medias immediately and filling them out partially.


if err := limiter.tryTokens(ctx, notif.OwnerID, notif.Data.NewTokenID); err != nil {
return task.PushNotificationMessage{}, err
}
amount := notif.Data.NewTokenQuantity
i := amount.BigInt().Uint64()
if i > 1 {
message.Body = fmt.Sprintf("You received %d new %s tokens", i, name)
} else {
message.Body = fmt.Sprintf("You received a new %s token", name)
}
}

return task.PushNotificationMessage{}, fmt.Errorf("unsupported notification action: %s", notif.Action)
}
Expand All @@ -570,6 +620,12 @@ func actionSupportsPushNotifications(action persist.Action) bool {
return true
case persist.ActionUserFollowedUsers:
return true
case persist.ActionNewTokensReceived:
return true
case persist.ActionCommentedOnPost:
return true
case persist.ActionAdmiredPost:
return true
default:
return false
}
Expand Down
Loading