Add submission.graded developer event (pupilfirst#643)

Also make it possible to "not" handle webhook delivery for newly added
developer events.

Co-authored-by: Mirosław Pragłowski <[email protected]>

app/jobs/webhook_deliveries/deliver_job.rb

@@ -9,7 +9,11 @@ def perform(event_type, course, resource)
         raise "#{event_type} was not one of the requested events in WebhookEndpoint##{webhook_endpoint.id}"
       end
 
-      payload = { data: data(event_type, resource), event: event_type }
+      payload_data = data(event_type, resource)
+
+      return if payload_data.blank?
+
+      payload = { data: payload_data, event: event_type }
 
       uri = URI.parse(webhook_endpoint.webhook_url)
 
@@ -48,10 +52,15 @@ def perform(event_type, course, resource)
 
     def data(event_type, resource)
       case event_type
-      when WebhookDelivery.events[:submission_created]
+      when WebhookDelivery.events[:submission_created],
+           WebhookDelivery.events[:submission_graded]
         TimelineEvents::CreateWebhookDataService.new(resource).data
       else
-        raise "Unknown webhook event type: #{event_type}"
+        Rails.logger.error(
+          "Could not find a data service for event #{event_type}"
+        )
+
+        nil
       end
     end
 

app/models/webhook_delivery.rb

@@ -1,5 +1,9 @@
 class WebhookDelivery < ApplicationRecord
   belongs_to :course
 
-  enum event: { submission_created: "submission.created" }
+  enum event: {
+         submission_created: 'submission.created',
+         submission_graded: 'submission.graded',
+         noop: 'noop' # special event, which does not require webhook delivery
+       }
 end

app/queries/concerns/developers_notifications.rb

@@ -0,0 +1,11 @@
+module DevelopersNotifications
+  def publish(course, event_type, actor, resource)
+    notification_service.execute(course, event_type, actor, resource)
+  end
+
+  private
+
+  def notification_service
+    Developers::NotificationService.new
+  end
+end

app/queries/create_grading_mutator.rb

@@ -1,5 +1,6 @@
 class CreateGradingMutator < ApplicationQuery
   include AuthorizeCoach
+  include DevelopersNotifications
 
   property :submission_id, validates: { presence: { message: 'Submission ID is required for grading' } }
   property :feedback, validates: { length: { maximum: 10_000 } }
@@ -35,6 +36,8 @@ def grade
       update_coach_note if note.present?
       send_feedback if feedback.present?
     end
+
+    publish(course, :submission_graded, current_user, submission)
   end
 
   private

app/services/timeline_events/create_webhook_data_service.rb

@@ -17,11 +17,28 @@ def data
           evaluation_criteria: evaluation_criteria
         },
         files: files
-      }
+      }.merge(evaluation)
     end
 
     private
 
+    def evaluation
+      return {} if @submission.pending_review?
+
+      {
+        evaluator: @submission.evaluator.name,
+        evaluated_at: @submission.evaluated_at,
+        grades:
+          @submission
+            .timeline_event_grades
+            .each_with_object({}) do |submission_grade, evaluation|
+              evaluation[submission_grade.evaluation_criterion_id] =
+                submission_grade.grade
+              evaluation
+            end
+      }
+    end
+
     def target
       @target ||= @submission.target
     end
@@ -40,7 +57,11 @@ def evaluation_criteria
     def files
       @submission.timeline_event_files.map do |timeline_event_file|
         file = timeline_event_file.file
-        file_path = Rails.application.routes.url_helpers.rails_blob_path(file, only_path: true)
+        file_path =
+          Rails.application.routes.url_helpers.rails_blob_path(
+            file,
+            only_path: true
+          )
 
         {
           filename: file.filename.to_s,

spec/jobs/webhook_deliveries/deliver_job_spec.rb

@@ -73,5 +73,17 @@
         )
       end
     end
+
+    context 'for events without a corresponding data service' do
+      let!(:event_type) { 'noop' }
+
+      it 'will not attempt delivery' do
+        expect {
+          subject.perform_now(event_type, course, submission)
+        }.not_to raise_exception
+
+        expect(WebhookDelivery.count).to eq(0)
+      end
+    end
   end
 end

spec/services/timeline_events/create_webhook_data_service_spec.rb

@@ -6,10 +6,16 @@
   let(:level) { create :level, course: course }
   let(:target_group) { create :target_group, level: level }
   let(:criterion) { create :evaluation_criterion, course: course }
-  let!(:target) { create :target, target_group: target_group, evaluation_criteria: [criterion] }
+  let!(:target) do
+    create :target, target_group: target_group, evaluation_criteria: [criterion]
+  end
   let(:submission) { create :timeline_event, target: target }
   let!(:pdf_file) { create :timeline_event_file, timeline_event: submission }
-  let!(:png_file) { create :timeline_event_file, file_path: 'files/icon_pupilfirst.png', timeline_event: submission }
+  let!(:png_file) do
+    create :timeline_event_file,
+           file_path: 'files/icon_pupilfirst.png',
+           timeline_event: submission
+  end
 
   describe '#data' do
     it 'returns data appropriate for sending via webhook' do
@@ -26,21 +32,23 @@
         ]
       }
 
-      pdf_file_data = hash_including(
-        filename: 'pdf-sample.pdf',
-        content_type: 'application/pdf',
-        byte_size: 7945,
-        checksum: '+n1+ZQss7GjzArMbooI12A==',
-        url: %r{https://test\.host/rails/active_storage/blobs/.*/pdf-sample\.pdf}
-      )
-
-      image_file_data = hash_including(
-        filename: 'icon_pupilfirst.png',
-        content_type: 'image/png',
-        byte_size: 10026,
-        checksum: 'm5ZqQ7BpvaojhnIlEkoRiQ==',
-        url: %r{https://test\.host/rails/active_storage/blobs/.*/icon_pupilfirst\.png}
-      )
+      pdf_file_data =
+        hash_including(
+          filename: 'pdf-sample.pdf',
+          content_type: 'application/pdf',
+          byte_size: 7945,
+          checksum: '+n1+ZQss7GjzArMbooI12A==',
+          url: %r{https://test\.host/rails/active_storage/blobs/.*/pdf-sample\.pdf}
+        )
+
+      image_file_data =
+        hash_including(
+          filename: 'icon_pupilfirst.png',
+          content_type: 'image/png',
+          byte_size: 10_026,
+          checksum: 'm5ZqQ7BpvaojhnIlEkoRiQ==',
+          url: %r{https://test\.host/rails/active_storage/blobs/.*/icon_pupilfirst\.png}
+        )
 
       data = subject.data
 
@@ -53,5 +61,26 @@
       expect(data[:files]).to include(pdf_file_data)
       expect(data[:files]).to include(image_file_data)
     end
+
+    context 'when the submission has been graded' do
+      let(:submission) { create :timeline_event, :evaluated, target: target }
+
+      let!(:submission_grading) do
+        create :timeline_event_grade,
+               evaluation_criterion: criterion,
+               timeline_event: submission
+      end
+
+      it 'includes grades in the response' do
+        data = subject.data
+
+        expect(data[:grades]).to eq(
+          { criterion.id => submission_grading.grade }
+        )
+
+        expect(data[:evaluator]).to eq(submission.evaluator.name)
+        expect(data[:evaluated_at]).to eq(submission.evaluated_at)
+      end
+    end
   end
 end

spec/spec_helper.rb

@@ -38,6 +38,7 @@
     # a real object. This is generally recommended, and will default to
     # `true` in RSpec 4.
     mocks.verify_partial_doubles = true
+    mocks.verify_doubled_constant_names = true
   end
 
   # With this configuration, the :focus filtering will only apply if any

spec/support/spec_helpers/developers_notifications_helper.rb

@@ -0,0 +1,22 @@
+module DevelopersNotificationsHelper
+  def prepare_developers_notification
+    notification_service = instance_double('Developers::NotificationService')
+    allow(notification_service).to receive(:execute).with(
+      an_instance_of(Course),
+      kind_of(Symbol),
+      an_instance_of(User),
+      kind_of(ApplicationRecord)
+    )
+    allow(Developers::NotificationService).to receive(:new).and_return(notification_service)
+    notification_service
+  end
+
+  def expect_published(notification_service, course, event_type, actor, resource)
+    expect(notification_service).to have_received(:execute).with(
+      course,
+      event_type,
+      actor,
+      resource
+    )
+  end
+end

spec/system/submissions/review_spec.rb

@@ -5,6 +5,7 @@
   include MarkdownEditorHelper
   include NotificationHelper
   include SubmissionsHelper
+  include DevelopersNotificationsHelper
 
   let(:school) { create :school, :current }
   let(:course) { create :course, school: school }
@@ -62,6 +63,8 @@
     end
 
     scenario 'coach evaluates a pending submission and gives a feedback' do
+      notification_service = prepare_developers_notification
+
       sign_in_user coach.user, referrer: review_course_path(course)
 
       expect(page).to have_content(target.title)
@@ -121,6 +124,8 @@
       find("button[aria-label='submissions-overlay-close']").click
       expect(page).to have_text(submission_pending_2.target.title)
       expect(page).to_not have_text(submission.target.title)
+
+      expect_published(notification_service, course, :submission_graded, coach.user, submission)
     end
 
     scenario 'coach generates feedback from review checklist' do