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

send original message to main channel if alleged spam was unbanned b… #173

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

evgeny-boger
Copy link

…y admin

@evgeny-boger evgeny-boger requested a review from umputun as a code owner November 7, 2024 21:02
@MissiaL
Copy link

MissiaL commented Nov 25, 2024

@umputun could you check this PR?
It's really helpful PR

@umputun
Copy link
Owner

umputun commented Dec 2, 2024

@umputun could you check this PR? It's really helpful PR

I don't understand why it's needed and what problem it fixes. First of all, restoring such a message in a normal (non-soft) mode doesn't make much sense at all (all other messages are removed too), and even in soft mode, publishing the message on behalf of the bot is a really strange thing to do.

If someone can convince me why it makes sense, I would consider it.

@MissiaL
Copy link

MissiaL commented Dec 3, 2024

We often have cases where a user is banned by accident. Group administrators have to search for this message and restore it, forward this message manually. This takes a lot of time in the flow of a large amount of spam.
In this PR, the user name is indicated when the message is restored, which is also very convenient

@umputun
Copy link
Owner

umputun commented Dec 3, 2024

We often have cases where a user is banned by accident. Group administrators have to search for this message and restore it, forward this message manually. This takes a lot of time in the flow of a large amount of spam. In this PR, the user name is indicated when the message is restored, which is also very convenient

I see, makes sense to me now.

Copy link
Owner

@umputun umputun left a comment

Choose a reason for hiding this comment

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

Thanks. Generally, the change looks legitimate to me. I didn't like the nested logic with if/else/if/else and asked to simplify it.

Another issue is that it has no tests for the change; it would be nice to have them added.

fmtName := ""
if name != "" {
fmtName = fmt.Sprintf("[%s](tg://user?id=%d)", name, userID)
} else {
Copy link
Owner

Choose a reason for hiding this comment

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

the logic with nested if/else is hard to read and follow. Can you simplify it by making it a single-level and preferable without else.

@umputun umputun requested a review from paskal December 3, 2024 17:46
@@ -59,6 +59,7 @@ Second, are messages the bot is sending. There are three messages user may want
- `--message.startup=, [$MESSAGE_STARTUP]` - message sent to the group when bot is started, can be empty
- `--message.spam=, [$MESSAGE_SPAM]` - message sent to the group when spam detected
- `--message.dry=, [$MESSAGE_DRY]` - message sent to the group when spam detected in dry mode
- `--message.restore=, [$MESSAGE_RESTORE]` - message sent to the group when original message is unbanned by admin, can be empty to supress reporting the original message
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should better highlight that the bot would be the poster of that message, and not the original message forwarded or magically restored.

@@ -298,6 +299,7 @@ message:
--message.dry= spam dry message (default: this is spam (dry mode)) [$MESSAGE_DRY]
--message.warn= warning message (default: You've violated our rules and this is your first and last warning. Further violations will lead to permanent access denial. Stay
compliant or face the consequences!) [$MESSAGE_WARN]
--message.restore= restore message [$MESSAGE_RESTORE]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This description is insufficient.

@@ -35,6 +35,7 @@ type TelegramListener struct {
TestingIDs []int64 // list of chat IDs to test the bot
StartupMsg string // message to send on startup to the primary chat
WarnMsg string // message to send on warning
RestoreMsg string // message to send on restore
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: for tabs creeped in otherwise space-formatted file here

@@ -104,6 +104,7 @@ type options struct {
Spam string `long:"spam" env:"SPAM" default:"this is spam" description:"spam message"`
Dry string `long:"dry" env:"DRY" default:"this is spam (dry mode)" description:"spam dry message"`
Warn string `long:"warn" env:"WARN" default:"You've violated our rules and this is your first and last warning. Further violations will lead to permanent access denial. Stay compliant or face the consequences!" description:"warning message"`
Restore string `long:"restore" env:"RESTORE" default:"" description:"restore message"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as in the Readme, should be updated to describe better what setting this variable does.

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.

4 participants