discord_messages #2
No reviewers
Labels
No Label
admins only
bug
design
duplicate
gimme feedback
good first issue
hmmmm
invalid
reference
wontfix
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: hmn/hmn#2
Loading…
Reference in New Issue
No description provided.
Delete Branch "discord_messages"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Looking good overall! I'm glad you introduced the concept of an "interned message"; I think that helps a lot with our communication about this topic at least.
I obviously left a lot of comments but I don't see anything fundamentally wrong here. I think there might still be some edge cases to consider with message editing and how that propagates to snippets and interned data, though. Maybe we should talk about that synchronously and figure out if it's even worth it to try and synchronize things like that.
@ -38,3 +41,3 @@
Use: "makesnippet [<message id>...]",
Use: "makesnippet <channel id> [<message id>...]",
Short: "Make snippets from saved Discord messages",
Long: "Make snippets from Discord messages whose content we have already saved. Useful for creating snippets from messages in non-showcase channels.",
These descriptions are no longer correct.
@ -615,2 +597,4 @@
}
// NOTE(asaf): Since any error from HandleIncomingMessage is an internal error and not a discord
// error, we only want to log it and not restart the bot. So we're not returning the error.
tab alignment :sadge:
@ -677,3 +612,1 @@
`,
result.SnippetID,
)
if interned != nil {
It may not be that important, but I usually find that when I return
nil, nil
from a function, I end up forgetting the nil check and causing bugs.Basically, I don't think pointers in Go are a great way to represent an optional value as an API decision. There are lots of reasons to return a pointer even when a non-nil value is guaranteed, and it's bitten me so many times that I have decided to just always use errors for that. That's why I've been returning
db.NotFound
from the post helper functions and such. I'm way more likely to handle things correctly that way.I'm open to discussion about how we want to handle this, but I also would probably like this to be consistent with the rest of the codebase, especially as we move toward adding more contributors. So I'd like to get your thoughts on that and come to a consensus.
@ -38,0 +36,4 @@
// NOTE(asaf): 5 seconds to ensure this runs on start, we then reset it to the correct
// interval after the first run.
backfillTicker := time.NewTicker(5 * time.Second)
If we're going to be manually resetting the ticker after every tick, it might as well not be a ticker.
I would probably change this to be a
time.Timer
, or perhaps even make a little utility calledDelayedTicker(delay time.Duration, duration time.Duration)
that we can use for cases like this. A utility might be overkill, but it also would be very self-contained, and I would trust it more because of how likely it is that we would fail totimer.Reset
on some branch.@ -47,1 +49,4 @@
case <-backfillTicker.C:
backfillTicker.Reset(backfillInterval)
// TODO(asaf): Do we need to update lastBackfillTime here? Otherwise we'll be rescraping
// from (start up time - 1 hour) every time.
Yeah this clearly seems like a bug. Seems like we should be setting this, or perhaps even storing this value in the database...let's just set it here for now though.
I will say though, it seems weird to just assume that we should always go back one backfill interval when we boot up the site. I guess it assumes that the Discord bot would be down for less than the backfill interval, which is probably true, but it seems a bit wrong to correlate the two. I would probably opt to change line 41 to something like this:
@ -109,0 +111,4 @@
continue
}
if interned == nil {
log.Error().Msg("couldn't find interned message")
Is it actually an error to not find an interned message in our db? I would expect that to be relatively likely, although I guess the live websocket connection would probably do that before a scrape.
Either way, since you don't have an
error
to attach to this, I would probably deem this as only worthy of an.Info()
or at most a.Warn()
.It's kind of an error here because we just pulled that same message out of the db.
Step 1: Get a list of interned message IDs that don't have content.
Step 2: Get the full interned message struct for each one.
It doesn't make much sense to find a message in step 1 and then not find it in step 2.
Gotcha, that makes sense - let's at least log the message ID in this case though.
@ -0,0 +38,4 @@
if !deleted && err == nil {
err = MaybeInternMessage(ctx, dbConn, msg)
}
If we think we might add more to this chain in the future, maybe we should add a setup like
but even as I type that it feels probably premature, and it would be hard to pass other arguments to and whatnot.
I think it's simple enough for now.
@ -0,0 +49,4 @@
func CleanUpShowcase(ctx context.Context, dbConn db.ConnOrTx, msg *Message) (bool, error) {
deleted := false
if msg.ChannelID == config.Config.Discord.ShowcaseChannelID {
This all might be easier with an early return, if you feel like it.
@ -0,0 +51,4 @@
deleted := false
if msg.ChannelID == config.Config.Discord.ShowcaseChannelID {
switch msg.Type {
case MessageTypeDefault, MessageTypeReply, MessageTypeApplicationCommand:
Do we want to include application commands here? Seems maybe wrong. We probably won't be able to respond inline in the channel without the response getting nuked...
No idea. I copied the cleanup code as-is. We should test this.
@ -0,0 +56,4 @@
return deleted, nil
}
hasGoodContent := true
Why does this start out true? Is this what makes application commands work?
@ -0,0 +99,4 @@
deleted := false
if msg.ChannelID == config.Config.Discord.LibraryChannelID {
switch msg.Type {
case MessageTypeDefault, MessageTypeReply, MessageTypeApplicationCommand:
Same concerns about application commands and early returns apply here.
@ -0,0 +157,4 @@
the database.
This does not create snippets or save content or do anything besides save the message itself.
*/
Comment is clearly in the wrong place - I'm guessing also outdated?
@ -0,0 +716,4 @@
if existingSnippet != nil {
// TODO(asaf): We're not handling the case where embeds were removed or modified.
// Also not handling the case where a message had both an attachment and an embed
// and the attachment was removed (leaving only the embed).
Do you want to handle that in this first pass? Seems like we should do something if the user does something that causes the message to be deleted - but would that cascade through this snippet elsewhere?
It wouldn't cause the message to be deleted. It would just leave the message with different content from what we captured when we initially created the snippet.
@ -0,0 +717,4 @@
// TODO(asaf): We're not handling the case where embeds were removed or modified.
// Also not handling the case where a message had both an attachment and an embed
// and the attachment was removed (leaving only the embed).
LinkedUserIsSnippetOwner := existingSnippet.OwnerID == interned.DiscordUser.HMNUserId
capital L! reeeeeeee
@ -0,0 +789,4 @@
}
if existingSnippet != nil {
// Update tags
What happens if the user updates a message to remove a tag? Is there any path that will un-tag the snippet? Do we want that?
What do you mean? This path deletes all project tags and only re-tags with whatever is in the message. Untagging just works.
Oh, never mind then.
@ -0,0 +793,4 @@
// Try to associate tags in the message with project tags in HMN.
// Match only tags for projects in which the current user is a collaborator.
messageTags := getDiscordTags(existingSnippet.Description)
Random thought based on usage I saw on HMN - we need to make sure the tag parser can recognize tags in situations like:
We should have an automated test for that and other situations if we don't yet already.
@ -0,0 +878,4 @@
}
// TODO(asaf): I believe this will also match https://example.com?hello=1&whatever=5
// Probably need to add word boundaries.
Ok, yet more reasons to update and verify this in all kinds of uses. Testing!!!
@ -183,0 +172,4 @@
return c.ErrorResponse(http.StatusInternalServerError, err)
}
if interned != nil {
// NOTE(asaf): Creating snippet even if the checkbox is off because the user asked us to.
We should add some text to the profile settings page informing the user of this.
I configured the mailer for gitea. Is this on?
Pull request closed