EdJoPaTo / grammy-inline-menu

Inline Menus for Telegram made simple. Successor of telegraf-inline-menu.
MIT License
354 stars 46 forks source link

toggle's "set" method never executes #156

Closed ssj4maiko closed 3 years ago

ssj4maiko commented 3 years ago

Describe the bug For Toogle option, although the menu is rendered, "set" function is never runs.

Versions Did you updated telegraf and telegraf-inline-menu to the latest version?

What version of NodeJS (and TypeScript) do you use?

To Reproduce I have the following code.

const categoriesTemplate = new MenuTemplate(
    ctx => 'Select categories'
);
    let options = Object.keys(categories);
    options.forEach((key:string) => {
        categoriesTemplate.toggle(categories[key], key, {
            set: async (ctx, checked, path): Promise<string> => {
                console.log('SET: ', checked, ctx, path);
                /* @ts-ignore */
                let SESSION = ctx.session;
                if (checked){
                    SESSION.categories.push(key);
                } else {
                    SESSION.categories.splice(SESSION.categories.indexOf(key),1);
                }
                console.log('SET - ',SESSION.categories);
                return 'unfinished code, since it doesn\'t reach here anyway';
            },
            isSet: async (ctx: unknown, path) => {
                console.log('ISSET: '/*,ctx*/);
                /* @ts-ignore */
                let SESSION = ctx.session as RedisSessionData;
                return SESSION.categories.indexOf(key) > -1;
            }
        });
    });
const MainMenuTemplate = new MenuTemplate(
    ctx => 'Main Menu'
);
MainMenuTemplate.submenu('Categories', 'categories', categoriesTemplate);
const MainMenu = new MenuMiddleware('/', MainMenuTemplate);

export default MainMenu;

It takes the keys from a json [Key]=Label, and draws the menu. To this point, it has worked properly. The isSet method is also working as expected. However, the console.log on "set" is never reached, so no matter how many times I press the buttons on telegram, all it does is essentially refresh it.

I tried taking out the async, but nothing works, it simply never runs. I don't know how the plugin works, so I should mention that I'm using "telegraf-session-redis", which has not been updated in a long time, so I'm unsure if it could be a case of conflict.

Another project that I have access to uses older versions of Telegraf, the same Redis Plugin and and older version of Inline Menu, and still works.

Expected behavior The "set" method would be executed, which would allow me to interact with the menu.

EdJoPaTo commented 3 years ago

The idea of the MenuTemplate is to have their structure finished on startup. The content is then loaded via functions when the template is rendered. Looking at this a loop is creating the buttons which contradicts the idea here.

You should try to use menuTemplate.select which basically provides a toggle for many values. It basically allows for Record<string, string> with key -> label which is exactly what your categories seems to be so it should be fairly simple to use.

Hope it helps :)

ssj4maiko commented 3 years ago

I see, it seems I misunderstood the content of "select", I had mistaken it as "one value out of many" (A radio button of sorts).

But in regards to your explanation about Templates, I can understand that the menu itself is programmed one time only with static options, but the values (true and false) shouldn't be, right? That should be the reason why isSet is a method, to dynamically change the values. So I'm not sure if that'll be a problem here.

The object categories are basically a multi-select option, with the user capable of toggling each on and off, with the results being saved on the session (As an array of key values, hence using indexOf/split and push). If a new category is to be added (Let's say, in a year), I believe just restarting the server will work that part out, so it's shouldn't be a problem.

I think you misunderstood the categories as being dynamic per user, but fortunately for me (yay) that's not the case, the per user content will be in the session.

Do correct me if I understood it wrong though.


I thought about trying to remake the menu every time the toggle is changed, but besides other problems on how to redesign the code to allow it and keep other functionalities working (to recognize who is talking to the bot), I'm not sure if just the toggles would require all of this, as the idea of the toggles just drawing the template statically contradicts its functionality.


But I did as you suggested, using select, although the design changed, the rendering was fine.

    /* @ts-ignore */
    categoriesTemplate.select('categories', categories, {
        showFalseEmoji: true,
        isSet: (ctx, key) => {
            console.log('isset');
            /* @ts-ignore */
            let SESSION = ctx.session as RedisSessionData;
            return SESSION.categories.indexOf(key) > -1;
        },
        set: (ctx, key, checked) => {
            console.log('SET: ', checked, ctx);
            /* @ts-ignore */
            let SESSION = ctx.session;
            if (checked) {
                SESSION.categories.push(key);
            } else {
                SESSION.categories.splice(SESSION.categories.indexOf(key), 1);
            }
            return true;
        }
    });

It still never reaches the set method... So I guess I understood it wrong after all?

EDIT: Doing a console log of the ctx during the isset, and checking the inner workings related to the "path", made me wonder if I should do something more specific about it.

These are the relevant parts of the context that I found:

  update: {
    update_id: *****,
    callback_query: {
      id: '*****',
      from: [Object],
      message: [Object],
      chat_instance: ' *****',
      data: '/categorias/categoriesT:imoveis'
    },
  },
  match: [
    '/categorias/',
    index: 0,
    input: '/categorias/categoriesT:imoveis',
    groups: undefined
  ]

As I posted on the 1st post, It's in a submenu (action = categorias), and in that submenu, I added the select named categories.

I then selected the 3rd option imoveis, which I would expect to return the 3rd match, but seems like it returned 0/groups undefined?. I'm unaware of how it's supposed to work, but thought this could be relevant.

EdJoPaTo commented 3 years ago

I see, it seems I misunderstood the content of "select", I had mistaken it as "one value out of many" (A radio button of sorts).

It basically can do both. Any idea on how to improve the documentation about it?

But in regards to your explanation about Templates, I can understand that the menu itself is programmed one time only with static options, but the values (true and false) shouldn't be, right? That should be the reason why isSet is a method, to dynamically change the values. So I'm not sure if that'll be a problem here.

The object categories are basically a multi-select option, with the user capable of toggling each on and off, with the results being saved on the session (As an array of key values, hence using indexOf/split and push). If a new category is to be added (Let's say, in a year), I believe just restarting the server will work that part out, so it's shouldn't be a problem.

I think you misunderstood the categories as being dynamic per user, but fortunately for me (yay) that's not the case, the per user content will be in the session.

You got it right there. Maybe to state it explicitly: The MenuTemplate only contains the structure, your data comes later. The Template doesnt care if the data is static or from a function. That function can use the ctx to get user specific data but it doesnt have to.

let SESSION = ctx.session as RedisSessionData;

Something like this looks like you might wanna create your own Context which know that type by itself rather than the need of using as. Also I'm not sure why the @ts-ignore is used here and what triggered it.

It still never reaches the set method... So I guess I understood it wrong after all?

From what I see so far I would assume the set method should be called. Not sure what is also going on. Not sure if the format of categories is different from what works as a choice as you somehow need @ts-ignore there. I think getting rid of the TypeScript compiler errors should be priority.

As I posted on the 1st post, It's in a submenu (action = categorias), and in that submenu, I added the select named categories.

I then selected the 3rd option imoveis, which I would expect to return the 3rd match, but seems like it returned 0/groups undefined?. I'm unaware of how it's supposed to work, but thought this could be relevant.

match contains the regular expression match of the callback_query.data. As you are rendering the menu it matches the menu, therefore it matched /categorias/ which looks correct for me. It is independent from the order of choices you provided the select.

I like seeing your efforts on understanding the problem there. Sadly I'm still not sure what the problem there might be. I would try to simplify it. Maybe temporarly provide ['a', 'b', 'c'] instead of categories. Maybe add it to the main menu and not a submenu in order to reduce other stuff going on.

I hope I could provide some ideas on how to continue the search for the cause.

ssj4maiko commented 3 years ago

It basically can do both. Any idea on how to improve the documentation about it?

So it can both "select one and deselects everything else automatically" and "select one and only deselects manually"? That feels confusing for a single method.

Something like this looks like you might wanna create your own Context which know that type by itself rather than the need of using as. Also I'm not sure why the @ts-ignore is used here and what triggered it.

Some time ago I had worked with Angular on a personal app, and because there were no extra plugins, I could do everything looking beautiful and finely. This time however, I had been having a lot of small compilations problems with types as I'm using external libraries like Telegraf that I don't know much about yet, and considering that the code is not working, it doesn't help in that regard either, so I'm adding ts-ignore for tests trying to make it work without thinking too much.

I'm a newbie in typescript, although I have used typed languages before, there are some things that I'm still learning, so for the time, I'm just trying not to waste too much time on stuff that may not work, and the fix it properly once it does. I'm only using in cases such as using it as local variables.

match contains the regular expression match of the callback_query.data. As you are rendering the menu it matches the menu, therefore it matched /categorias/ which looks correct for me. It is independent from the order of choices you provided the select.

I like seeing your efforts on understanding the problem there. Sadly I'm still not sure what the problem there might be. I would try to simplify it. Maybe temporarly provide ['a', 'b', 'c'] instead of categories. Maybe add it to the main menu and not a submenu in order to reduce other stuff going on.

I hope I could provide some ideas on how to continue the search for the cause.

I think I found some more relevant information:

https://github.com/EdJoPaTo/telegraf-inline-menu/blob/main/source/menu-template.ts - Line 336

This is where the "set" options are set up.

I have changed my variables as according to your suggestion into random strings: The field is abc, the options are [def, ghi, xyz], and they are inside the submenu /categorias.

But when I check the regular expression there, it gives me /abcT:(.+)$/ , so I'm guessing it's ignoring the submenu? I did try to add categorias/ just as a test straight there, and I did get the error actions are not relative. They have to be below the menu (dont contain / or ..), but then again, maybe a bug when using inside submenus?

I am trying to patch it up locally as testing for something that can work, so if I truly find a bug I can report, but since I only have the JS compiled resource on the node_modules folder, and am trying to understand the ideas, it's a bit convoluted.

I did notice that ctx.update.callback_query.data does return the selected option '/categorias/abcT:xyz' hence I thought that maybe the regex, which doesn't look for anything before the select's name is ignoring it.

And about moving it to the main menu, the situation is that the main menu may contain other submenus, besides it already has a link to the company's website. So to keep it presentable to clients, it's better presented on its own as a submenu.


I just noticed that you have the .chooseIntoSubmenu into Submenu method... Although I would have to find a way to get the categories from the API before the MainMenu is exported... Although I'm guessing this one works with a single option, and returns back to the main menu, right? ...

ssj4maiko commented 3 years ago

I found the error. And I guess it's indeed not a bug. Now that I look at the first post, it's indeed not there because I thought it wasn't relevant.

Because I get the options from an API, the template is made after it returns, however, the rest of the menu ends up updated later, so while the options are added properly, the actions don't seem to target the updated menu.

So the moment that I just made everything static, the "set" worked.

I'm not sure if this could be considered a bug, since, as you said initially, you are supposed to have everything done by the time you export the Main Menu, but it seems I got mislead by the fact that the updates did show, although the interaction did not.

TL;DR

Not sure how the paths work in finding if an option exists or not, but I guess those are not updated after exported


Finally I was able to do it. Now that I knew the problem, I had to do a lot of rearrangement on the code.

Because I had the MainMenu and Authentication in different places, I had to turn them into functions to accept the results from the API, and then those would render the Menu as soon as it's rendered, so basically, on my base bot file:

getCategorias().then((cats) => {
    let mm = MainMenu(cats.data.data); //creates the manu with the variable already in.
    bot.use(mm);
    bot.MainMenu = mm; // The bot is made with my extended class where I added the MainMenu option attribute, this allows me to access this dynamic Menu from other places.

    let ss = StartupStage(bot.MainMenu); //Here is where I confirm the authentication, so if it's clear, goes straight to the MainMenu
    bot.use(ss.middleware());

    bot.launch()

    process.once('SIGINT', () => bot.stop('SIGINT'))
    process.once('SIGTERM', () => bot.stop('SIGTERM'))
}).catch((err) => {
    console.log(err);
});