Add options to show advanced stats #3520
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
houndci-bot
reviewed
May 21, 2019
| @@ -62,7 +63,7 @@ def destroy | |||
| def budget_params | |||
| descriptions = Budget::Phase::PHASE_KINDS.map{|p| "description_#{p}"}.map(&:to_sym) | |||
| valid_attributes = [:phase, :currency_symbol] + descriptions | |||
| params.require(:budget).permit(*valid_attributes, translation_params(Budget)) | |||
| params.require(:budget).permit(*valid_attributes, *report_attributes, translation_params(Budget)) | |||
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [103/100] (https://github.com/bbatsov/ruby-style-guide#80-character-limits)
javierm
force-pushed
the
backport-remove_custom_poll_group_file
branch
from
98b4a9e
to
d4017a6
Compare
javierm
force-pushed
the
backport-refactor_stats_enabled
branch
from
dded561
to
d151805
Compare
javierm
force-pushed
the
backport-remove_custom_poll_group_file
branch
from
d4017a6
to
2d29243
Compare
This table will store which reports (stats, results, ...) will be shown for a certain process (polls, budgets, ...). Note Rails fails to save a poll and its report when both are new records if we add a `validate :process, presence: true` rule. Since it caused a lot of trouble when creating records for tests during factories rule completely. Instead, I've created the `results_enabled=` and `stats_enabled=` methods, so tests are easier to set up, while also automatically creating a report if it doesn't already exist. This also decouples form structure and database implemenation. Originally I named this table `enabled_reports` and instead of having `stats` and `results` columns, it had an `enabled` column and a `kind` column, which would be set to "stats" or "results". However, although that table would allow us to add arbitrary reports easily, I found the way we had to handle the `has_many` relationship was a bit too complex.
We're going to add an `advanced_stats` report, and having 3 identical sets of methods would be too much duplication.
They're easier to test and to read in the model.
javierm
force-pushed
the
backport-refactor_stats_enabled
branch
from
d151805
to
13e68e4
Compare
javierm
force-pushed
the
backport-refactor_stats_enabled
branch
from
13e68e4
to
9ae0cbb
Compare
voodoorai2000
approved these changes
May 23, 2019
javierm
changed the base branch from
backport-remove_custom_poll_group_file
to
master
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
Objectives
Visual Changes
Release notes