tickets: Make ticket closing prompt more granular #1808

Wolveric · 2024-12-30T13:13:59Z

A server manager recently expressed their concerns, with YAGPDB
unconditionally suggesting it was downloading attachments
sent in tickets. They left this disabled for the privacy of members,
and felt the prompt may scare members, as it contradicted
their stated behaviour.

This PR updates the ticket closing function,
to use a more granular prompt, based on whether
it is creating a transcript of the ticket and/or downloading attachments.

It has been highlighted by one uses, that the lack of granularity from the closing message, concerns them. Consequently, this commit updates the ticket closing function, to use a variable for the closing message, as opposed to the const. msg. used currently. This acts largely as a prelude to the subsequent commits, which will add more to the message, conditionally, based on the logs YAGPDB will create.

This commit adds the initial condition for updating the closing message, if logs will be created. For both conditions, we want to add a prompt that closing the ticket may take a while, if logs are being created. So this change, creates a builder from the initial message, and adds this reminder, if logs would be created.

Wolveric · 2024-12-30T13:32:23Z

Ok, presumably that constant was used elsewhere... will submit a fix promptly...

Wolveric · 2024-12-30T13:50:13Z

Gonna convert this to a draft, because the interactions with slash commands may make it more complicated.

Due to the design of the ticket button handling functions, closing tickets resulted in the ticket closing prompt being doubled up on with an ephemeral response. For this reason, these prompts served little purpose, as a prompt was already created as a normal message, as part of the `tickets.closeTicket` function. Consquently, this effectively just introduced a dependency on the `tickets.closingTicketMsg` constant, that this series of commits, served to remove.

SoggySaussages · 2024-12-30T18:48:58Z

With commit 333d879, would you please test this with the
“Close with reason” button and let me know what you find?

jo3-l · 2024-12-30T22:01:06Z

tickets/tickets_bot.go

+	// We only need to build up a more detailed closing msg.
+	// if we're creating logs.


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

tickets/tickets_bot.go

jo3-l · 2024-12-30T22:07:33Z

tickets/tickets_bot.go

+		if conf.TicketsUseTXTTranscripts {
+			closingMsgBuilder.WriteString("\nCreating logs.")
+		}
+
+		if conf.DownloadAttachments {
+			closingMsgBuilder.WriteString("\nDownloading attachments.")
+		}


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?

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...

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.

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,

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.

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...

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

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...

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

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

Co-authored-by: Joseph Liu <[email protected]>

SoggySaussages · 2024-12-31T08:08:01Z

333d879 will need to be reverted to maintain current functionality.

Wolveric · 2024-12-31T13:13:03Z

333d879 will need to be reverted to maintain current functionality.

I don't think I will revert that directly, as it still has the issue of depending on the existing constant. An edited version with a more generic response should be reasonable though...

SoggySaussages · 2024-12-31T13:39:45Z

333d879 will need to be reverted to maintain current functionality.

I don't think I will revert that directly, as it still has the issue of depending on the existing constant. An edited version with a more generic response should be reasonable though...

What do you think about taking the builder you have currently and
separating into a function that can be called in all three places?

Wolveric · 2024-12-31T13:44:22Z

333d879 will need to be reverted to maintain current functionality.

I don't think I will revert that directly, as it still has the issue of depending on the existing constant. An edited version with a more generic response should be reasonable though...

What do you think about taking the builder you have currently and separating into a function that can be called in all three places?

That would result in the work being done up to twice, including when it's not necessary.

tickets.closeTicket can exit before attempting to log & delete the ticket, so it only makes sense to construct that prompt conditionally.

SoggySaussages · 2024-12-31T13:53:19Z

333d879 will need to be reverted to maintain current functionality.

I don't think I will revert that directly, as it still has the issue of depending on the existing constant. An edited version with a more generic response should be reasonable though...

What do you think about taking the builder you have currently and separating into a function that can be called in all three places?

That would result in the work being done up to twice, including when it's not necessary.

tickets.closeTicket can exit before attempting to log & delete the ticket, so it only makes sense to construct that prompt conditionally.

I’m not saying don’t construct it conditionally, I’m saying put those
conditions into a function and execute it where needed.

Wolveric · 2024-12-31T13:56:41Z

333d879 will need to be reverted to maintain current functionality.

I don't think I will revert that directly, as it still has the issue of depending on the existing constant. An edited version with a more generic response should be reasonable though...

What do you think about taking the builder you have currently and separating into a function that can be called in all three places?

That would result in the work being done up to twice, including when it's not necessary.
tickets.closeTicket can exit before attempting to log & delete the ticket, so it only makes sense to construct that prompt conditionally.

I’m not saying don’t construct it conditionally, I’m saying put those conditions into a function and execute it where needed.

The issue is, that message is only needed in the condition the ticket can be successfully closed.
Those ephemeral responses aren't based on that condition.

SoggySaussages · 2024-12-31T13:59:37Z

333d879 will need to be reverted to maintain current functionality.

I don't think I will revert that directly, as it still has the issue of depending on the existing constant. An edited version with a more generic response should be reasonable though...

What do you think about taking the builder you have currently and separating into a function that can be called in all three places?

That would result in the work being done up to twice, including when it's not necessary.
tickets.closeTicket can exit before attempting to log & delete the ticket, so it only makes sense to construct that prompt conditionally.

I’m not saying don’t construct it conditionally, I’m saying put those conditions into a function and execute it where needed.

The issue is, that message is only needed in the condition the ticket can be successfully closed. Those ephemeral responses aren't based on that condition.

it is still possible for that ticket to fail to close after sending that
message, therefore that message is never a guarantee that the ticket
is about to successfully close.

SoggySaussages · 2024-12-31T14:05:30Z

333d879 will need to be reverted to maintain current functionality.

I don't think I will revert that directly, as it still has the issue of depending on the existing constant. An edited version with a more generic response should be reasonable though...

What do you think about taking the builder you have currently and separating into a function that can be called in all three places?

That would result in the work being done up to twice, including when it's not necessary.
tickets.closeTicket can exit before attempting to log & delete the ticket, so it only makes sense to construct that prompt conditionally.

I’m not saying don’t construct it conditionally, I’m saying put those conditions into a function and execute it where needed.

The issue is, that message is only needed in the condition the ticket can be successfully closed. Those ephemeral responses aren't based on that condition.

it is still possible for that ticket to fail to close after sending that message, therefore that message is never a guarantee that the ticket is about to successfully close.

It’s arguably significantly more likely that the ticket will fail to create
and/or send message logs or fail to delete the channel than someone
will try to spam closing a ticket. Also, in the case that the ticket is in
fact already being closed the ephemeral response would not be
making a false claim.

Wolveric · 2024-12-31T14:05:36Z

333d879 will need to be reverted to maintain current functionality.

I don't think I will revert that directly, as it still has the issue of depending on the existing constant. An edited version with a more generic response should be reasonable though...

What do you think about taking the builder you have currently and separating into a function that can be called in all three places?

That would result in the work being done up to twice, including when it's not necessary.
tickets.closeTicket can exit before attempting to log & delete the ticket, so it only makes sense to construct that prompt conditionally.

I’m not saying don’t construct it conditionally, I’m saying put those conditions into a function and execute it where needed.

The issue is, that message is only needed in the condition the ticket can be successfully closed. Those ephemeral responses aren't based on that condition.

it is still possible for that ticket to fail to close after sending that
message, therefore that message is never a guarantee that the ticket
is about to successfully close.

Ok.

Regardless, it doesn't make sense to construct it before testing those conditions, then again if those conditions do succeed. It makes more sense to do it in tickets.closeTicket, and just have a generic response acknowledging the interaction worked.

SoggySaussages · 2024-12-31T14:10:41Z

333d879 will need to be reverted to maintain current functionality.

I don't think I will revert that directly, as it still has the issue of depending on the existing constant. An edited version with a more generic response should be reasonable though...

What do you think about taking the builder you have currently and separating into a function that can be called in all three places?

That would result in the work being done up to twice, including when it's not necessary.
tickets.closeTicket can exit before attempting to log & delete the ticket, so it only makes sense to construct that prompt conditionally.

I’m not saying don’t construct it conditionally, I’m saying put those conditions into a function and execute it where needed.

The issue is, that message is only needed in the condition the ticket can be successfully closed. Those ephemeral responses aren't based on that condition.

it is still possible for that ticket to fail to close after sending that
message, therefore that message is never a guarantee that the ticket
is about to successfully close.

Ok.

Regardless, it doesn't make sense to construct it before testing those conditions, then again if those conditions do succeed. It makes more sense to do it in tickets.closeTicket, and just have a generic response acknowledging the interaction worked.

The “conditions” you’re referring to are just <ticket is already being
closed>, right? In either case, the ephemeral message would not be
making a false claim in this case.

I still maintain that in most instances people are going to be seeing
these two messages pop up for about half a second before the
channel is deleted. If you give two different messages, you’re just
going to increase the amount of text that needs to be read in that
time.

Wolveric · 2024-12-31T15:09:10Z

We should outline the potential approaches here, from the user and technical perspective:

Remove he ephemeral response entirely (current change):
- Technical points:
  - Removes any extra work outright
  - Discord will get in a huff for improperly handling the interaction
- User points:
  - If user sees the interaction complete before the channel is deleted, Discord will be shown them an error that the interaction failed to response, which is confusing if YAGPDB handles closing the ticket successfully
Add a method for constructing the closing prompt:
- Technical points:
  - Discord is happy, because we responded to the interaction
  - We're building the response in both the button handlers, and tickets.closeTicket
- User points:
  - If the user sees the interaction complete, they won't see the error from Discord about the interaction failing to respond
  - If the user sees the interaction complete, they will see the closing message parroted soon afterwards, which is now redundant info to them, and may be confusing
Use a generic response for button interactions:
- Technical points:
  - Discord is happy, because we responded to the interaction
  - We're spending very little time on the response, if we use a constant string
- User points:
  - If the user sees the interaction complete, they won't see any errors
  - If they see the generic response, they will know YAGPDB is trying to close the ticket
  - If they see the follow-up message from tickets.closeTicket, they can see what actions YAGPDB is trying to do.

SoggySaussages · 2024-12-31T15:20:41Z

~~Your first user point for option two is inaccurate,~~ and I’m not sure
what your user point for option one is. I don’t see how seeing two
of the exact same message can be confusing as to what’s going on,
If anything it just reinforces with certainty what’s going on, no?

Edit: crossed out issues addressed by Willow’s edits

Wolveric · 2024-12-31T15:31:35Z

Confusion might be overstating the response to the message being doubled up, but they may question, "Why did the bot post that message twice...??"

If YAGPDB was already attempting to close the ticket though, a message stating YAGPDB is attempting to close the ticket, then another saying it's already closing the ticket, almost certainly would be.

Alternatively, a generic response of, "Attempting to close ticket." makes a lot more sense followed up with, "Already working on closing this ticket, please wait..."

SoggySaussages · 2024-12-31T15:36:20Z

I think there is a difference between questioning something and
being confused. I don’t see how a message stating that the bot is
attempting to close the ticket followed up by a message that the
ticket is already being closed could be confusing. Both messages
would be 100% accurate. Notably, the ticket closing message does
not mention _ initializing_ the closing of the ticket, only that it is in
present tense occurring.

Wolveric · 2024-12-31T15:38:56Z

I think using "Attempting to close ticket" would be the preferable response for the ephemeral message. It could fit both with the response for YAGPDB already closing the ticket, and when its starting a new closing process.

SoggySaussages · 2024-12-31T15:40:11Z

Imo, given the choice between having a user question something,
and having a user be frustrated that they don’t have any time to
read two separate messages before the entire channel disappears,
we should choose the former. My guess is that most users will
understand that the second message is for the benefit of the entire
channel, seeing that the first message has the only you can see
this message note beneath it and the second message doesn’t.

Wolveric · 2024-12-31T16:29:46Z

I think something this PR has highlighted, is that there's too much functionality wrapped up in tickets.closeTicket, so I'm going to convert it back to a draft for the time being, and look at redoing it as a rework of the ticket system in future.

Wolveric added 4 commits December 30, 2024 12:37

tickets: Add log creation reminder to closing msg.

04452e9

tickets: Add attachment download closing prompt

f6938d7

Wolveric marked this pull request as draft December 30, 2024 13:49

Wolveric marked this pull request as ready for review December 30, 2024 15:35

jo3-l reviewed Dec 30, 2024

View reviewed changes

tickets: Remove unnecessary comma from closing prompt

f81416f

Co-authored-by: Joseph Liu <[email protected]>

Wolveric marked this pull request as draft December 31, 2024 16:29

ashishjh-bst force-pushed the dev branch from 8c996a4 to 8fdb33c Compare December 31, 2024 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tickets: Make ticket closing prompt more granular #1808

tickets: Make ticket closing prompt more granular #1808

Wolveric commented Dec 30, 2024

Wolveric commented Dec 30, 2024

Wolveric commented Dec 30, 2024

SoggySaussages commented Dec 30, 2024

jo3-l Dec 30, 2024

jo3-l Dec 30, 2024

Wolveric Dec 30, 2024

jo3-l Dec 30, 2024

Wolveric Dec 30, 2024

jo3-l Dec 30, 2024

Wolveric Dec 31, 2024

SoggySaussages Dec 31, 2024

Wolveric Dec 31, 2024

SoggySaussages Dec 31, 2024

Wolveric Dec 31, 2024

SoggySaussages commented Dec 31, 2024

Wolveric commented Dec 31, 2024

SoggySaussages commented Dec 31, 2024

Wolveric commented Dec 31, 2024

SoggySaussages commented Dec 31, 2024

Wolveric commented Dec 31, 2024

SoggySaussages commented Dec 31, 2024

SoggySaussages commented Dec 31, 2024

Wolveric commented Dec 31, 2024

SoggySaussages commented Dec 31, 2024 •

edited

Loading

Wolveric commented Dec 31, 2024 •

edited

Loading

SoggySaussages commented Dec 31, 2024 •

edited

Loading

Wolveric commented Dec 31, 2024

SoggySaussages commented Dec 31, 2024 •

edited

Loading

Wolveric commented Dec 31, 2024

SoggySaussages commented Dec 31, 2024

Wolveric commented Dec 31, 2024

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

	// 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.

tickets: Make ticket closing prompt more granular #1808

Are you sure you want to change the base?

tickets: Make ticket closing prompt more granular #1808

Conversation

Wolveric commented Dec 30, 2024

Wolveric commented Dec 30, 2024

Wolveric commented Dec 30, 2024

SoggySaussages commented Dec 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SoggySaussages commented Dec 31, 2024

Wolveric commented Dec 31, 2024

SoggySaussages commented Dec 31, 2024

Wolveric commented Dec 31, 2024

SoggySaussages commented Dec 31, 2024

Wolveric commented Dec 31, 2024

SoggySaussages commented Dec 31, 2024

SoggySaussages commented Dec 31, 2024

Wolveric commented Dec 31, 2024

SoggySaussages commented Dec 31, 2024 • edited Loading

Wolveric commented Dec 31, 2024 • edited Loading

SoggySaussages commented Dec 31, 2024 • edited Loading

Wolveric commented Dec 31, 2024

SoggySaussages commented Dec 31, 2024 • edited Loading

Wolveric commented Dec 31, 2024

SoggySaussages commented Dec 31, 2024

Wolveric commented Dec 31, 2024

SoggySaussages commented Dec 31, 2024 •

edited

Loading

Wolveric commented Dec 31, 2024 •

edited

Loading

SoggySaussages commented Dec 31, 2024 •

edited

Loading

SoggySaussages commented Dec 31, 2024 •

edited

Loading