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

Past jobs can still be viewed when an enrichment definition or harvest definition is removed #76

Merged
merged 9 commits into from
Jul 25, 2024
2 changes: 1 addition & 1 deletion app/models/harvest_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ class HarvestJob < ApplicationRecord
belongs_to :pipeline_job
belongs_to :harvest_definition
belongs_to :extraction_job, optional: true
has_one :harvest_report, dependent: :destroy
has_one :harvest_report, dependent: nil
Copy link
Contributor

Choose a reason for hiding this comment

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

fyi standard would agree with you it seems 😄
https://github.com/standardrb/standard-rails/blob/main/config/base.yml#L181


delegate :extraction_definition, to: :harvest_definition
delegate :transformation_definition, to: :harvest_definition
Expand Down
12 changes: 6 additions & 6 deletions app/models/harvest_report.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
# frozen_string_literal: true

class HarvestReport < ApplicationRecord
belongs_to :pipeline_job
belongs_to :harvest_job
belongs_to :pipeline_job, optional: true
belongs_to :harvest_job, optional: true

STATUSES = %w[queued cancelled running completed errored].freeze

Expand All @@ -11,9 +11,9 @@ class HarvestReport < ApplicationRecord
enum :load_status, STATUSES, prefix: :load
enum :delete_status, STATUSES, prefix: :delete

delegate :harvest_definition, to: :harvest_job
delegate :extraction_definition, to: :harvest_job
delegate :transformation_definition, to: :harvest_job
delegate :harvest_definition, to: :harvest_job, allow_nil: true
delegate :extraction_definition, to: :harvest_job, allow_nil: true
delegate :transformation_definition, to: :harvest_job, allow_nil: true

enum :kind, { harvest: 0, enrichment: 1 }

Expand Down Expand Up @@ -97,7 +97,7 @@ def ready_to_delete_previous_records?
private

def considered_cancelled?
statuses.any?('cancelled') || harvest_job.cancelled? || pipeline_job.cancelled?
statuses.any?('cancelled') || harvest_job&.cancelled? || pipeline_job.cancelled?
end

def considered_running?
Expand Down
3 changes: 2 additions & 1 deletion app/sidekiq/harvest_worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ def child_perform(harvest_job)
@pipeline_job = harvest_job.pipeline_job

@harvest_report = HarvestReport.create(pipeline_job: @pipeline_job, harvest_job: @harvest_job,
kind: @harvest_job.harvest_definition.kind)
kind: @harvest_job.harvest_definition.kind,
definition_name: @harvest_job.harvest_definition.name)

if @pipeline_job.extraction_job.nil? || @harvest_job.harvest_definition.enrichment?
create_extraction_job
Expand Down
50 changes: 26 additions & 24 deletions app/views/pipeline_jobs/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
) %>

<tr>
<td><%= report.harvest_job.name %></td>
<td><%= report.definition_name || report.harvest_job.name %></td>
danielaboost marked this conversation as resolved.
Show resolved Hide resolved
<td>
<% if pipeline_job.schedule.present? %>
Schedule
Expand All @@ -88,30 +88,32 @@
<td><%= report.records_rejected %></td>
<td><%= report.records_deleted %></td>
<td>
<i class="bi bi-three-dots-vertical table__control" data-bs-toggle='dropdown'></i>
<ul class="dropdown-menu dropdown-menu-end">
<% if report.harvest_job.extraction_job_id.present? %>
<li>
<%= link_to pipeline_harvest_definition_extraction_definition_extraction_job_path(
@pipeline.id,
report.harvest_definition.id,
report.extraction_definition.id,
report.harvest_job.extraction_job_id
), class: 'dropdown-item' do %>
<i class="bi bi-link-45deg me-2"></i> View Extracted Data
<% end %>
</li>
<% end %>
<% if report.harvest_definition %>
<i class="bi bi-three-dots-vertical table__control" data-bs-toggle='dropdown'></i>
<ul class="dropdown-menu dropdown-menu-end">
<% if report.harvest_job && report.harvest_job.extraction_job_id.present? %>
<li>
<%= link_to pipeline_harvest_definition_extraction_definition_extraction_job_path(
@pipeline.id,
report.harvest_definition.id,
report.extraction_definition.id,
report.harvest_job.extraction_job_id
), class: 'dropdown-item' do %>
<i class="bi bi-link-45deg me-2"></i> View Extracted Data
<% end %>
</li>
<% end %>

<% if report.status == 'running' || report.status == 'queued' %>
<li>
<%= button_to cancel_pipeline_pipeline_job_path(@pipeline.id, report.pipeline_job_id),
class: 'dropdown-item' do %>
<i class="bi bi-x-circle me-2"></i> Cancel job
<% end %>
</li>
<% end %>
</ul>
<% if report.status == 'running' || report.status == 'queued' %>
<li>
<%= button_to cancel_pipeline_pipeline_job_path(@pipeline.id, report.pipeline_job_id),
class: 'dropdown-item' do %>
<i class="bi bi-x-circle me-2"></i> Cancel job
<% end %>
</li>
<% end %>
</ul>
<% end %>
</td>
</tr>
<% else %>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# frozen_string_literal: true

class AddDefinitionNameToHarvestReport < ActiveRecord::Migration[7.1]
def change
add_column :harvest_reports, :definition_name, :string
end
end
3 changes: 2 additions & 1 deletion db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions spec/models/extraction_definition_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,8 @@
expect { subject.destroy }.to change(HarvestJob, :count).by(-1)
end

it "destroys the harvest reports" do
expect { subject.destroy }.to change(HarvestReport, :count).by(-1)
it "does not destroy the harvest reports" do
danielaboost marked this conversation as resolved.
Show resolved Hide resolved
expect { subject.destroy }.to change(HarvestReport, :count).by(0)
end
end

Expand Down
6 changes: 3 additions & 3 deletions spec/models/harvest_definition_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@

context "when a harvest definition has previously been run" do
let!(:destination) { create(:destination) }
let!(:pipeline_job) { create(:pipeline_job, pipeline: pipeline, destination:) }
let!(:pipeline_job) { create(:pipeline_job, pipeline: pipeline, destination:, harvest_definitions_to_run: [harvest_definition.id.to_s]) }
let!(:harvest_job) { create(:harvest_job, :completed, harvest_definition:, pipeline_job:) }
let!(:harvest_report) { create(:harvest_report, pipeline_job:, harvest_job:) }

Expand All @@ -167,8 +167,8 @@
expect { harvest_definition.destroy }.to change(HarvestJob, :count).by(-1)
end

it "destroys the harvest reports" do
expect { harvest_definition.destroy }.to change(HarvestReport, :count).by(-1)
it "does not destroy the harvest reports" do
expect { harvest_definition.destroy }.to change(HarvestReport, :count).by(0)
end
end
end
Expand Down
10 changes: 8 additions & 2 deletions spec/models/harvest_report_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
require 'rails_helper'

RSpec.describe HarvestReport do
subject { create(:harvest_report, pipeline_job:, harvest_job:) }
subject { create(:harvest_report, pipeline_job:, harvest_job:, definition_name: harvest_job.harvest_definition.name) }

let(:pipeline) { create(:pipeline) }
let(:destination) { create(:destination) }
Expand All @@ -12,7 +12,13 @@
let(:harvest_job) { create(:harvest_job, harvest_definition:, pipeline_job:) }

describe 'associations' do
it { is_expected.to belong_to(:pipeline_job) }
it { is_expected.to belong_to(:pipeline_job).optional }
end

describe '#initialize' do
it 'has the same name as its harvest definition' do
expect(subject.definition_name).to eq harvest_definition.name
danielaboost marked this conversation as resolved.
Show resolved Hide resolved
end
end

describe 'status checks' do
Expand Down
Loading