Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tickets: Make ticket closing prompt more granular #1808

Draft
wants to merge 6 commits into
base: dev
Choose a base branch
from
Draft

tickets: Make ticket closing prompt more granular #1808

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
04687c9
tickets: Switch to variable for closing message
Wolveric Dec 30, 2024
2535de2
tickets: Replace closing msg. when creating logs
Wolveric Dec 30, 2024
04452e9
tickets: Add log creation reminder to closing msg.
Wolveric Dec 30, 2024
f6938d7
tickets: Add attachment download closing prompt
Wolveric Dec 30, 2024
333d879
tickets: Drop ticket closing response from buttons
Wolveric Dec 30, 2024
f81416f
tickets: Remove unnecessary comma from closing prompt
Wolveric Dec 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 21 additions & 17 deletions tickets/tickets_bot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,8 +193,6 @@ func openTicket(ctx context.Context, gs *dstate.GuildSet, ms *dstate.MemberState
var closingTickets = make(map[int64]bool)
var closingTicketsLock sync.Mutex

const closingTicketMsg = "Closing ticket, creating logs, downloading attachments and so on.\nThis may take a while if the ticket is big."

func closeTicket(gs *dstate.GuildSet, currentTicket *Ticket, ticketCS *dstate.ChannelState, conf *models.TicketConfig, member *discordgo.User, reason string, ctx context.Context) (string, error) {
// protect again'st calling close multiple times at the sime time
closingTicketsLock.Lock()
Expand All @@ -210,8 +208,28 @@ func closeTicket(gs *dstate.GuildSet, currentTicket *Ticket, ticketCS *dstate.Ch
closingTicketsLock.Unlock()
}()

closingMsg := "Closing ticket."

// We only need to build up a more detailed closing msg.
// if we're creating logs.
Comment on lines +213 to +214
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// We only need to build up a more detailed closing msg.
// if we're creating logs.
// We only need to generate a more detailed closing message if we're creating logs.

the full stop is misplaced as is

if conf.TicketsUseTXTTranscripts || conf.DownloadAttachments {
var closingMsgBuilder strings.Builder
closingMsgBuilder.WriteString(closingMsg)

if conf.TicketsUseTXTTranscripts {
closingMsgBuilder.WriteString("\nCreating logs.")
}

if conf.DownloadAttachments {
closingMsgBuilder.WriteString("\nDownloading attachments.")
}
Comment on lines +219 to +225
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The syntax of the generated message is a bit awkward:

Closing ticket.
Creating logs.
Downloading attachments.
This may take a while if the ticket is long.

I wonder if we can arrange for the message to be more similar to the original. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt the "and so on" read weirdly, as those are the only two actions it's doing, and the comma version reads weirdly without it.

I could have tried to do a version conditionally inserting "and", but this seemed like the least complex approach...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. For completeness, an alternative that is not too complicated is a switch enumerating all the cases, which is not too difficult here:

transcripts, attachments := conf.TicketsUseTXTTranscripts, conf.DownloadAttachments
var closingMsg string
switch {
case transcripts && attachments:
	closingMsg = "Closing ticket, creating logs, and downloading attachments. [...]"
case transcripts:
	...
...
}

but it is arguable whether this is a real improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the standpoint of minimising allocations, it's almost certainly an improvement. I was thinking of making a change of growing the allocation before building the string, but choosing a single option is almost certainly better,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is definitely more performant, but this code is not on the hot path and we do not make much effort to minimize allocations in general so that concern may be misplaced. The main questions, I think, are quality of the output message, code readability, and extensibility, and it's a bit less clear whether the above suggestion is a meaningful improvement on all three fronts to be worthwhile--I don't have a strong opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively: The initial line could be added as three statements;
Closing ticket. Creating logs. Downloading attachments.

Having looked at my solution, using a switch statement, and string builder, it doesn't look better to me...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about a slice of actions joined by commas and switched to title case
at the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't particularly see how that helps. It would create a single source for changing any separators, yes, but would largely result in checking the same conditions anyway...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just joins it all into one sentence instead of three separate ones.
maybe I’m not understanding what the current issue is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial quibble, was about the fact I changed the response to be separated onto different lines, and the message reading somewhat strangely as a result. I kind of agree with that in hindsight, and I think the three statements could be on the same line.

So basically, just swapping \n for


closingMsgBuilder.WriteString("\nThis may take a while, if the ticket is long.")
Wolveric marked this conversation as resolved.
Show resolved Hide resolved
closingMsg = closingMsgBuilder.String()
}

// send a heads up that this can take a while
common.BotSession.ChannelMessageSend(currentTicket.Ticket.ChannelID, closingTicketMsg)
common.BotSession.ChannelMessageSend(currentTicket.Ticket.ChannelID, closingMsg)

currentTicket.Ticket.ClosedAt.Time = time.Now()
currentTicket.Ticket.ClosedAt.Valid = true
Expand Down Expand Up @@ -298,13 +316,6 @@ func handleButton(evt *eventsystem.EventData, ic *discordgo.InteractionCreate, m
}
}

common.BotSession.CreateInteractionResponse(ic.ID, ic.Token, &discordgo.InteractionResponse{
Type: discordgo.InteractionResponseChannelMessageWithSource,
Data: &discordgo.InteractionResponseData{
Content: closingTicketMsg,
Flags: discordgo.MessageFlagsEphemeral,
},
})
response.Data.Content, err = closeTicket(evt.GS, currentTicket, currentChannel, conf, member.User, "", evt.Context())
case "close-reason":
response = &discordgo.InteractionResponse{
Expand Down Expand Up @@ -360,13 +371,6 @@ func handleModal(evt *eventsystem.EventData, ic *discordgo.InteractionCreate, me
}
}

common.BotSession.CreateInteractionResponse(ic.ID, ic.Token, &discordgo.InteractionResponse{
Type: discordgo.InteractionResponseChannelMessageWithSource,
Data: &discordgo.InteractionResponseData{
Content: closingTicketMsg,
Flags: discordgo.MessageFlagsEphemeral,
},
})
response.Data.Content, err = closeTicket(evt.GS, currentTicket, currentChannel, conf, member.User, value, evt.Context())
}

Expand Down