-
Notifications
You must be signed in to change notification settings - Fork 15
Feat/spam digest job #905
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
base: develop
Are you sure you want to change the base?
Feat/spam digest job #905
Conversation
- Introduced SpamDigestEvent to notify admins through Decidim’s notification system - Added I18n translations for AI spam digest summaries
d7719b0 to
1d46b3a
Compare
AyakorK
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of reviews, but we probably don't need to fix everything, some of them might be critical but other issues are reported just to keep a trace on it !
| Class.new do | ||
| def initialize(path, url) | ||
| @path = path | ||
| @url = url | ||
| end | ||
|
|
||
| def path(_params = nil) | ||
| @path | ||
| end | ||
|
|
||
| def url(_params = nil) | ||
| @url | ||
| end | ||
| end.new( | ||
| resource_path, | ||
| helpers.root_url(host:, protocol: "http") | ||
| ) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a huge fan of anonymous classes, Struct might be a little more maintainable or even better an external class that would help making some specs on it (like a ResourceLocator)
| admins = organization.admins.where(notifications_sending_frequency: frequency) | ||
| next if admins.empty? | ||
|
|
||
| spam_count = count_spam(organization, frequency) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm slight
Maybe we should try to do the filter using SQL and not Ruby, but maybe try to see with other devs to check their POV
| # Returns all spam reports created since the given time | ||
| def spam_reports_since(since) | ||
| Decidim::Report | ||
| .joins(:moderation) | ||
| .where(reason: "spam") | ||
| .where("decidim_reports.created_at >= ?", since) | ||
| end | ||
|
|
||
| def spam_user_reports_since(since) | ||
| Decidim::UserReport | ||
| .joins(:user) | ||
| .where(reason: "spam") | ||
| .where("decidim_user_reports.created_at >= ?", since) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great ! But we probably could refactor this as the only difference is the .joins of each method
| class SpamDigestGeneratorJob < ApplicationJob | ||
| queue_as :mailers | ||
|
|
||
| def perform(frequency) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to have: We could validate the frequency parameter to avoid silent failures if an invalid value is passed.
Example but it might not be exactly what you wanted to do
FREQUENCIES = {
daily: "daily",
weekly: "weekly"
}.freeze
Then check with: raise ArgumentError unless FREQUENCIES.key?(frequency.to_sym)
| end | ||
|
|
||
| def send_notification_to_admins! | ||
| return if spam_report? && !frequency_notifications_is_realtime?(@report.moderation.user.organization.admins) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@report.moderation.user.organization.admins
1/2 This probably loads all admins into memory just to check a boolean. We might consider using .exists? for better performance, once again it might not be a big problem as there is rarely more than 10 admins
| end | ||
|
|
||
| def frequency_notifications_is_realtime?(admins) | ||
| admins.any? { |a| a.notifications_sending_frequency == "realtime" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2/2
Maybe a refactor using something like this could work ? I'm not sure so please don't mind telling me if I'm mistaking
def has_realtime_admins?(organization)
organization.admins.exists?(notifications_sending_frequency: "realtime")
end
…don't have organization.resource_title. So I refacto using translated_attribute
…the hardcoded http protocol
…cted by mailer Decidim
… spec: perform_now without (argument)
|
Hello @BarbaraOliveira13 , thx for your PR !!! I try to run the job I think it may be related to this line https://github.com/OpenSourcePolitics/decidim-app/pull/905/files#diff-236c78d077b1c09f66a2df7f331aefd14b4c5c86155cd2a12f2f2fb42038ff26R53, because decidim_moderations table has no column decidim_organization_id |
Hello @Stef-Rousset Oh, I had the right chain before I think... I should changed too much time :s |
|
Fail CI is not relevant of my changes. It's a flaky CF Redesigned upload modal flaky test #10961 @Stef-Rousset you can re-run the job, I fix it ! 🙏 |
|
Next step:
|
hello @BarbaraOliveira13 , great job, I run the code locally and everything works !!! |
Thanks ! done 👍 |
✉️ Feature: Decidim-AI — Spam Summary Digest Event (Weekly / Daily)
🔧 This PR also fix a CI part by normalize nl local file.
MAIN SUBJECT :

This PR adds a Decidim digest system that aggregates spam reports detected by Decidim-AI and sends a daily or weekly summary email, to each organization’s administrators (selon leur choix de config 'frequency').
It integrates with Decidim EventsManager and EmailEvent notification system.
🧠 Technical Notes
resource: organization ensures Decidim’s BaseEvent behaves correctly
(the spam digest concerns all types of content: proposals, projects, comments, etc.)
If need more details, cf conclusion in the issue
🧭 Steps to Reproduce
(Replace xxx with the password stored in Passbolt)
5. Restart the server:
spring stop && bundle exec rails s10. Create the reporting_user (Spam Bot): script for console:
bundle exec sidekiqExpected output:

📌 Related Issues
https://github.com/orgs/OpenSourcePolitics/projects/26/views/1?filterQuery=assignee%3A%40me&pane=issue&itemId=129336496&issue=OpenSourcePolitics%7Cintern-tasks%7C114
Tasks
📷 Screenshots
Please add screenshots of the changes you're proposing if related to the UI
Extra information
For the DATA Team:⚠️ The digest is not stored in any table — it’s generated dynamically when EventsManager.publish is called.