Without explicit scope, set default scope only with default handler

[PonomareVlad]

This pull request refines command scope assignment logic in the CommandGroup and Command classes and significantly expands the test suite to verify correct behavior for commands with and without handlers, as well as various scope configurations. The changes ensure that commands are properly assigned to default and explicit scopes based on their handler presence and scope declarations.

Command scope assignment improvements

• Updated CommandGroup to automatically add commands with no explicit scopes to the default scope, ensuring all commands are properly scoped even if not explicitly assigned.
• Modified Command to use an internal _hasHandler flag to determine if a handler exists before adding the command to the default scope, improving reliability of handler detection.

Test suite enhancements

• Added comprehensive tests to command-group.test.ts to cover new scope assignment behaviors, including cases for commands with/without handlers and with/without explicit scopes.
• Improved and expanded tests for merging and retaining scopes and localizations between different command groups, ensuring correct merging logic and localization support. [1] [2]
• Refactored existing tests to use explicit scope assignment and handler presence for clarity and consistency with the updated logic.

[carafelix]

I would like to test what happens when someone tries to register a command without any handlers... Just because I don't remember exactly what it could compromise, but this little change makes a lot of sense tbh.

something which test the following:

commands
    .command('broadcast', 'Send broadcast to all users')
    
    await commands.setCommands(bot)

[PonomareVlad]

@carafelix is that test cases correct ?

commands
    .command('...', '...') // Command without default handler and explicit scope

void {
    scope: { type: 'default' },
    commands: [{ command: '...', description: '...' }],
}

commands
    .command('...', '...', () => {}) // Command with default handler

void {
    scope: { type: 'default' },
    commands: [{ command: '...', description: '...' }],
}

commands
    .command('...', '...') // Command without default handler
    .addToScope({ type: 'chat', chat_id: 0 }) // but with explicit scope

void {
    scope: { type: 'chat', chat_id: 0 },
    commands: [{ command: '...', description: '...' }],
}

commands
    .command('...', '...', () => {}) // Command with default handler
    .addToScope({ type: 'chat', chat_id: 0 }) // and explicit scope

void {
    scope: { type: 'default' },
    commands: [{ command: '...', description: '...' }],
}

void {
    scope: { type: 'chat', chat_id: 0 },
    commands: [{ command: '...', description: '...' }],
}

Where every void {} contains payload for setMyCommands method

[carafelix]

@PonomareVlad yup! That looks like the should-be-intended functionality :P

Please add those test cases and fix the unrelated test cases that got affected by this and I'll merge.

TO-DO reminder for myself: Refactor test cases which are too-heavily coupled with the inners output. Just check for the properties needed

[PonomareVlad]

If we assume that the user of this plugin uses it to set explicit scopes, then previous default scope commands setup can be considered obsolete:

commands
    .command('...', '...') // Command without default handler
    .addToScope({ type: 'chat', chat_id: 0 }) // but with explicit scope

void {
    scope: { type: 'chat', chat_id: 0 },
    commands: [{ command: '...', description: '...' }],
}

@carafelix so, in this case do we should to perform reset of default scope using empty commands array (or deleteMyCommands method) ?

commands
    .command('...', '...') // Command without default handler
    .addToScope({ type: 'chat', chat_id: 0 }) // but with explicit scope

void {
    scope: { type: 'default' },
    commands: [],
}

void {
    scope: { type: 'chat', chat_id: 0 },
    commands: [{ command: '...', description: '...' }],
}

[carafelix]

in this case do we should to perform reset of default scope using empty commands array

If, and only if, all the above test passes.

As a backwards fix, I think it would be ideal.

I think would benefit people who use to declare a default scoped command in any manner and later on decided to remove it from the default scope and only scope it.

Iirc, if you have any other command in the default scope this would get reset anyways but for groups with only 1 command, it would not.

Thanks for thinking about that too

[PonomareVlad]

@carafelix I saw some test cases for default scope, where we provide chat_id property, is this correct behavior or we need to fix this too ? (maybe in another PR)

commands/test/command-group.test.ts

Line 211 in 6653e2a

scope: { type: "default", chat_id: 10 },

commands/test/command-group.test.ts

Line 302 in 6653e2a

scope: { type: "default", chat_id: 10 },

commands/test/command-group.test.ts

Line 328 in 6653e2a

scope: { type: "default", chat_id: 10 },

BotCommandScopeDefault

[PonomareVlad]

@carafelix also, can you check current changes in PR before I will go next (is this proper test cases, placement and format) ?

[carafelix]

we provide chat_id property, is this correct behavior or we need to fix this too ?

It's okay. Chat id is a required property in every update manage by the plugin. We pass it in the testing simply as a mock.

[carafelix]

Nice :3
The tests you added are all good ^-^ gj

[carafelix]

This test is render useless here. Deletion is just making it pass but defeat the porpoise of the test.

Instead could you test it in a specific scope but without handler? So it is clear that toSingleScopeArgs we still get commands without handlers but marked.

something like:

commands.command("test", "handler", (_) => _)
        .addToScope({
              type: "all_private_chats",
         });

commands.command("markme", "nohandler")
        .addToScope({
              type: "all_private_chats",
         });

// Test for it in scope: "all_private_chats"

[PonomareVlad]

commands/test/command-group.test.ts

Lines 144 to 147 in 485c8cf

	commands.command("test", "handler", (_) => _)
	.addToScope({ type: "all_private_chats" });
	commands.command("markme", "nohandler")
	.addToScope({ type: "all_private_chats" });

[carafelix]

Same here, instead of just deleting the default case, could you manually add a scope back in the command definition?

maybe:

        .addToScope({
              type: "all_private_chats",
         })
        .addToScope({
              type: "all_chat_administrators",
         });
         
// check each scope is merge too

maybe diverge command a from b and check each it correspond so they share like... all_chat_administrators and all_group_chats but diverge in all_private_chats

[PonomareVlad]

commands/test/command-group.test.ts

Lines 324 to 334 in 485c8cf

	const a = new CommandGroup();
	a.command("a", "private chats")
	.addToScope({ type: "all_private_chats" })
	.addToScope({ type: "all_chat_administrators" })
	.localize("es", "a_es", "private localized");
	const b = new CommandGroup();
	b.command("b", "group chats")
	.addToScope({ type: "all_group_chats" })
	.addToScope({ type: "all_chat_administrators" })
	.localize("fr", "b_fr", "group localized");