Correctly show finished budgets at budget index #2369
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
MariaCheca
reviewed
Jan 24, 2018
| @@ -7,9 +7,9 @@ | |||
| <div class="row" data-equalizer data-equalizer-on="medium"> | |||
| <div class="small-12 medium-9 column padding" data-equalizer-watch> | |||
|
|
|||
| <h1><%= @budget.name %></h1> | |||
| <h1><%= current_budget.name %></h1> | |||
There was a problem hiding this comment.
Missing some instance variables @current_budget 😱
MariaCheca
approved these changes
Jan 24, 2018
bertocq
force-pushed
the
fix_finished_budgets_list
branch
from
547970e
to
21a4d24
Compare
Budget's home page has changed, no longer we'll be showing a list of active budgets, but only one current (open) budget and a list of finished ones. So no need to create 3 budgets in a row, but a finished budget (because we already have a valid budget created)
bertocq
force-pushed
the
fix_finished_budgets_list
branch
2 times, most recently
from
bc53862
to
075e2ef
Compare
But only finished budgets should be listed here, not drafting ones, neither current ones.
Drafting budgets will no longer be listed under any conditions at the budget index... not even as "current" budget.
We only need finished budget's at budget's index "Finished budgets" section. So we add the `finished` scope to @budgets variable, and rename it so its clear what it contains. Also avoid showing the "Finished budgets" section if there is none
Clear instance variable names help understand what's going around when you're deep 2 or 3 partials. In this case @Budget is only used to carry around the current_budget so @current_budget is more descriptive. Using `current_budget` directly around would be an alternative, but maybe not as maintainable in case we want to change which budget is being shown (for example the drafting one if you're admin).
bertocq
force-pushed
the
fix_finished_budgets_list
branch
from
075e2ef
to
c01c08a
Compare
clairezed
pushed a commit
to CDJ11/CDJ
that referenced
this pull request
Jun 26, 2018
…ets_list Correctly show finished budgets at budget index
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.
What
Currently we're showing all budgets at the "Finished Budgets" section of the Budget's index page. That's not right, only finished ones should be listed there!
How
As a bonus: renaming @Budget usage to @current_budget for extra descriptive variable name 547970e
Screenshots
If you run
rake db:dev_seedsyou'll be able to see both finished budgets listed, but not the accepting one:Test
I started by refactoring the budget feature spec as we're no longer listing all budget's but only finished ones fe90ecb
Then added expectations to see the finished budget at the list but not the current or drafting ones 4e854c0
Removed Drafting budget scenarios no longer valid b24bb2d
And finally made rubocop 🤖 smile by making the file 100 linelength compliant 59e5854
Deployment
As usual
Warnings
Maybe too much refactoring for just 4 lines of code, right? But I'm doing the boyscout here and cleaning as I pass by... Should we start doing separate-but-related PR's to make important changes easier to read? I tried to with the commit history and PR description 🙇