From c468655b8386d02daec3db2e2bffa6aab4049708 Mon Sep 17 00:00:00 2001 From: jorg-vr Date: Fri, 10 Nov 2023 11:53:39 +0100 Subject: [PATCH 1/2] Do not send emails to github noreply --- app/models/repository.rb | 6 ++++-- test/models/repository_test.rb | 37 ++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index a9d0922ba0..cbcbcae42c 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -117,8 +117,10 @@ def process_activities_email_errors_delayed(kwargs = {}) end def process_activities_email_errors(kwargs = {}) - kwargs[:user] = admins.first if kwargs.empty? && admins.any? - kwargs[:email] = Rails.application.config.dodona_email if kwargs.empty? + has_valid_recipient = -> { kwargs[:user] || kwargs[:email]&.exclude?('@users.noreply.github.com') } + + kwargs[:user] = admins.first if !has_valid_recipient.call && admins.any? + kwargs[:email] = Rails.application.config.dodona_email unless has_valid_recipient.call process_activities rescue AggregatedConfigErrors => e diff --git a/test/models/repository_test.rb b/test/models/repository_test.rb index 66c5d068d6..3c519b7b0d 100644 --- a/test/models/repository_test.rb +++ b/test/models/repository_test.rb @@ -374,10 +374,47 @@ def teardown @remote.copy_dir(@echo.path, new_dir) @remote.commit('copy exercise') + @repository.reset + assert_difference 'ActionMailer::Base.deliveries.size', +1 do + @repository.process_activities_email_errors(email: 'test@foo.bar') + end + + assert_equal 'test@foo.bar', ActionMailer::Base.deliveries.last.to[0] + end + + test 'should not send a mail to github noreply' do + # make sure commit fails + @repository.stubs(:commit).returns([false, ['commit fail']]) + + # add an activity to make sure that commit will be executed inside @repository.process_activities + new_dir = 'echo2' + @remote.copy_dir(@echo.path, new_dir) + @remote.commit('copy exercise') + + @repository.reset + assert_difference 'ActionMailer::Base.deliveries.size', +1 do + @repository.process_activities_email_errors(email: 'dodona@users.noreply.github.com') + end + + assert_equal 'dodona@ugent.be', ActionMailer::Base.deliveries.last.to[0] + end + + test 'should email first admin if present' do + # make sure commit fails + @repository.stubs(:commit).returns([false, ['commit fail']]) + @repository.admins << create(:staff, email: 'staff@foo.bar') + + # add an activity to make sure that commit will be executed inside @repository.process_activities + new_dir = 'echo2' + @remote.copy_dir(@echo.path, new_dir) + @remote.commit('copy exercise') + @repository.reset assert_difference 'ActionMailer::Base.deliveries.size', +1 do @repository.process_activities_email_errors end + + assert_equal 'staff@foo.bar', ActionMailer::Base.deliveries.last.to[0] end test 'has allowed course should filter correctly' do From d6e061b3b6c665a1b6ebb75cf127011867f6d2f5 Mon Sep 17 00:00:00 2001 From: jorg-vr Date: Fri, 10 Nov 2023 14:25:03 +0100 Subject: [PATCH 2/2] Make code more readable --- app/models/repository.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index cbcbcae42c..a0bc27972d 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -117,10 +117,13 @@ def process_activities_email_errors_delayed(kwargs = {}) end def process_activities_email_errors(kwargs = {}) - has_valid_recipient = -> { kwargs[:user] || kwargs[:email]&.exclude?('@users.noreply.github.com') } + recipient_is_invalid = kwargs.empty? || kwargs[:email]&.end_with?('@users.noreply.github.com') - kwargs[:user] = admins.first if !has_valid_recipient.call && admins.any? - kwargs[:email] = Rails.application.config.dodona_email unless has_valid_recipient.call + if recipient_is_invalid && admins.any? + kwargs[:user] = admins.first + elsif recipient_is_invalid + kwargs[:email] = Rails.application.config.dodona_email + end process_activities rescue AggregatedConfigErrors => e