tangled.org
Signed-off-by: Seongmin Lee git@boltless.me
+1
-1
appview/issues/issues.go
+1
-1
appview/issues/issues.go
···
488
488
// reset atUri to make rollback a no-op
489
489
atUri = ""
490
490
491
-
rp.notifier.NewIssueComment(r.Context(), &comment, mentions)
491
+
rp.notifier.NewComment(r.Context(), &comment)
492
492
493
493
ownerSlashRepo := reporesolver.GetBaseRepoPath(r, f)
494
494
rp.pages.HxLocation(w, fmt.Sprintf("/%s/issues/%d#comment-%d", ownerSlashRepo, issue.IssueId, comment.Id))
+110
-118
appview/notify/db/db.go
+110
-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.GetPulls(
138
+
n.db,
139
+
orm.FilterEq("owner_did", subjectDid),
140
+
orm.FilterEq("rkey", comment.Subject.RecordKey()),
141
+
)
142
+
if err != nil {
143
+
log.Printf("NewComment: failed to get pulls: %v", err)
144
+
return
145
+
}
146
+
if len(pulls) == 0 {
147
+
log.Printf("NewComment: no pull found for %s", comment.Subject)
148
+
return
149
+
}
150
+
pull := pulls[0]
151
+
152
+
pull.Repo, err = db.GetRepo(n.db, orm.FilterEq("at_uri", pull.RepoAt))
153
+
if err != nil {
154
+
log.Printf("NewComment: failed to get repos: %v", err)
155
+
return
156
+
}
157
+
158
+
recipients.Insert(syntax.DID(pull.Repo.Did))
159
+
for _, p := range pull.Participants() {
160
+
recipients.Insert(syntax.DID(p))
161
+
}
162
+
163
+
entityType = "pull"
164
+
entityId = pull.AtUri().String()
165
+
repoId = &pull.Repo.Id
166
+
p := int64(pull.ID)
167
+
pullId = &p
168
+
default:
169
+
return // no-op
91
170
}
92
-
for _, m := range mentions {
171
+
172
+
for _, m := range comment.Mentions {
93
173
recipients.Remove(m)
94
174
}
95
175
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
176
n.notifyEvent(
104
-
actorDid,
177
+
comment.Did,
105
178
recipients,
106
-
models.NotificationTypeIssueCreated,
179
+
models.NotificationTypeIssueCommented,
107
180
entityType,
108
181
entityId,
109
182
repoId,
···
111
184
pullId,
112
185
)
113
186
n.notifyEvent(
114
-
actorDid,
115
-
sets.Collect(slices.Values(mentions)),
187
+
comment.Did,
188
+
sets.Collect(slices.Values(comment.Mentions)),
116
189
models.NotificationTypeUserMentioned,
117
190
entityType,
118
191
entityId,
···
122
195
)
123
196
}
124
197
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))
198
+
func (n *databaseNotifier) DeleteComment(ctx context.Context, comment *models.Comment) {
199
+
// no-op
200
+
}
201
+
202
+
func (n *databaseNotifier) NewIssue(ctx context.Context, issue *models.Issue, mentions []syntax.DID) {
203
+
collaborators, err := db.GetCollaborators(n.db, orm.FilterEq("repo_at", issue.Repo.RepoAt()))
127
204
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)
205
+
log.Printf("failed to fetch collaborators: %v", err)
133
206
return
134
207
}
135
-
issue := issues[0]
136
208
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
209
+
// build the recipients list
210
+
// - owner of the repo
211
+
// - collaborators in the repo
212
+
// - remove users already mentioned
142
213
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))
214
+
for _, c := range collaborators {
215
+
recipients.Insert(c.SubjectDid)
159
216
}
160
-
161
217
for _, m := range mentions {
162
218
recipients.Remove(m)
163
219
}
164
220
165
-
actorDid := syntax.DID(comment.Did)
221
+
actorDid := syntax.DID(issue.Did)
166
222
entityType := "issue"
167
223
entityId := issue.AtUri().String()
168
224
repoId := &issue.Repo.Id
···
172
228
n.notifyEvent(
173
229
actorDid,
174
230
recipients,
175
-
models.NotificationTypeIssueCommented,
231
+
models.NotificationTypeIssueCreated,
176
232
entityType,
177
233
entityId,
178
234
repoId,
···
260
316
)
261
317
}
262
318
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
319
func (n *databaseNotifier) UpdateProfile(ctx context.Context, profile *models.Profile) {
328
320
// no-op
329
321
}
+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
···
781
781
return
782
782
}
783
783
784
-
s.notifier.NewPullComment(r.Context(), &comment, mentions)
784
+
s.notifier.NewComment(r.Context(), &comment)
785
785
786
786
ownerSlashRepo := reporesolver.GetBaseRepoPath(r, f)
787
787
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