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

add all edition reports for content workflow and edition churn #1917

Merged
merged 4 commits into from
Sep 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions app/controllers/reports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,18 @@ def edition_churn
redirect_to Report.new("edition_churn").url, allow_other_host: true
end

def all_edition_churn
redirect_to Report.new("all_edition_churn").url, allow_other_host: true
end

def content_workflow
redirect_to Report.new("content_workflow").url, allow_other_host: true
end

def all_content_workflow
redirect_to Report.new("all_content_workflow").url, allow_other_host: true
end

def all_urls
redirect_to Report.new("all_urls").url, allow_other_host: true
end
Expand Down
39 changes: 39 additions & 0 deletions app/presenters/all_content_workflow_presenter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
class AllContentWorkflowPresenter < CSVPresenter
def initialize(scope = Edition.all)
super(scope)
self.column_headings = %i[
content_title
content_slug
content_url
current_status
stage
format
current_assignee
created_at
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to have date_created and time_created in the initialize function when we have created_at (assuming we keep all 3 in the build_csv method)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was explicitly requested by those who are consuming the data - the better thing to do in my mind would be to remove the created_at field, but I left it in there for consistency with the other reports 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be overbearingly explicit, the column_headings array in the initialize function need to match perfectly with what we create in the build_csv function (as this all needs to line up properly in the right order on the csv file). So, if we add or remove something from the first array then we need to do the same in the one below, yeah? 😄

date_created
time_created
]
end

private

def build_csv(csv)
csv << column_headings.collect { |ch| ch.to_s.humanize }
scope.each do |item|
item.actions.each do |action|
csv << [
item.title,
item.slug,
"#{Plek.website_root}/#{item.slug}",
item.state,
action.request_type,
item.format,
item.assignee,
action.created_at.to_fs(:db),
action.created_at.to_date.to_s,
action.created_at.to_fs(:time),
]
end
end
end
end
33 changes: 33 additions & 0 deletions app/presenters/all_edition_churn_presenter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
class AllEditionChurnPresenter < CSVPresenter
def initialize(scope = Edition.all)
super(scope)
self.column_headings = %i[
id
panopticon_id
name
slug
state
editioned_on
version_number
date_created
time_created
]
end

private

def get_value(header, edition)
case header
when :name
edition.title
when :editioned_on
edition.created_at.iso8601
when :date_created
edition.created_at.to_date.to_s
when :time_created
edition.created_at.to_fs(:time)
else
super
end
end
end
10 changes: 9 additions & 1 deletion app/views/reports/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,22 @@
<strong><%= link_to 'Churn in non-archived editions', edition_churn_report_path(format: :csv) %></strong><br />
<%= report_last_updated("edition_churn")%>
</p>
<p>
<strong><%= link_to 'Churn in all editions', all_edition_churn_report_path(format: :csv) %></strong><br />
<%= report_last_updated("all_edition_churn")%>
</p>
<p>
<strong><%= link_to 'Progress on all non-archived editions', progress_report_path(format: :csv) %></strong><br />
<%= report_last_updated("editorial_progress")%>
</p>
<p>
<strong><%= link_to 'Content summary and workflow history', content_workflow_report_path(format: :csv) %></strong><br />
<strong><%= link_to 'Content summary and workflow history for all published editions', all_content_workflow_report_path(format: :csv) %></strong><br />
<%= report_last_updated("content_workflow")%>
</p>
<p>
<strong><%= link_to 'Content summary and workflow history for all editions', content_workflow_report_path(format: :csv) %></strong><br />
<%= report_last_updated("all_content_workflow")%>
</p>
<p>
<strong><%= link_to 'All URLs', all_urls_report_path(format: :csv) %></strong><br />
<%= report_last_updated("all_urls")%>
Expand Down
54 changes: 50 additions & 4 deletions config/brakeman.ignore
Original file line number Diff line number Diff line change
@@ -1,13 +1,36 @@
{
"ignored_warnings": [
{
"warning_type": "Redirect",
"warning_code": 18,
"fingerprint": "2cb25751c6a39d8b813d792d58ba4f1fe596aa00d263284ffe5ecef8a546c88b",
"check_name": "Redirect",
"message": "Possible unprotected redirect",
"file": "app/controllers/reports_controller.rb",
"line": 21,
"link": "https://brakemanscanner.org/docs/warning_types/redirect/",
"code": "redirect_to(Report.new(\"all_edition_churn\").url, :allow_other_host => true)",
"render_path": null,
"location": {
"type": "method",
"class": "ReportsController",
"method": "all_edition_churn"
},
"user_input": "Report.new(\"all_edition_churn\").url",
"confidence": "High",
"cwe_id": [
601
],
"note": ""
},
{
"warning_type": "Redirect",
"warning_code": 18,
"fingerprint": "4de24c49b5d87e19ef510a17b4952468eaac4f2bfdcf3d5a67507cf694c25c93",
"check_name": "Redirect",
"message": "Possible unprotected redirect",
"file": "app/controllers/reports_controller.rb",
"line": 25,
"line": 33,
"link": "https://brakemanscanner.org/docs/warning_types/redirect/",
"code": "redirect_to(Report.new(\"all_urls\").url, :allow_other_host => true)",
"render_path": null,
Expand All @@ -23,6 +46,29 @@
],
"note": "This is a redirect to AWS S3 for file download, unlikely to be dangerous."
},
{
"warning_type": "Redirect",
"warning_code": 18,
"fingerprint": "605ea82aeb307735d184ae89ae7300b2571376340aa1cd7d48c1c79232f6e7d2",
"check_name": "Redirect",
"message": "Possible unprotected redirect",
"file": "app/controllers/reports_controller.rb",
"line": 29,
"link": "https://brakemanscanner.org/docs/warning_types/redirect/",
"code": "redirect_to(Report.new(\"all_content_workflow\").url, :allow_other_host => true)",
"render_path": null,
"location": {
"type": "method",
"class": "ReportsController",
"method": "all_content_workflow"
},
"user_input": "Report.new(\"all_content_workflow\").url",
"confidence": "High",
"cwe_id": [
601
],
"note": ""
},
{
"warning_type": "Dynamic Render Path",
"warning_code": 15,
Expand Down Expand Up @@ -107,7 +153,7 @@
"check_name": "Redirect",
"message": "Possible unprotected redirect",
"file": "app/controllers/reports_controller.rb",
"line": 21,
"line": 25,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth keeping these ordered by line number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is autogenerated by brakeman - I suppose we could, but it'll mean constantly fighting against the tool to do it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given it's not really for human consumption I'd prefer to leave it. Thoughts?

"link": "https://brakemanscanner.org/docs/warning_types/redirect/",
"code": "redirect_to(Report.new(\"content_workflow\").url, :allow_other_host => true)",
"render_path": null,
Expand Down Expand Up @@ -204,6 +250,6 @@
"note": "This is a redirect to AWS S3 for file download, unlikely to be dangerous."
}
],
"updated": "2023-04-20 10:50:54 +0100",
"brakeman_version": "5.4.0"
"updated": "2023-09-07 15:54:38 +0100",
"brakeman_version": "5.3.1"
}
4 changes: 3 additions & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@
get "reports/progress" => "reports#progress", as: :progress_report
get "reports/organisation-content" => "reports#organisation_content", :as => :organisation_content_report
get "reports/edition-churn" => "reports#edition_churn", as: "edition_churn_report"
get "reports/content_workflow" => "reports#content_workflow", as: "content_workflow_report"
get "reports/all-edition-churn" => "reports#all_edition_churn", as: "all_edition_churn_report"
get "reports/content-workflow" => "reports#content_workflow", as: "content_workflow_report"
get "reports/all-content-workflow" => "reports#all_content_workflow", as: "all_content_workflow_report"
get "reports/all-urls" => "reports#all_urls", as: "all_urls_report"

get "user_search" => "user_search#index"
Expand Down
6 changes: 6 additions & 0 deletions lib/csv_report_generator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,18 @@ def presenters
Edition.not_in(state: %w[archived]).order(created_at: 1),
),

AllEditionChurnPresenter.new(
Edition.all.order(created_at: 1),
),

OrganisationContentPresenter.new(
Artefact.where(owning_app: "publisher").not_in(state: %w[archived]),
),

ContentWorkflowPresenter.new(Edition.published.order(created_at: :desc)),

AllContentWorkflowPresenter.new(Edition.all.order(created_at: :desc)),

AllUrlsPresenter.new(
Artefact.where(owning_app: "publisher").not_in(state: %w[archived]),
),
Expand Down
10 changes: 6 additions & 4 deletions test/unit/csv_report_generator_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,16 @@ class CsvReportGeneratorTest < ActiveSupport::TestCase
@generator.run!
end

assert_equal 5, @stubbed_s3_client.api_requests.size
assert_equal 7, @stubbed_s3_client.api_requests.size
assert(@stubbed_s3_client.api_requests.all? { |r| r[:operation_name] == :put_object })
assert(@stubbed_s3_client.api_requests.all? { |r| r[:params][:bucket] == "example" })

assert_equal "editorial_progress.csv", @stubbed_s3_client.api_requests[0][:params][:key]
assert_equal "edition_churn.csv", @stubbed_s3_client.api_requests[1][:params][:key]
assert_equal "organisation_content.csv", @stubbed_s3_client.api_requests[2][:params][:key]
assert_equal "content_workflow.csv", @stubbed_s3_client.api_requests[3][:params][:key]
assert_equal "all_urls.csv", @stubbed_s3_client.api_requests[4][:params][:key]
assert_equal "all_edition_churn.csv", @stubbed_s3_client.api_requests[2][:params][:key]
assert_equal "organisation_content.csv", @stubbed_s3_client.api_requests[3][:params][:key]
assert_equal "content_workflow.csv", @stubbed_s3_client.api_requests[4][:params][:key]
assert_equal "all_content_workflow.csv", @stubbed_s3_client.api_requests[5][:params][:key]
assert_equal "all_urls.csv", @stubbed_s3_client.api_requests[6][:params][:key]
end
end