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.

Sign up for GitHub

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

Run background reports with Sidekiq, not fork #10615

Merged
merged 1 commit into from
Mar 29, 2023
Merged

Run background reports with Sidekiq, not fork #10615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions app/controllers/admin/reports_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,10 @@ def render_data?

def render_report_as(format)
if OpenFoodNetwork::FeatureToggle.enabled?(:background_reports, spree_current_user)
job = ReportJob.new
JobProcessor.perform_forked(
job,
job = ReportJob.perform_later(
report_class, spree_current_user, params, format
)
sleep 1 until job.done?
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering why this is needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Once the job is done, we want to render it here in the same web request. So we are waiting for the result. That's like the current behaviour. We display the report (or download) when it's ready.

It's planned to send a download link for longer running reports. Then we don't need to wait and can tell the user straight away that the report is processing. But we don't have that yet.


# This result has been rendered by Rails in safe mode already.
job.result.html_safe # rubocop:disable Rails/OutputSafety
Expand Down
29 changes: 0 additions & 29 deletions app/services/job_processor.rb
View file
Open in desktop

This file was deleted.

57 changes: 0 additions & 57 deletions spec/services/job_processor_spec.rb
View file
Open in desktop

This file was deleted.

15 changes: 15 additions & 0 deletions spec/system/admin/reports_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,21 @@
end
end

describe "Background processing" do
before do
Flipper.enable(:background_reports)
ActiveJob::Base.queue_adapter.perform_enqueued_jobs = true
end

it "can run the customers report" do
login_as_admin_and_visit admin_report_path(
report_type: :customers, report_subtype: :mailing_list
)
click_button "Go"
expect(page).to have_content "EMAIL FIRST NAME"
end
end

Comment on lines +34 to +48
Copy link
Member

Choose a reason for hiding this comment

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

Because we're passing report parameters and chosen format to the job, it might be worth testing that these options have been used.
But I wouldn't suggest that prevents us from merging this, this is just a thought maybe for the next iteration.

describe "Can access Customers reports and generate customers report" do
before do
login_as_admin_and_visit admin_reports_path
Expand Down