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

Denylist feature #396

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Conversation

dmytroavramenko
Copy link
Collaborator

No description provided.

@dmytroavramenko dmytroavramenko changed the title Feature/uabot 189 denylist Denylist feature Sep 3, 2024
Copy link

sonarqubecloud bot commented Sep 3, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
9.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@@ -302,6 +303,7 @@ export const getBot = async (bot: Bot<GrammyContext>) => {

// Main message composer
notChannelComposer.use(messagesComposer);
notChannelComposer.use(denylistComposer);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be best if you used the denylistComposer composer inside the messages composer. Please look at the examples above of the noAntisemitismComposer composer.

await bot.handleUpdate(update);

expect(outgoingRequests.length).toEqual(0);
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I also would add a case to check it inside a message. So we will be sure that it blocks the message if it's not only the entire block message


describe('disabled feature', () => {
beforeAll(() => {
chatSession.chatSettings.denylist = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should not clear the list. It still should include the word. But we need to turn the settings off, and the bot won't delete the message. This is the idea of the test. So please, update it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I got the idea. You assume the feature is enabled if we have at least one word and turned off if the list is empty. It will work, but we need to give the user more flexibility. Please add an additional checkbox for toggling this feature. It will help temporarily disable the feature without deleting all words.

const denyWord = denylist.find((word) => text.toLowerCase().includes(word.toLowerCase()));
if (denyWord) {
await context.deleteMessage();
await logDenylistMessage(context, text);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please log the exception word instead of the entire list of words? Logging all the words could break our logs, and we don't really need to. It's more important for us to know the word than the list.

/**
* Messages to display when deleting content based on the denylist.
*/
export const deleteDenylistMessages = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the list of words! Thanks a lot!

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.

2 participants