Addressing #2336 to default quotes to streamer name if no @ is provided #2753

Wrongtown · 2024-08-27T16:08:12Z

Description of the Change

When !quote add is invoked this change checks for an @ in the originator arg of the command, and if it doesn't find one we assume that the quote should be attributed to the streamer and splices in @+channelData.displayName. If there is an @ then that string is used for attribution as normal.

This is my first PR here so please do let me know if there's anything you'd have preferred I do differently.

Example

!quote add I'm so disappointed in you.

The above would attribute the quote "I'm so disappointed in you." to whoever the streamer is.

Counter-example

!quote add @zunderscore Do I have to do everything around here?

The above would attribute the quote "Do I have to do everything around here?" to twitch user @zunderscore

Update after review comments

Added opt-in Attribute new quote to streamer if nobody is explicitly tagged with @ option to Quote Management command.
Used string template instead of concatenation for streamer username.

Applicable Issues

#2336

Testing

Verified that when no @ attribution is included in an invocation of the !quote add command that the streamer username is attributed.
Verified that when an @ attribution is included in an invocation of the !quote add command that the @ attribution is still assigned as expected.
Verified that other commands in the !quote family do not appear to have somehow been caught up in the blast radius of my change.

After review comments

Verified that !quote add can be invoked as usual if user has not opted in.
Verified that the default to streamer functionality for !quote add works as expected if user does opt in.

Screenshots

v5.63.2

… @ is provided

CKY- · 2024-08-27T16:20:31Z

i have a question

!quote add zunderscore Do I have to do everything around here?

what about this use case

ebiggz

Hey @Wrongtown!

Thanks for taking this on and for contributing to Firebot! I think your approach here is sound and clean.

The only thing I think we should add to this is make it configurable by the user. This is because some folks may not always use the @ symbol when referencing someone to quote. Difficulty with that is there's no good way for us to programmatically determine if the first word is a username, nickname or otherwise. So making it a configurable option (defaulted to off so current behavior doesn't suddenly change on users) puts the nexus on the Streamer to decide how they prefer it functions.

System Commands support "options" which get rendered in the UI when the user edits the command. Take a look at the useTTS option currently in use for the Quotes sys command as a reference for how to define and reference options in the onTriggerEvent. It will mainly just entail adding the new option and then referencing it in your if statement, perhaps something like:

if (commandOptions.defaultToStreamer && !args[1].includes("@")) {

Also for future reference if you end up submitting more PRs, please point them at the v5 branch. The v5 branch represents our active development branch whereas master represents what is currently released to users. 👍🏻

Let us know if you have any Qs, either here or in the #dev channel of our Discord!

ebiggz · 2024-08-27T16:22:30Z

src/backend/chat/commands/builtin/quotes.ts

+
+                    //If no @ is included in the originator arg, set to @streamerName and the rest is the quote
+                    if (!args[1].includes("@")) { 
+                        args.splice(1,0,"@"+channelData.displayName) 


Super minor thing, we generally prefer string templates over string concatenation:

`@${channelData.displayName}`

Addressed in 1f79440

Wrongtown · 2024-08-27T16:45:08Z

The only thing I think we should add to this is make it configurable by the user. This is because some folks may not always use the @ symbol when referencing someone to quote. Difficulty with that is there's no good way for us to programmatically determine if the first word is a username, nickname or otherwise. So making it a configurable option (defaulted to off so current behavior doesn't suddenly change on users) puts the nexus on the Streamer to decide how they prefer it functions.

Makes sense, will look into that. Thanks for the prompt review!

Also for future reference if you end up submitting more PRs, please point them at the v5 branch. The v5 branch represents our active development branch whereas master represents what is currently released to users. 👍🏻

Dangit, I knew I'd forgotten something 🤦‍♂️

…tting

Wrongtown · 2024-08-27T16:58:20Z

i have a question

!quote add zunderscore Do I have to do everything around here?

what about this use case

You're quite right, if they intend to provide a username but don't include the @ it would attribute the quote "zunderscore Do I have to do everything around here?" to the streamer.

So as requested above, I'll look into adding a setting to opt-in on this change. If the streamer does opt in, the above would still happen, of course.

…ttps://github.com/Wrongtown/Firebot into feature/2336-default-to-streamer-for-quotes-setting

Oceanity · 2024-08-30T23:25:08Z

src/backend/chat/commands/builtin/quotes.ts

@@ -278,6 +285,13 @@ export const QuotesManagementSystemCommand: SystemCommand<{
                    const channelData = await TwitchApi.channels.getChannelInformation();

                    const currentGameName = channelData && channelData.gameName ? channelData.gameName : "Unknown game";
+
+                    // //If no @ is included in the originator arg, set to @streamerName and the rest is the quote


This check should be over Line 280, as !quote add AHHHHHHHHHH would fail out before it reaches this point (only 2 args)

Good point, silly me none of my testing was a single word quote.

I've addressed this in 595b1c4.

Kinda a nitpick, but could you move the following so they'll still be under the if statements?

const channelData = await TwitchApi.channels.getChannelInformation(); const currentGameName = channelData && channelData.gameName ? channelData.gameName : "Unknown game";

If this is above, whether or not the command was valid, it's going to make an API call to get channel information, which will cause unnecessary delay if the command was used incorrectly.

Other than that, looks good to me! :D

Maybe I'm missing something more elegant here? The new section (quoted below) depends on already having fetched channelData.displayname, so the API call can't come after the if statements as far as I can see?

if (commandOptions.defaultStreamerAttribution && !args[1].includes("@")) { args.splice(1,0,`@${channelData.displayName}`) }

maybe to be safe

const shouldInsertUsername = args.length === 1 || (commandOptions.defaultStreamerAttribution && !args[1].includes("@"));

maybe to be safe

const shouldInsertUsername = args.length === 1 || (commandOptions.defaultStreamerAttribution && !args[1].includes("@"));

Sorry @Oceanity I've been flat out and unable to try this suggestion out. Assuming I've understood it of course, I hope I've addressed it in b6e4ce2?

That branch looks like it still has the method in the same order. So, the idea is to move the invalid state above the API call.

Like, currently how you have it is:

API call to Twitch to get channelData

Check if the command provided was actually valid

Fail out if not, so API call is always made, even when in a fail state

Ideally it should be:

Check if the command provided was actually valid

Fail out if not

API call to Twitch to get channelData

I should have probably edited all my notes together instead of tacking on multiple comments because that probably made things unnecessarily confusing 😛

So here's my full proposal:

case "add": { // Get info on whether or not command is correct // If we need to add username, one less arg required, but don't poll API yet // `args.length === 1` is to prevent the check on `args[1].includes` throwing // Exception: can't access method 'includes' on undefined // `!quote add @Oceanity blablabla` needs minimum 3 args // `!quote add blablabla` needs minimum 2 args const shouldInsertUsername = args.length === 1 || (commandOptions.defaultStreamerAttribution && !args[1].includes("@")); const expectedArgs = shouldInsertUsername ? 2 : 3; // If args count not met, fail out after only initializing 2 bools and 0 API calls // (could even remove `expectedArgs` and inline that here, but this is more readable) if (args.length < expectedArgs) { await twitchChat.sendChatMessage(`Please provide some quote text!`); return resolve(); } // Now that it is for sure a valid quote add, start getting the data from the API const channelData = await TwitchApi.channels.getChannelInformation(); const currentGameName = channelData && channelData.gameName ? channelData.gameName : "Unknown game"; if (shouldInsertUsername) { args.splice(1,0,`@${channelData.displayName}`); } // rest of Add Quote logic }

Hopefully this is a bit easier to follow! 😁

@Oceanity I definitely did get a confused by the stream of consciousness style 😅 Thanks very much for laying it out again like that 🙇‍♂️

@Oceanity I'm pretty confident that I've actually addressed your request in a5b0c29.

I did tweak it a little so as not to circumvent the opt-in requirement @ebiggz requested; we don't want to jump to inserting the streamer name if the Option has not been selected in the settings.

…ttps://github.com/Wrongtown/Firebot into feature/2336-default-to-streamer-for-quotes-setting

… feature/2336-default-to-streamer-for-quotes-setting

…ture/2336-default-to-streamer-for-quotes-setting

Wrongtown · 2024-09-12T14:15:35Z

@ebiggz I foolishly forgot what repo I was working on with 00b6ee7 but thankfully that doesn't appear to have actually introduced any code change 🤦‍♂️

ebiggz · 2024-09-13T02:43:14Z

@Wrongtown No worries! LGTM. Thanks again for working on this 👍🏻

ebiggz and others added 2 commits August 27, 2024 09:55

Merge pull request crowbartools#2752 from crowbartools/v5

98a7760

v5.63.2

Addressing crowbartools#2336 to default quotes to streamer name if no…

543749c

… @ is provided

Wrongtown requested a review from ebiggz as a code owner August 27, 2024 16:08

Wrongtown mentioned this pull request Aug 27, 2024

[Feature Request] Ability to customise system commands further #2524

Open

ebiggz changed the base branch from master to v5 August 27, 2024 16:20

ebiggz requested changes Aug 27, 2024

View reviewed changes

crowbartools deleted a comment from dennisrijsdijk Aug 27, 2024

Merge branch 'v5' into feature/2336-default-to-streamer-for-quotes-se…

b7e1e10

…tting

Wrongtown added 3 commits August 28, 2024 16:08

PR#2753 - prefer string templates over string concatenation

1f79440

PR#2753 - Added quote management setting making this change opt-in

667ffbe

Merge branch 'feature/2336-default-to-streamer-for-quotes-setting' of h…

ac53832

…ttps://github.com/Wrongtown/Firebot into feature/2336-default-to-streamer-for-quotes-setting

Oceanity reviewed Aug 30, 2024

View reviewed changes

PR#2753 - Check arguments after applying streamer default

595b1c4

Wrongtown requested review from ebiggz and Oceanity August 31, 2024 09:36

Wrongtown mentioned this pull request Aug 31, 2024

feat: Remember Active Dashboard Chat User, Message Text, and Replying-To #2772

Merged

Wrongtown added 6 commits August 31, 2024 21:14

PR#2753 - Simplify to use logical AND as suggested

d0b4699

PR#2753 - use shouldInsertStreamerUsername constant suggested

b6e4ce2

Merge branch 'feature/2336-default-to-streamer-for-quotes-setting' of h…

5b0b04f

…ttps://github.com/Wrongtown/Firebot into feature/2336-default-to-streamer-for-quotes-setting

PR#2753 - Confirm command in syntactically valid before API calls

a5b0c29

Merge branch 'master' of https://github.com/crowbartools/Firebot into…

00b6ee7

… feature/2336-default-to-streamer-for-quotes-setting

Merge branch 'v5' of https://github.com/crowbartools/Firebot into fea…

b3b6960

…ture/2336-default-to-streamer-for-quotes-setting

ebiggz approved these changes Sep 13, 2024

View reviewed changes

ebiggz merged commit d1cf9c1 into crowbartools:v5 Sep 13, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addressing #2336 to default quotes to streamer name if no @ is provided #2753

Addressing #2336 to default quotes to streamer name if no @ is provided #2753

Wrongtown commented Aug 27, 2024 •

edited

Loading

CKY- commented Aug 27, 2024

ebiggz left a comment •

edited

Loading

ebiggz Aug 27, 2024 •

edited

Loading

Wrongtown Aug 28, 2024

Wrongtown commented Aug 27, 2024

Wrongtown commented Aug 27, 2024

Oceanity Aug 30, 2024

Wrongtown Aug 31, 2024

Wrongtown Aug 31, 2024 •

edited

Loading

Oceanity Aug 31, 2024

Wrongtown Aug 31, 2024

Oceanity Aug 31, 2024 •

edited

Loading

Wrongtown Sep 10, 2024

Oceanity Sep 10, 2024 •

edited

Loading

Wrongtown Sep 12, 2024

Wrongtown Sep 12, 2024

Wrongtown commented Sep 12, 2024

ebiggz commented Sep 13, 2024

Addressing #2336 to default quotes to streamer name if no @ is provided #2753

Addressing #2336 to default quotes to streamer name if no @ is provided #2753

Conversation

Wrongtown commented Aug 27, 2024 • edited Loading

Description of the Change

Example

Counter-example

Update after review comments

Applicable Issues

Testing

After review comments

Screenshots

CKY- commented Aug 27, 2024

ebiggz left a comment • edited Loading

Choose a reason for hiding this comment

ebiggz Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wrongtown commented Aug 27, 2024

Wrongtown commented Aug 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wrongtown Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oceanity Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oceanity Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wrongtown commented Sep 12, 2024

ebiggz commented Sep 13, 2024

Wrongtown commented Aug 27, 2024 •

edited

Loading

ebiggz left a comment •

edited

Loading

ebiggz Aug 27, 2024 •

edited

Loading

Wrongtown Aug 31, 2024 •

edited

Loading

Oceanity Aug 31, 2024 •

edited

Loading

Oceanity Sep 10, 2024 •

edited

Loading