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.

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

fix: add delay registration to default scope handler #50

Merged
merged 4 commits into from
Nov 8, 2024

Conversation

roziscoding
Copy link
Contributor

Fixes #49

@roziscoding roziscoding self-assigned this Nov 6, 2024
@roziscoding roziscoding changed the base branch from main to next November 6, 2024 15:39
@roziscoding roziscoding changed the title test: add integration test for default handler (#44) fix: delay reigstration of default scope handler Nov 6, 2024
@carafelix carafelix changed the title fix: delay reigstration of default scope handler fix: add delay registration to default scope handler Nov 6, 2024
carafelix
carafelix previously approved these changes Nov 6, 2024
Copy link
Member

@carafelix carafelix left a comment

Choose a reason for hiding this comment

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

Simple and clean. LGTM

src/command.ts Outdated Show resolved Hide resolved
@carafelix carafelix dismissed their stale review November 6, 2024 16:46

Further testing is needed

@carafelix
Copy link
Member

carafelix commented Nov 7, 2024

        const defaultHandler = spy(() => {});
        const specificHandler = spy(() => {});

        const commandGroup = new CommandGroup();
        commandGroup.command("command", "_", defaultHandler, { prefix: "!" })
          .addToScope({ type: "all_group_chats" }, specificHandler);

        const bot = getBot();
        bot.use(commands());
        bot.use(commandGroup);

        await bot.handleUpdate(
          getDummyUpdate({
            chatType: "group",
            userInput: "!command",
          }),
        );
        assertSpyCalls(defaultHandler, 0); // called once
        assertSpyCalls(specificHandler, 1); // no call

Set up this little integration test and it's failing. Also tried in a real bot, and the default handler it's still taking prevalence.

Should I commit the test (+ the change to getDummyUpdate to allow changing the chat type) to the pr?

I'll take a look now on why is it not properly fixing the issue

@KnorpelSenf
Copy link
Member

#50 (comment) needs to be fixed before tests are useful

Copy link
Member

@carafelix carafelix left a comment

Choose a reason for hiding this comment

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

The man as spoken

@carafelix carafelix merged commit 8e9612d into next Nov 8, 2024
6 checks passed
@KnorpelSenf KnorpelSenf deleted the delayed-default-handler branch November 8, 2024 09:04
carafelix added a commit that referenced this pull request Nov 8, 2024
* fix: register default scope handler last

* feat: add integration test for ensuring default handler is register last

* fix: return main composer instead of the nested one

* fix: return a completely new composer

---------

Co-authored-by: Hero Protagonist <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default command handler overshadowing every other scope
3 participants