replies and mentions #1155

benny-conn · 2023-08-15T22:21:15Z

Changes:

Added 3 notifications: SomeoneRepliedToYourCommentNotification, SomeoneMentionedYouNotification, and SomeoneMentionedYourCommunityNotification notifications. Mentions are manually passed into the mutations for creating comments and posts as opposed to being detected by the backend because a community can be mentioned and because there is not uniqueness between community names and user names we need to have specificity between the two
Added contract_id to the events and notifications table for the new community notification

Considerations:

None of these notifications include any sort of "who" field that shows who mentioned you or replied, because this information can be inferred by for example the post creator or comment creator. Do we want to double up anyway and add a resolving field for who the notification was kicked off by
There is no notification kicked off for mentioning someone in a collectors note (and in turn the feed event associated with adding a collectors note) or a user bio. Other platforms tend to not notify in cases like this as well and tend to only notify in very timing dependent contexts. A bio or a description is not closely tied with time because it is supposed to represent some info for the user or token universally. A post or a comment however exists in time, when a post is posted is important to the context of a post and same for a comment. Is this how we intend to have mentions work as well? @kaitoo1
We aren't storing the mentions in the database anywhere else, like on the comments or posts table. Do we need to? They are immortalized as events and notifications only currently.

TODO:

Test SomeoneMentionedYourCommunityNotification

benny-conn · 2023-08-15T23:16:26Z

event/event.go

+		u, err := h.dataloaders.UserByAddress.Load(db.GetUserByAddressBatchParams{
+			Address: c.OwnerAddress,
+			Chain:   int32(c.Chain),
+		})
+		if err != nil {
+			logger.For(ctx).Warnf("error loading user by address: %s", err)
+			return "", nil


It is likely that a user does not exist. Not every contract creator is on gallery, in this case, don't error but don't return anything so there will be no notification sent.

radazen

Looks great to me! I left some comments. One other question: any idea how the frontend is going to handle mentions, both in terms of inputting them, and in terms of displaying them? Is it an inline mention like

hey jarrel I think you might like this!

or more of a tag-with-the-post like

hey I think you might like this! --with jarrel

If it's the former, how do we figure out which part of the comment text gets the mention highlighting? If it's the latter, I guess it's not a problem we need to solve!

radazen · 2023-08-16T20:32:58Z

db/queries/core/query.sql

@@ -15,6 +15,9 @@ SELECT * FROM users WHERE id = ANY(@user_ids) AND deleted = false
             CASE WHEN NOT @paging_forward::bool THEN (created_at, id) END DESC
    LIMIT $1;

+-- name: GetAllUsersByIDs :many


This should probably be GetUsersByUsernames, since it's checking username_idempotent and not id. The sqlc.arg variable should probably be usernames, too. Or maybe lowercase_usernames, since that's a requirement?

Follow-up: is this actually being used? I couldn't find any references to it.

Was not being used! removed

radazen · 2023-08-16T20:45:09Z

db/queries/core/query.sql

 -- name: CreateAdmireEvent :one
 INSERT INTO events (id, actor_id, action, resource_type_id, admire_id, feed_event_id, post_id, subject_id, data, group_id, caption) VALUES ($1, $2, $3, $4, $5, sqlc.narg('feed_event'), sqlc.narg('post'), $5, $6, $7, $8) RETURNING *;

 -- name: CreateCommentEvent :one
-INSERT INTO events (id, actor_id, action, resource_type_id, comment_id, feed_event_id, post_id, subject_id, data, group_id, caption) VALUES ($1, $2, $3, $4, $5, sqlc.narg('feed_event'), sqlc.narg('post'), $5, $6, $7, $8) RETURNING *;
+INSERT INTO events (id, actor_id, action, resource_type_id, comment_id, feed_event_id, post_id, subject_id, data, group_id, caption) VALUES ($1, $2, $3, $4, $5, sqlc.narg('feed_event'), sqlc.narg('post'), $6, $7, $8, $9) RETURNING *;


Is this parameter adjustment applicable to CreateAdmireEvent, too?

radazen · 2023-08-17T14:22:48Z

event/event.go

 	if err != nil {
 		return err
 	}

+	if owner == "" {


Could we leave a comment here explaining why an empty owner is an expected case? I understand the rationale, but I'll probably forget by the time I'm looking at this code in the future!

radazen · 2023-08-17T14:32:04Z

event/event.go

+		if err != nil {
+			return "", err
+		}
+		u, err := h.dataloaders.UserByAddress.Load(db.GetUserByAddressBatchParams{


I think you'll need to use the contract_creators view here. Because we can manually override contract creator status (i.e. we can set a particular user to be the creator of a contract even if they don't have the address that owns it), determining a contract's creator is a little more involved than just checking the address. Fortunately, contract_creators exists for that exact purpose, and it'll give you the correct owner if you look up a contract ID.

Did not know about this view! Thank you!

radazen · 2023-08-17T16:33:21Z

graphql/resolver/schema.resolvers.helpers.go

@@ -132,6 +132,34 @@ var nodeFetcher = model.NodeFetcher{

 		notifConverted := notif.(model.NewTokensNotification)

+		return &notifConverted, nil
+	},
+	OnSomeoneMentionedYouNotification: func(ctx context.Context, dbid persist.DBID) (*model.SomeoneMentionedYouNotification, error) {


Since all of these notification fetch functions are identical except for the model type (including the existing ones that predate this PR), could we replace all the boilerplate with a generic helper? Something like

func fetchNotificationByID[T model.Notification] (ctx context.Context, did persist.DBID) (*T, error) { notif, err := resolveNotificationByID(ctx, dbid) if err != nil { return nil, err } notifConverted := notif.(T) return &notifConverted, nil } ... OnSomeoneMentionedYouNotification: fetchNotificationByID[model.SomeoneMentionedYouNotification]

radazen · 2023-08-17T16:42:17Z

graphql/resolver/schema.resolvers.helpers.go

@@ -1923,8 +2015,8 @@ func communityToModel(ctx context.Context, community db.Contract, forceRefresh *

 	// TODO: Should this use CreatorAddress or OwnerAddress?


Seems like we can remove the TODO if OwnerAddress is correct one to use, yeah?

On a related note: do you know if we actually set creator_address in the database anymore? I was looking at it a while back, and while we do have existing values for it, I didn't see anywhere that we'd update that column. Has it been entirely replaced by owner_address, or does it still serve a purpose?

We don't use that column at all anymore as far as the backend goes. Although I imagine we may want to use it in the future to distinct contract deployers to contract owners, but for now we only care about the "owner".

radazen · 2023-08-17T16:56:48Z

graphql/schema/schema.graphql

+  updatedTime: Time
+
+  # Don't need a `who` here since the `comment` or `post` can provide that
+  comment: Comment @goField(forceResolver: true)


Is it possible for both comment and post to be non-null? I'm trying to think through how we'd use this on the frontend. If only one of the available sources is going to be filled in, it'd probably make more sense to have a union type, like:

union MentionSource = Comment | Post

And then the frontend can check the returned type:

...on Comment { } ...on Post { }

On the other hand, if both sources can be non-null, what would that mean? Perhaps that someone replied to your comment on a particular post? Even in that case, it might make more sense to use a union, cause then the frontend isn't in charge of figuring out what different permutations mean (comment with no post means mentioned in comment, comment with post means mentioned in comment on a post, post with no comment means mentioned in a post, etc).

Though in hindsight, as I was typing that, I was assuming we had a Comment.post field that could resolve the Post that a comment was left on, and it doesn't look like we have that yet. How difficult would that be to add?

Love the idea of a union type here.

As far as comment.post goes, there is a code comment on the comment type about whether to include the feed event, so maybe we return a union type as well on the comment for the entity being commented on, either feed event or post.

kaitoo1 · 2023-08-18T01:16:36Z

Looks great to me! I left some comments. One other question: any idea how the frontend is going to handle mentions, both in terms of inputting them, and in terms of displaying them? Is it an inline mention like

hey jarrel I think you might like this!

or more of a tag-with-the-post like

hey I think you might like this! --with jarrel

If it's the former, how do we figure out which part of the comment text gets the mention highlighting? If it's the latter, I guess it's not a problem we need to solve!

@radazen we're going to be displaying them inline, with an "@"

In terms of input, the user will type "@" and then the username inline within the comment, description etc. typing "@" will trigger a search box from which they can select the entity they want to mention.

radazen · 2023-08-18T03:08:58Z

@kaitoo1 Thanks for explaining! So if we have a text comment and a list of IDs (user or community) that have been mentioned, how to we combine those to get the desired output? Look up the user/community and search for their names with an @ in front of them? Seems like there would be edge cases there: users or communities changing their names, mentioning two communities like @Gallery and @Gallery Presents: Some Stuff and making sure we do greedy matching, two mentioned communities with the same name, a user and a community with the same name, etc.

radazen · 2023-08-18T18:00:53Z

@kaitoo1 Thanks for explaining! So if we have a text comment and a list of IDs (user or community) that have been mentioned, how to we combine those to get the desired output? Look up the user/community and search for their names with an @ in front of them? Seems like there would be edge cases there: users or communities changing their names, mentioning two communities like @Gallery and @Gallery Presents: Some Stuff and making sure we do greedy matching, two mentioned communities with the same name, a user and a community with the same name, etc.

I guess what I'm trying to say is: do we have enough information to make inline mentions work, or should we include additional details in the mention input type? I.e. instead of just the ID of the person who was mentioned, maybe a position + length indicating which part of the comment text corresponds to the mention?

* deleted comments and reply pagination * Include only posts (#1157) * typo * remove unused comment queries * fix reply to and count replies * migration for updating comment indicies * removed comments --------- Co-authored-by: jarrel <[email protected]>

* mention data * mention entity * dummy commit * mentions table * notification * rename migration, fix queries, rename index

benny-conn added 2 commits August 15, 2023 14:31

initial implementation

e01ee90

correct subject, post mentions

4ec8e54

benny-conn requested review from radazen, jarrel-b and kaitoo1 August 15, 2023 22:21

github-actions bot added migration schema query labels Aug 15, 2023

resolve community mention correctly

75b1cbc

benny-conn commented Aug 15, 2023

View reviewed changes

radazen reviewed Aug 17, 2023

View reviewed changes

benny-conn added 3 commits August 17, 2023 13:35

Merge branch 'main' into benny/replies-mentions

f148087

initial comment response

fd6f6a7

Merge branch 'main' into benny/replies-mentions

cc1476f

github-actions bot added the dataloader label Aug 21, 2023

benny-conn added 11 commits August 22, 2023 12:44

Merge branch 'main' into benny/replies-mentions

89264f0

Benny/mention data new (#1170)

dd4a741

* mention data * mention entity * dummy commit * mentions table * notification * rename migration, fix queries, rename index

bump migration numbers

03b64cb

fix conflicts

9cc9113

merge main

9475017

merge main

c1b2854

dummy commit

5175311

merge main

e835108

migration numbers

b811dbd

push notifications for new events

7f28a4d

Merge branch 'main' into benny/replies-mentions

23e79ea

skip replyto

f8a4dc7

benny-conn merged commit b8c2324 into main Oct 3, 2023
6 checks passed

benny-conn deleted the benny/replies-mentions branch October 3, 2023 21:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replies and mentions #1155

replies and mentions #1155

benny-conn commented Aug 15, 2023 •

edited

Loading

benny-conn Aug 15, 2023

radazen left a comment

radazen Aug 16, 2023

radazen Aug 17, 2023

benny-conn Aug 17, 2023

radazen Aug 16, 2023

radazen Aug 17, 2023

radazen Aug 17, 2023

benny-conn Aug 17, 2023

radazen Aug 17, 2023

radazen Aug 17, 2023

benny-conn Aug 17, 2023 •

edited

Loading

radazen Aug 17, 2023

benny-conn Aug 17, 2023

kaitoo1 commented Aug 18, 2023

radazen commented Aug 18, 2023 •

edited

Loading

radazen commented Aug 18, 2023

		@@ -1923,8 +2015,8 @@ func communityToModel(ctx context.Context, community db.Contract, forceRefresh *

		// TODO: Should this use CreatorAddress or OwnerAddress?

replies and mentions #1155

replies and mentions #1155

Conversation

benny-conn commented Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

radazen left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benny-conn Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaitoo1 commented Aug 18, 2023

radazen commented Aug 18, 2023 • edited Loading

radazen commented Aug 18, 2023

benny-conn commented Aug 15, 2023 •

edited

Loading

benny-conn Aug 17, 2023 •

edited

Loading

radazen commented Aug 18, 2023 •

edited

Loading