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

Feat/delete messages upto 24 hrs #945

Closed
wants to merge 29 commits into from
Closed

Feat/delete messages upto 24 hrs #945

wants to merge 29 commits into from

Conversation

ankitsmt211
Copy link
Member

@ankitsmt211 ankitsmt211 commented Nov 5, 2023

First draft of feature, delete messages upto 24 hrs in same channel.

Additions:

  • db entries also has an emergency brake to prevent spamming DB to death.
  • a conditional sub routine, when approaching records limit auto deletes N no of oldest records from table to make some space for new entries.

command interface:

image

when there's no records:
image

if there's any records within selected duration:
image

Note: Duration is optional, if skipped it defaults to last hour.

@ankitsmt211 ankitsmt211 requested review from a team as code owners November 5, 2023 23:08
@ankitsmt211
Copy link
Member Author

Couple of things to work atm:

  • do something with reason for using this command, perhaps store it somewhere.
  • How long to keep message data in DB, currently anything older than 2 hrs is removed.

Copy link
Member

@Zabuzard Zabuzard left a comment

Choose a reason for hiding this comment

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

Please dont forget to also update the PP.md file. We must inform users about how we store their data.

@ankitsmt211
Copy link
Member Author

Please dont forget to also update the PP.md file. We must inform users about how we store their data.

Is this necessary? user_id, message_id, timestamp of actions, channel_id is already mentioned in database section.

source:
image

@ankitsmt211 ankitsmt211 self-assigned this Nov 6, 2023
@Zabuzard
Copy link
Member

Zabuzard commented Nov 7, 2023

Please dont forget to also update the PP.md file. We must inform users about how we store their data.

Is this necessary? user_id, message_id, timestamp of actions, channel_id is already mentioned in database section.

source: image

Ah, good. Perhaps tweak this paragraph:

Furthermore, upon utilization of our help service, user_ids, channel_ids and timestamps are stored to track when/how many questions a user asks. The data may be stored for up to 30 days.

(added and timestamps)

@ankitsmt211
Copy link
Member Author

ankitsmt211 commented Nov 12, 2023

Atm duration is optional (default is 1hr) but if provided can be entered from 1-24 as value. I was thinking perhaps a better way would be to provide choices such as 3 hrs, 6 hrs, 12 hrs and 24 hrs where 1 hr is default. This makes easier for user to enter duration altho there are already checks in place for value to be between 1-24 if i stick to current way of input.

@ankitsmt211 ankitsmt211 changed the title Feat/delete last hour messages Feat/delete messages upto 24 hrs Nov 12, 2023
@@ -58,7 +58,7 @@ private void updateHistory(MessageReceivedEvent event) {

incrementRecordsCounter();
} else {
logger.warn("purge history reached limit");
logger.debug("purge history reached limit");
Copy link
Member

Choose a reason for hiding this comment

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

the log would be more useful if it also gives a suggestion how to fix the problem.

"Purge History reached limit. Either delete some rows of the table or wait for them to expire."

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually i think i might have a better idea, Imagine the DB is maxed out with 7.5k messages. No more records are being saved, which means any new spam messages would be ignored and using purge-in-channel would only delete older messages which might not be the best thing . Ideally i think we should keep the messages from newest to oldest upto the "max limit".

Possible Solution:
Let's say we have a limit 7.5k, we hit a mark of 7.4k. A routine should be trigged that only clears out last 500 records or so. This way all the new messages are being tracked and command is not useless past the limit. Thoughts ?

@ankitsmt211
Copy link
Member Author

@Zabuzard @Tais993 @Taz03
I need some suggestions about a possible problem,

Context:

  • A routine (MessageHistoryRoutine) that clears DB records every 24 hrs(no need to hold records past that)
  • Purge command that has duration options such as 3hrs, 6 hrs, 12 hrs, 24 hrs.(since we're only storing records upto 24 hrs, max is that)

It become problematic when someone in future adds a new option in PurgeHistoryCommand for 48 hrs or 72 hrs and forgets to reflect that change in routine class that clears every expired record past 24 hrs(so db will never hold anything past 24 hrs).

Point is numbers can change with time and i wanna make sure both the classes are on same page when it comes to this duration limit.

I decided to move constants to a different class Duration, figured it would be neat to add other constants related to duration as well. I'm still not sure if this helps much, Any alternatives or thoughts or perhaps im doing something wrong?

Current:

MessageHistoryRoutine.java

image

PurgeHistoryCommand.java

image

Possibly newer changes:

Add Duration.java

image

MessageHistoryRoutine.java

image

PurgeHistoryCommand.java

image

@ankitsmt211 ankitsmt211 added enhancement New feature or request priority: normal labels Nov 20, 2023
* enum will hold duration constants for feature
* tiny fixes in message for user of command
* hrs-> hr for single message and in x hrs -> within x hrs
@ankitsmt211 ankitsmt211 added the new command Add a new command or group of commands to the bot label Nov 25, 2023
@ankitsmt211
Copy link
Member Author

This feature has reached it's final stage and is now open for reviews.

@ankitsmt211 ankitsmt211 closed this by deleting the head repository Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request new command Add a new command or group of commands to the bot priority: normal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete last X messages from user for moderation
2 participants