tangled.org
Signed-off-by: Seongmin Lee git@boltless.me
+1
-1
appview/issues/issues.go
+1
-1
appview/issues/issues.go
···
489
489
// reset atUri to make rollback a no-op
490
490
atUri = ""
491
491
492
-
rp.notifier.NewIssueComment(r.Context(), &comment, mentions)
492
+
rp.notifier.NewComment(r.Context(), &comment)
493
493
494
494
ownerSlashRepo := reporesolver.GetBaseRepoPath(r, f)
495
495
rp.pages.HxLocation(w, fmt.Sprintf("/%s/issues/%d#comment-%d", ownerSlashRepo, issue.IssueId, comment.Id))
+111
-118
appview/notify/db/db.go
+111
-118
appview/notify/db/db.go
···
74
74
// no-op
75
75
}
76
76
77
-
func (n *databaseNotifier) NewIssue(ctx context.Context, issue *models.Issue, mentions []syntax.DID) {
78
-
collaborators, err := db.GetCollaborators(n.db, orm.FilterEq("repo_at", issue.Repo.RepoAt()))
77
+
func (n *databaseNotifier) NewComment(ctx context.Context, comment *models.Comment) {
78
+
var (
79
+
// built the recipients list:
80
+
// - the owner of the repo
81
+
// - | if the comment is a reply -> everybody on that thread
82
+
// | if the comment is a top level -> just the issue owner
83
+
// - remove mentioned users from the recipients list
84
+
recipients = sets.New[syntax.DID]()
85
+
entityType string
86
+
entityId string
87
+
repoId *int64
88
+
issueId *int64
89
+
pullId *int64
90
+
)
91
+
92
+
subjectDid, err := comment.Subject.Authority().AsDID()
79
93
if err != nil {
80
-
log.Printf("failed to fetch collaborators: %v", err)
94
+
log.Printf("NewComment: expected did based at-uri for comment.subject")
81
95
return
82
96
}
97
+
switch comment.Subject.Collection() {
98
+
case tangled.RepoIssueNSID:
99
+
issues, err := db.GetIssues(
100
+
n.db,
101
+
orm.FilterEq("did", subjectDid),
102
+
orm.FilterEq("rkey", comment.Subject.RecordKey()),
103
+
)
104
+
if err != nil {
105
+
log.Printf("NewComment: failed to get issues: %v", err)
106
+
return
107
+
}
108
+
if len(issues) == 0 {
109
+
log.Printf("NewComment: no issue found for %s", comment.Subject)
110
+
return
111
+
}
112
+
issue := issues[0]
113
+
114
+
recipients.Insert(syntax.DID(issue.Repo.Did))
115
+
if comment.IsReply() {
116
+
// if this comment is a reply, then notify everybody in that thread
117
+
parentAtUri := *comment.ReplyTo
118
+
119
+
// find the parent thread, and add all DIDs from here to the recipient list
120
+
for _, t := range issue.CommentList() {
121
+
if t.Self.AtUri() == parentAtUri {
122
+
for _, p := range t.Participants() {
123
+
recipients.Insert(p)
124
+
}
125
+
}
126
+
}
127
+
} else {
128
+
// not a reply, notify just the issue author
129
+
recipients.Insert(syntax.DID(issue.Did))
130
+
}
83
131
84
-
// build the recipients list
85
-
// - owner of the repo
86
-
// - collaborators in the repo
87
-
// - remove users already mentioned
88
-
recipients := sets.Singleton(syntax.DID(issue.Repo.Did))
89
-
for _, c := range collaborators {
90
-
recipients.Insert(c.SubjectDid)
132
+
entityType = "issue"
133
+
entityId = issue.AtUri().String()
134
+
repoId = &issue.Repo.Id
135
+
issueId = &issue.Id
136
+
case tangled.RepoPullNSID:
137
+
pulls, err := db.GetPullsWithLimit(
138
+
n.db,
139
+
1,
140
+
orm.FilterEq("owner_did", subjectDid),
141
+
orm.FilterEq("rkey", comment.Subject.RecordKey()),
142
+
)
143
+
if err != nil {
144
+
log.Printf("NewComment: failed to get pulls: %v", err)
145
+
return
146
+
}
147
+
if len(pulls) == 0 {
148
+
log.Printf("NewComment: no pull found for %s", comment.Subject)
149
+
return
150
+
}
151
+
pull := pulls[0]
152
+
153
+
pull.Repo, err = db.GetRepo(n.db, orm.FilterEq("at_uri", pull.RepoAt))
154
+
if err != nil {
155
+
log.Printf("NewComment: failed to get repos: %v", err)
156
+
return
157
+
}
158
+
159
+
recipients.Insert(syntax.DID(pull.Repo.Did))
160
+
for _, p := range pull.Participants() {
161
+
recipients.Insert(syntax.DID(p))
162
+
}
163
+
164
+
entityType = "pull"
165
+
entityId = pull.AtUri().String()
166
+
repoId = &pull.Repo.Id
167
+
p := int64(pull.ID)
168
+
pullId = &p
169
+
default:
170
+
return // no-op
91
171
}
92
-
for _, m := range mentions {
172
+
173
+
for _, m := range comment.Mentions {
93
174
recipients.Remove(m)
94
175
}
95
176
96
-
actorDid := syntax.DID(issue.Did)
97
-
entityType := "issue"
98
-
entityId := issue.AtUri().String()
99
-
repoId := &issue.Repo.Id
100
-
issueId := &issue.Id
101
-
var pullId *int64
102
-
103
177
n.notifyEvent(
104
-
actorDid,
178
+
comment.Did,
105
179
recipients,
106
-
models.NotificationTypeIssueCreated,
180
+
models.NotificationTypeIssueCommented,
107
181
entityType,
108
182
entityId,
109
183
repoId,
···
111
185
pullId,
112
186
)
113
187
n.notifyEvent(
114
-
actorDid,
115
-
sets.Collect(slices.Values(mentions)),
188
+
comment.Did,
189
+
sets.Collect(slices.Values(comment.Mentions)),
116
190
models.NotificationTypeUserMentioned,
117
191
entityType,
118
192
entityId,
···
122
196
)
123
197
}
124
198
125
-
func (n *databaseNotifier) NewIssueComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) {
126
-
issues, err := db.GetIssues(n.db, orm.FilterEq("at_uri", comment.Subject))
199
+
func (n *databaseNotifier) DeleteComment(ctx context.Context, comment *models.Comment) {
200
+
// no-op
201
+
}
202
+
203
+
func (n *databaseNotifier) NewIssue(ctx context.Context, issue *models.Issue, mentions []syntax.DID) {
204
+
collaborators, err := db.GetCollaborators(n.db, orm.FilterEq("repo_at", issue.Repo.RepoAt()))
127
205
if err != nil {
128
-
log.Printf("NewIssueComment: failed to get issues: %v", err)
129
-
return
130
-
}
131
-
if len(issues) == 0 {
132
-
log.Printf("NewIssueComment: no issue found for %s", comment.Subject)
206
+
log.Printf("failed to fetch collaborators: %v", err)
133
207
return
134
208
}
135
-
issue := issues[0]
136
209
137
-
// built the recipients list:
138
-
// - the owner of the repo
139
-
// - | if the comment is a reply -> everybody on that thread
140
-
// | if the comment is a top level -> just the issue owner
141
-
// - remove mentioned users from the recipients list
210
+
// build the recipients list
211
+
// - owner of the repo
212
+
// - collaborators in the repo
213
+
// - remove users already mentioned
142
214
recipients := sets.Singleton(syntax.DID(issue.Repo.Did))
143
-
144
-
if comment.IsReply() {
145
-
// if this comment is a reply, then notify everybody in that thread
146
-
parentAtUri := *comment.ReplyTo
147
-
148
-
// find the parent thread, and add all DIDs from here to the recipient list
149
-
for _, t := range issue.CommentList() {
150
-
if t.Self.AtUri() == parentAtUri {
151
-
for _, p := range t.Participants() {
152
-
recipients.Insert(p)
153
-
}
154
-
}
155
-
}
156
-
} else {
157
-
// not a reply, notify just the issue author
158
-
recipients.Insert(syntax.DID(issue.Did))
215
+
for _, c := range collaborators {
216
+
recipients.Insert(c.SubjectDid)
159
217
}
160
-
161
218
for _, m := range mentions {
162
219
recipients.Remove(m)
163
220
}
164
221
165
-
actorDid := syntax.DID(comment.Did)
222
+
actorDid := syntax.DID(issue.Did)
166
223
entityType := "issue"
167
224
entityId := issue.AtUri().String()
168
225
repoId := &issue.Repo.Id
···
172
229
n.notifyEvent(
173
230
actorDid,
174
231
recipients,
175
-
models.NotificationTypeIssueCommented,
232
+
models.NotificationTypeIssueCreated,
176
233
entityType,
177
234
entityId,
178
235
repoId,
···
260
317
)
261
318
}
262
319
263
-
func (n *databaseNotifier) NewPullComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) {
264
-
pulls, err := db.GetPulls(n.db,
265
-
orm.FilterEq("owner_did", comment.Subject.Authority()),
266
-
orm.FilterEq("rkey", comment.Subject.RecordKey()),
267
-
)
268
-
if err != nil {
269
-
log.Printf("NewPullComment: failed to get pulls: %v", err)
270
-
return
271
-
}
272
-
if len(pulls) == 0 {
273
-
log.Printf("NewPullComment: no pull found for %s", comment.Subject)
274
-
return
275
-
}
276
-
pull := pulls[0]
277
-
278
-
repo, err := db.GetRepo(n.db, orm.FilterEq("at_uri", pull.RepoAt))
279
-
if err != nil {
280
-
log.Printf("NewPullComment: failed to get repos: %v", err)
281
-
return
282
-
}
283
-
284
-
// build up the recipients list:
285
-
// - repo owner
286
-
// - all pull participants
287
-
// - remove those already mentioned
288
-
recipients := sets.Singleton(syntax.DID(repo.Did))
289
-
for _, p := range pull.Participants() {
290
-
recipients.Insert(syntax.DID(p))
291
-
}
292
-
for _, m := range mentions {
293
-
recipients.Remove(m)
294
-
}
295
-
296
-
actorDid := comment.Did
297
-
eventType := models.NotificationTypePullCommented
298
-
entityType := "pull"
299
-
entityId := pull.AtUri().String()
300
-
repoId := &repo.Id
301
-
var issueId *int64
302
-
p := int64(pull.ID)
303
-
pullId := &p
304
-
305
-
n.notifyEvent(
306
-
actorDid,
307
-
recipients,
308
-
eventType,
309
-
entityType,
310
-
entityId,
311
-
repoId,
312
-
issueId,
313
-
pullId,
314
-
)
315
-
n.notifyEvent(
316
-
actorDid,
317
-
sets.Collect(slices.Values(mentions)),
318
-
models.NotificationTypeUserMentioned,
319
-
entityType,
320
-
entityId,
321
-
repoId,
322
-
issueId,
323
-
pullId,
324
-
)
325
-
}
326
-
327
320
func (n *databaseNotifier) UpdateProfile(ctx context.Context, profile *models.Profile) {
328
321
// no-op
329
322
}
+8
-8
appview/notify/merged_notifier.go
+8
-8
appview/notify/merged_notifier.go
···
53
53
m.fanout("DeleteStar", ctx, star)
54
54
}
55
55
56
-
func (m *mergedNotifier) NewIssue(ctx context.Context, issue *models.Issue, mentions []syntax.DID) {
57
-
m.fanout("NewIssue", ctx, issue, mentions)
56
+
func (m *mergedNotifier) NewComment(ctx context.Context, comment *models.Comment) {
57
+
m.fanout("NewComment", ctx, comment)
58
+
}
59
+
60
+
func (m *mergedNotifier) DeleteComment(ctx context.Context, comment *models.Comment) {
61
+
m.fanout("DeleteComment", ctx, comment)
58
62
}
59
63
60
-
func (m *mergedNotifier) NewIssueComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) {
61
-
m.fanout("NewIssueComment", ctx, comment, mentions)
64
+
func (m *mergedNotifier) NewIssue(ctx context.Context, issue *models.Issue, mentions []syntax.DID) {
65
+
m.fanout("NewIssue", ctx, issue, mentions)
62
66
}
63
67
64
68
func (m *mergedNotifier) NewIssueState(ctx context.Context, actor syntax.DID, issue *models.Issue) {
···
81
85
m.fanout("NewPull", ctx, pull)
82
86
}
83
87
84
-
func (m *mergedNotifier) NewPullComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) {
85
-
m.fanout("NewPullComment", ctx, comment, mentions)
86
-
}
87
-
88
88
func (m *mergedNotifier) NewPullState(ctx context.Context, actor syntax.DID, pull *models.Pull) {
89
89
m.fanout("NewPullState", ctx, actor, pull)
90
90
}
+7
-7
appview/notify/notifier.go
+7
-7
appview/notify/notifier.go
···
13
13
NewStar(ctx context.Context, star *models.Star)
14
14
DeleteStar(ctx context.Context, star *models.Star)
15
15
16
+
NewComment(ctx context.Context, comment *models.Comment)
17
+
DeleteComment(ctx context.Context, comment *models.Comment)
18
+
16
19
NewIssue(ctx context.Context, issue *models.Issue, mentions []syntax.DID)
17
-
NewIssueComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID)
18
20
NewIssueState(ctx context.Context, actor syntax.DID, issue *models.Issue)
19
21
DeleteIssue(ctx context.Context, issue *models.Issue)
20
22
···
22
24
DeleteFollow(ctx context.Context, follow *models.Follow)
23
25
24
26
NewPull(ctx context.Context, pull *models.Pull)
25
-
NewPullComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID)
26
27
NewPullState(ctx context.Context, actor syntax.DID, pull *models.Pull)
27
28
28
29
UpdateProfile(ctx context.Context, profile *models.Profile)
···
42
43
func (m *BaseNotifier) NewStar(ctx context.Context, star *models.Star) {}
43
44
func (m *BaseNotifier) DeleteStar(ctx context.Context, star *models.Star) {}
44
45
46
+
func (m *BaseNotifier) NewComment(ctx context.Context, comment *models.Comment) {}
47
+
func (m *BaseNotifier) DeleteComment(ctx context.Context, comment *models.Comment) {}
48
+
45
49
func (m *BaseNotifier) NewIssue(ctx context.Context, issue *models.Issue, mentions []syntax.DID) {}
46
-
func (m *BaseNotifier) NewIssueComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) {
47
-
}
48
50
func (m *BaseNotifier) NewIssueState(ctx context.Context, actor syntax.DID, issue *models.Issue) {}
49
51
func (m *BaseNotifier) DeleteIssue(ctx context.Context, issue *models.Issue) {}
50
52
51
53
func (m *BaseNotifier) NewFollow(ctx context.Context, follow *models.Follow) {}
52
54
func (m *BaseNotifier) DeleteFollow(ctx context.Context, follow *models.Follow) {}
53
55
54
-
func (m *BaseNotifier) NewPull(ctx context.Context, pull *models.Pull) {}
55
-
func (m *BaseNotifier) NewPullComment(ctx context.Context, models *models.Comment, mentions []syntax.DID) {
56
-
}
56
+
func (m *BaseNotifier) NewPull(ctx context.Context, pull *models.Pull) {}
57
57
func (m *BaseNotifier) NewPullState(ctx context.Context, actor syntax.DID, pull *models.Pull) {}
58
58
59
59
func (m *BaseNotifier) UpdateProfile(ctx context.Context, profile *models.Profile) {}
+4
-18
appview/notify/posthog/notifier.go
+4
-18
appview/notify/posthog/notifier.go
···
86
86
}
87
87
}
88
88
89
-
func (n *posthogNotifier) NewPullComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) {
90
-
err := n.client.Enqueue(posthog.Capture{
91
-
DistinctId: comment.Did.String(),
92
-
Event: "new_pull_comment",
93
-
Properties: posthog.Properties{
94
-
"pull_at": comment.Subject,
95
-
"mentions": mentions,
96
-
},
97
-
})
98
-
if err != nil {
99
-
log.Println("failed to enqueue posthog event:", err)
100
-
}
101
-
}
102
-
103
89
func (n *posthogNotifier) NewPullClosed(ctx context.Context, pull *models.Pull) {
104
90
err := n.client.Enqueue(posthog.Capture{
105
91
DistinctId: pull.OwnerDid,
···
179
165
}
180
166
}
181
167
182
-
func (n *posthogNotifier) NewIssueComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) {
168
+
func (n *posthogNotifier) NewComment(ctx context.Context, comment *models.Comment) {
183
169
err := n.client.Enqueue(posthog.Capture{
184
170
DistinctId: comment.Did.String(),
185
-
Event: "new_issue_comment",
171
+
Event: "new_comment",
186
172
Properties: posthog.Properties{
187
-
"issue_at": comment.Subject,
188
-
"mentions": mentions,
173
+
"subject_at": comment.Subject,
174
+
"mentions": comment.Mentions,
189
175
},
190
176
})
191
177
if err != nil {
+1
-1
appview/pulls/pulls.go
+1
-1
appview/pulls/pulls.go
···
794
794
return
795
795
}
796
796
797
-
s.notifier.NewPullComment(r.Context(), &comment, mentions)
797
+
s.notifier.NewComment(r.Context(), &comment)
798
798
799
799
ownerSlashRepo := reporesolver.GetBaseRepoPath(r, f)
800
800
s.pages.HxLocation(w, fmt.Sprintf("/%s/pulls/%d#comment-%d", ownerSlashRepo, pull.PullId, comment.Id))
History
8 rounds
6 comments
boltless.me
submitted
#7
1 commit
expand
collapse
appview/notify: merge new comment events into one
Signed-off-by: Seongmin Lee <git@boltless.me>
2/3 failed, 1/3 success
expand
collapse
merge conflicts detected
expand
collapse
expand
collapse
- appview/notify/db/db.go:260
- appview/notify/merged_notifier.go:81
- appview/notify/notifier.go:22
- appview/pulls/opengraph.go:277
expand 0 comments
boltless.me
submitted
#6
1 commit
expand
collapse
appview/notify: merge new comment events into one
Signed-off-by: Seongmin Lee <git@boltless.me>
1/3 failed, 2/3 success
expand
collapse
expand 0 comments
boltless.me
submitted
#5
1 commit
expand
collapse
appview/notify: merge new comment events into one
Signed-off-by: Seongmin Lee <git@boltless.me>
3/3 success
expand
collapse
expand 0 comments
boltless.me
submitted
#4
1 commit
expand
collapse
appview/notify: merge new comment events into one
Signed-off-by: Seongmin Lee <git@boltless.me>
3/3 success
expand
collapse
expand 2 comments
@oppi.li thank you for the review!
when we intend to perform backfill of records, we should support ingesting the old records.
Yeah I think it would be better to leave ingester logic for issue.comment update/delete operations. Users should be able to modify existing records.
Backfilling issue.comment will be quite tricky as we cannot distinguish new comments since certain timestamp. Users can modify both created field and rkey.
So there are only two options:
- ingest legacy comment records until we completely drop them (we should silently migrate user records as much as possible before that)
- have "legacy records" list in tangled appview and only ingest for those records. This way, other appviews won't be able to support legacy records.
we can stop maintaining the old record for much longer, although i suspect that the new comment record will be quite stable
There are few issues that can threat the comment lexicon stability:
I think we are roughly fine after changing body field type to sh.tangled.markup#markdown. Though we might we need more discussion for #328 just in case.
boltless.me
submitted
#3
1 commit
expand
collapse
appview/notify: merge new comment events into one
Signed-off-by: Seongmin Lee <git@boltless.me>
1/3 failed, 2/3 success
expand
collapse
expand 0 comments
boltless.me
submitted
#2
1 commit
expand
collapse
appview/notify: merge new comment events into one
Signed-off-by: Seongmin Lee <git@boltless.me>
3/3 success
expand
collapse
expand 0 comments
boltless.me
submitted
#1
1 commit
expand
collapse
appview/notify: merge new comment events into one
Signed-off-by: Seongmin Lee <git@boltless.me>
3/3 success
expand
collapse
boltless.me
submitted
#0
1 commit
expand
collapse
appview/notify: merge new comment events into one
Signed-off-by: Seongmin Lee <git@boltless.me>
1/3 failed, 2/3 timeout
expand
collapse
expand 3 comments
note: I'm choosing sh.tangled.comment NSID here. Tell me if we would prefer something else like sh.tangled.feed.comment
I think sh.tangled.comment is fine, but more importantly, are we still sticking to sh.tangled or does it make sense to move this to the org.tangled namespace? Assuming we're moving everything eventually. cc @oppi.li
I think we should stick on sh.tangled for now as there can be large amounts of changes from PR refactor / did-for-repo. We can consider moving entire collections to org.tangled once tangled lexicons become more stable.
on the whole this changeset lgtm. some questions i have regarding backwards compatibility:
sh.tangled.issue.commentwill not be ingested anymore