-
Notifications
You must be signed in to change notification settings - Fork 25
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
Performance improvement for form responses #1474
Conversation
* Improve loading performance of 'Responses by status' table * Use S3 to store results of async export jobs
@@ -33,31 +33,5 @@ | |||
<%= paginate @events %> | |||
|
|||
<p> | |||
<%= link_to "Export Events to CSV", admin_export_events_url(format: "csv"), class: "usa-button export-btn", target: "_blank", rel: "noopener" %> | |||
<%= link_to "Export Events to CSV", admin_export_events_url, class: "usa-button", target: "_blank", rel: "noopener" %> |
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 know it's pre-existing behavior but it's kind of weird that this opens in a new tab when none of the other exports do.
csv_content = Version.to_csv(versionable) | ||
ActionCable.server.broadcast( |
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.
When I run this locally, this job fails with error:
ArgumentError: wrong number of arguments (given 1, expected 2)
@@ -16,6 +18,17 @@ def notification(title: '', body: '', path: '', emails: []) | |||
to: emails | |||
end | |||
|
|||
def async_report_notification(email:, start_time:, completion_time:, record_count:, url:) |
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.
Ryan, you mentioned making some readability improvements to the email. If you're doing that, I think it would be useful to include the name of the exported file in the email body and/or subject, unless there's some security reason not to.
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.
good point. i've noted this in Trello and we can follow up with feature
* update filename and job options * clear form tag filter * update seeds * return all websites * ensure s3 helper has a timestamp as well * update record_count
* remove opener from export events * indicate export buttons with an icon
No description provided.