progaem / a4iFfki

Achievements Telegram bot
MIT License
3 stars 1 forks source link

Repeated achievements #5

Closed 6b656b closed 2 months ago

6b656b commented 2 months ago

Added new logic: if a user writes the prompt which has already been used and there is a corresponding achievement in a sticker pack, bot would update an existing sticker (increment the number of issues) and give it to the user.

RuS2m commented 2 months ago

This is a great addition to the bot, thank you for your contribution!

I really like the re-usage of on_sticker_reply method, however I suspect that since the method is primarily used as a separate telegram command handler, in the current feature implementation when user would give a repeated achievement, we would do twice as many access checks for them, including invoking counters tracking their call twice. This would mean that this type of bot interaction would count as two instead of one, nearing the exhausting of the throttling limit for the user twice faster than intended.

In order to fix this, I would suggest moving the re-used logic from on_sticker_reply method into separate private function within Bot class. Something like the following:

We would declare a private method inside Bot class:

    async def __give_user_existing_achievement(to_user_id: int, to_user_name: str, chat_id: int, from_user_name: str, chat_name: str, achievement_sticker_info: ChatSticker, description_sticker_info: ChatSticker, update: Update, context: CallbackContext)
        achievement_sticker = self.sticker_file_manager.get_bytes_from_path(achievement_sticker_info.file_path)
        user_description_sticker = self.sticker_artist.draw_description_sticker(description_sticker_info.engraving_text)
        # add and update stickers
        achievement_user_sticker_file_id = await self.sticker_manager.add_user_stickers(
            description_sticker_info.sticker_set_owner_id,
            to_user_id,
            to_user_name,
            chat_id,
            chat_name,
            (achievement_sticker_info.file_path, achievement_sticker),
            user_description_sticker,
            description_sticker_info.engraving_text,
            update,
            context
        )
        achievement_description_chat_sticker_file_id = await self.sticker_manager.increase_counter_on_chat_description_sticker(
            description_sticker_info.sticker_set_owner_id,
            chat_id,
            description_sticker_info.file_id,
            description_sticker_info.sticker_set_name,
            description_sticker_info.index_in_sticker_set,
            description_sticker_info.engraving_text,
            description_sticker_info.times_achieved,
            update,
            context
        )
        await self.__respond_with_achievement_stickers(update, context, from_user_name, to_user_name, chat_name, chat_id, description_sticker_info.engraving_text, achievement_user_sticker_file_id, achievement_description_chat_sticker_file_id)

Re-use this method inside Bot::on_sticker_reply:

            ...
            if sticker_type == 'achievement':
                if await self.warnings_processor.add_give_achievement_warning(update, context):
                    session.close()
                    return

                # retrieve mentioned achievement and it's description stickers
                index_based_lookup = {s.index_in_sticker_set: s for s in chat_sticker_set_info}
                achievement_sticker_info = index_based_lookup[sticker_index]
                description_sticker_info = index_based_lookup[sticker_index + 5]

                await __give_user_existing_achievement(to_user_id, to_user_name, chat_id, from_user_name, chat_name, achievement_sticker_info, description_sticker_info, update, context)
            elif sticker_type == 'empty':
                context = await self.telegram.reply_text(
                    "This achievement is not unblocked yet, so you can't give it to someone else!",
                    update,
                    context
                )

        session.commit()
        session.close()

And inside the on_give method:

        ...
        # if sticker already exists we want to use it (not create another one)
        (achievement_sticker_info, description_sticker_info, session) = await self.sticker_manager.find_existing_sticker(chat_id, prompt)
        if sticker_file_id:
            await __give_user_existing_achievement(to_user_id, to_user_name, chat_id, from_user_name, chat_name, achievement_sticker_info, description_sticker_info, update, context)
            session.commit()
            session.close()
            return
        session.commit()
        session.close()

We also would have to slightly change the db call we make, so you can adapt this suggestion in a different way. My example is a bit too verbose with number of functions arguments and I'm not 100% happy with it either, but will be very happy to know what you think of this suggestion and discuss it!

6b656b commented 2 months ago

@RuS2m Thanks for your comments, I fixed the logic