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

Conversation

FelixMarcusMillne
Copy link
Contributor

@FelixMarcusMillne FelixMarcusMillne commented Sep 6, 2023

  • also with additional time and date created fields split from editioned_at to allow for easier data parsing

Making these changes based on a request by the content improvement team to better understand the rate of change of editions in publisher - they need the historical data, not just that of all current editions, so I'm adding in a couple of extra reports for them.

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

Follow these steps if you are doing a Rails upgrade.

@@ -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?

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? 😄

Copy link
Contributor

@sivsubra-tw sivsubra-tw left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification, Felix! Changes look good 👍

@FelixMarcusMillne FelixMarcusMillne merged commit 7368ee7 into main Sep 13, 2023
6 checks passed
@FelixMarcusMillne FelixMarcusMillne deleted the all_edition_reports branch September 13, 2023 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants