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

[Wrongtown]

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

[CKY-]

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]

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

`@${channelData.displayName}`

[Wrongtown]

Addressed in 1f79440

[Wrongtown]

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 🤦‍♂️

[Wrongtown]

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.

[Oceanity]

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

[Wrongtown]

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

[Wrongtown]

I've addressed this in 595b1c4.

[Oceanity]

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

[Wrongtown]

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}`)
}

[Oceanity]

maybe to be safe

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

[Wrongtown]

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?

[Oceanity]

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! 😁

[Wrongtown]

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

[Wrongtown]

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

[Wrongtown]

@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]

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