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))
+105
-110
appview/notify/db/db.go
+105
-110
appview/notify/db/db.go
···
73
73
// no-op
74
74
}
75
75
76
-
func (n *databaseNotifier) NewIssue(ctx context.Context, issue *models.Issue, mentions []syntax.DID) {
76
+
func (n *databaseNotifier) NewComment(ctx context.Context, comment *models.Comment) {
77
+
var (
78
+
recipients []syntax.DID
79
+
entityType string
80
+
entityId string
81
+
repoId *int64
82
+
issueId *int64
83
+
pullId *int64
84
+
)
77
85
78
-
// build the recipients list
79
-
// - owner of the repo
80
-
// - collaborators in the repo
81
-
var recipients []syntax.DID
82
-
recipients = append(recipients, syntax.DID(issue.Repo.Did))
83
-
collaborators, err := db.GetCollaborators(n.db, db.FilterEq("repo_at", issue.Repo.RepoAt()))
86
+
subjectDid, err := comment.Subject.Authority().AsDID()
84
87
if err != nil {
85
-
log.Printf("failed to fetch collaborators: %v", err)
88
+
log.Printf("NewComment: expected did based at-uri for comment.subject")
86
89
return
87
90
}
88
-
for _, c := range collaborators {
89
-
recipients = append(recipients, c.SubjectDid)
90
-
}
91
+
switch comment.Subject.Collection() {
92
+
case tangled.RepoIssueNSID:
93
+
issues, err := db.GetIssues(
94
+
n.db,
95
+
db.FilterEq("did", subjectDid),
96
+
db.FilterEq("rkey", comment.Subject.RecordKey()),
97
+
)
98
+
if err != nil {
99
+
log.Printf("NewComment: failed to get issues: %v", err)
100
+
return
101
+
}
102
+
if len(issues) == 0 {
103
+
log.Printf("NewComment: no issue found for %s", comment.Subject)
104
+
return
105
+
}
106
+
issue := issues[0]
107
+
108
+
recipients = append(recipients, syntax.DID(issue.Repo.Did))
109
+
if comment.IsReply() {
110
+
// if this comment is a reply, then notify everybody in that thread
111
+
parentAtUri := *comment.ReplyTo
112
+
allThreads := issue.CommentList()
113
+
114
+
// find the parent thread, and add all DIDs from here to the recipient list
115
+
for _, t := range allThreads {
116
+
if t.Self.AtUri() == parentAtUri {
117
+
recipients = append(recipients, t.Participants()...)
118
+
}
119
+
}
120
+
} else {
121
+
// not a reply, notify just the issue author
122
+
recipients = append(recipients, syntax.DID(issue.Did))
123
+
}
91
124
92
-
actorDid := syntax.DID(issue.Did)
93
-
entityType := "issue"
94
-
entityId := issue.AtUri().String()
95
-
repoId := &issue.Repo.Id
96
-
issueId := &issue.Id
97
-
var pullId *int64
125
+
entityType = "issue"
126
+
entityId = issue.AtUri().String()
127
+
repoId = &issue.Repo.Id
128
+
issueId = &issue.Id
129
+
case tangled.RepoPullNSID:
130
+
pulls, err := db.GetPullsWithLimit(
131
+
n.db,
132
+
1,
133
+
db.FilterEq("owner_did", subjectDid),
134
+
db.FilterEq("rkey", comment.Subject.RecordKey()),
135
+
)
136
+
if err != nil {
137
+
log.Printf("NewComment: failed to get pulls: %v", err)
138
+
return
139
+
}
140
+
if len(pulls) == 0 {
141
+
log.Printf("NewComment: no pull found for %s", comment.Subject)
142
+
return
143
+
}
144
+
pull := pulls[0]
145
+
146
+
pull.Repo, err = db.GetRepo(n.db, db.FilterEq("at_uri", pull.RepoAt))
147
+
if err != nil {
148
+
log.Printf("NewComment: failed to get repos: %v", err)
149
+
return
150
+
}
151
+
152
+
recipients = append(recipients, syntax.DID(pull.Repo.Did))
153
+
for _, p := range pull.Participants() {
154
+
recipients = append(recipients, syntax.DID(p))
155
+
}
156
+
157
+
entityType = "pull"
158
+
entityId = pull.AtUri().String()
159
+
repoId = &pull.Repo.Id
160
+
p := int64(pull.ID)
161
+
pullId = &p
162
+
default:
163
+
return // no-op
164
+
}
98
165
99
166
n.notifyEvent(
100
-
actorDid,
167
+
comment.Did,
101
168
recipients,
102
-
models.NotificationTypeIssueCreated,
169
+
models.NotificationTypeIssueCommented,
103
170
entityType,
104
171
entityId,
105
172
repoId,
···
107
174
pullId,
108
175
)
109
176
n.notifyEvent(
110
-
actorDid,
111
-
mentions,
177
+
comment.Did,
178
+
comment.Mentions,
112
179
models.NotificationTypeUserMentioned,
113
180
entityType,
114
181
entityId,
···
118
185
)
119
186
}
120
187
121
-
func (n *databaseNotifier) NewIssueComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) {
122
-
issues, err := db.GetIssues(n.db, db.FilterEq("at_uri", comment.Subject))
123
-
if err != nil {
124
-
log.Printf("NewIssueComment: failed to get issues: %v", err)
125
-
return
126
-
}
127
-
if len(issues) == 0 {
128
-
log.Printf("NewIssueComment: no issue found for %s", comment.Subject)
129
-
return
130
-
}
131
-
issue := issues[0]
188
+
func (n *databaseNotifier) DeleteComment(ctx context.Context, comment *models.Comment) {
189
+
// no-op
190
+
}
191
+
192
+
func (n *databaseNotifier) NewIssue(ctx context.Context, issue *models.Issue, mentions []syntax.DID) {
132
193
194
+
// build the recipients list
195
+
// - owner of the repo
196
+
// - collaborators in the repo
133
197
var recipients []syntax.DID
134
198
recipients = append(recipients, syntax.DID(issue.Repo.Did))
135
-
136
-
if comment.IsReply() {
137
-
// if this comment is a reply, then notify everybody in that thread
138
-
parentAtUri := *comment.ReplyTo
139
-
allThreads := issue.CommentList()
140
-
141
-
// find the parent thread, and add all DIDs from here to the recipient list
142
-
for _, t := range allThreads {
143
-
if t.Self.AtUri() == parentAtUri {
144
-
recipients = append(recipients, t.Participants()...)
145
-
}
146
-
}
147
-
} else {
148
-
// not a reply, notify just the issue author
149
-
recipients = append(recipients, syntax.DID(issue.Did))
199
+
collaborators, err := db.GetCollaborators(n.db, db.FilterEq("repo_at", issue.Repo.RepoAt()))
200
+
if err != nil {
201
+
log.Printf("failed to fetch collaborators: %v", err)
202
+
return
203
+
}
204
+
for _, c := range collaborators {
205
+
recipients = append(recipients, c.SubjectDid)
150
206
}
151
207
152
-
actorDid := syntax.DID(comment.Did)
208
+
actorDid := syntax.DID(issue.Did)
153
209
entityType := "issue"
154
210
entityId := issue.AtUri().String()
155
211
repoId := &issue.Repo.Id
···
159
215
n.notifyEvent(
160
216
actorDid,
161
217
recipients,
162
-
models.NotificationTypeIssueCommented,
218
+
models.NotificationTypeIssueCreated,
163
219
entityType,
164
220
entityId,
165
221
repoId,
···
248
304
)
249
305
}
250
306
251
-
func (n *databaseNotifier) NewPullComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) {
252
-
pulls, err := db.GetPulls(n.db,
253
-
db.FilterEq("owner_did", comment.Subject.Authority()),
254
-
db.FilterEq("rkey", comment.Subject.RecordKey()),
255
-
)
256
-
if err != nil {
257
-
log.Printf("NewPullComment: failed to get pulls: %v", err)
258
-
return
259
-
}
260
-
if len(pulls) == 0 {
261
-
log.Printf("NewPullComment: no pull found for %s", comment.Subject)
262
-
return
263
-
}
264
-
pull := pulls[0]
265
-
266
-
repo, err := db.GetRepo(n.db, db.FilterEq("at_uri", pull.RepoAt))
267
-
if err != nil {
268
-
log.Printf("NewPullComment: failed to get repos: %v", err)
269
-
return
270
-
}
271
-
272
-
// build up the recipients list:
273
-
// - repo owner
274
-
// - all pull participants
275
-
var recipients []syntax.DID
276
-
recipients = append(recipients, syntax.DID(repo.Did))
277
-
for _, p := range pull.Participants() {
278
-
recipients = append(recipients, syntax.DID(p))
279
-
}
280
-
281
-
actorDid := comment.Did
282
-
eventType := models.NotificationTypePullCommented
283
-
entityType := "pull"
284
-
entityId := pull.AtUri().String()
285
-
repoId := &repo.Id
286
-
var issueId *int64
287
-
p := int64(pull.ID)
288
-
pullId := &p
289
-
290
-
n.notifyEvent(
291
-
actorDid,
292
-
recipients,
293
-
eventType,
294
-
entityType,
295
-
entityId,
296
-
repoId,
297
-
issueId,
298
-
pullId,
299
-
)
300
-
n.notifyEvent(
301
-
actorDid,
302
-
mentions,
303
-
models.NotificationTypeUserMentioned,
304
-
entityType,
305
-
entityId,
306
-
repoId,
307
-
issueId,
308
-
pullId,
309
-
)
310
-
}
311
-
312
307
func (n *databaseNotifier) UpdateProfile(ctx context.Context, profile *models.Profile) {
313
308
// no-op
314
309
}
+8
-8
appview/notify/merged_notifier.go
+8
-8
appview/notify/merged_notifier.go
···
54
54
m.fanout("DeleteStar", ctx, star)
55
55
}
56
56
57
-
func (m *mergedNotifier) NewIssue(ctx context.Context, issue *models.Issue, mentions []syntax.DID) {
58
-
m.fanout("NewIssue", ctx, issue, mentions)
57
+
func (m *mergedNotifier) NewComment(ctx context.Context, comment *models.Comment) {
58
+
m.fanout("NewComment", ctx, comment)
59
+
}
60
+
61
+
func (m *mergedNotifier) DeleteComment(ctx context.Context, comment *models.Comment) {
62
+
m.fanout("DeleteComment", ctx, comment)
59
63
}
60
64
61
-
func (m *mergedNotifier) NewIssueComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) {
62
-
m.fanout("NewIssueComment", ctx, comment, mentions)
65
+
func (m *mergedNotifier) NewIssue(ctx context.Context, issue *models.Issue, mentions []syntax.DID) {
66
+
m.fanout("NewIssue", ctx, issue, mentions)
63
67
}
64
68
65
69
func (m *mergedNotifier) NewIssueState(ctx context.Context, actor syntax.DID, issue *models.Issue) {
···
82
86
m.fanout("NewPull", ctx, pull)
83
87
}
84
88
85
-
func (m *mergedNotifier) NewPullComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) {
86
-
m.fanout("NewPullComment", ctx, comment, mentions)
87
-
}
88
-
89
89
func (m *mergedNotifier) NewPullState(ctx context.Context, actor syntax.DID, pull *models.Pull) {
90
90
m.fanout("NewPullState", ctx, actor, pull)
91
91
}
+6
-6
appview/notify/notifier.go
+6
-6
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
···
52
54
func (m *BaseNotifier) DeleteFollow(ctx context.Context, follow *models.Follow) {}
53
55
54
56
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
-
}
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
···
793
793
return
794
794
}
795
795
796
-
s.notifier.NewPullComment(r.Context(), &comment, mentions)
796
+
s.notifier.NewComment(r.Context(), &comment)
797
797
798
798
ownerSlashRepo := reporesolver.GetBaseRepoPath(r, f)
799
799
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