Change concept of current budget to account for multiple budgets #2322

voodoorai2000 · 2018-01-15T17:47:51Z

Where

Related Issue: Add "Current Budget" concept #2317

What

Add currrent_budget helper for controllers and views
The current_budget helper takes into account that the current budget is the last budget created
It also takes into account, that it's possible to have a budget in drafting phase, in which case, the current_budget is really the previous budget if it exists or nil otherwise
Refactor Management and Valuator controllers and views to display only the current_budget
Add scope Budget.open to represent, budgets that are not finished. This substitutes the old Budget.current implementation and it's used only in the admin budget's index view, which has two tabs open budgets and finished_budgets (see screenshot)

How

Adding a private method current_budget to the ApplicationController and making it accessible to views
Refactoring existing concept of Budget.current

Screenshots

Admin budget's index

Test

Added unit and controller specs for current_budget and Budget.open

Deployment

As usual

Warnings

None

bertocq

💯 👏 for that helper_method at the app controller 👌 🙇

When there was only one budget this implementation worked fine Nowadays there can be multiple budgets, and therefore the definition of the current_budget has changed. It is no longer a budget that has not finished, but rather, the last budget created that is not in the initial drafting phase. Budgets in the drafting phase are not considered the current_budget, but rather a budget that is still being prepared and that soon will become the current_budget

Before Budget.current could return multiple budgets, now there can only be a single current_budget. Adding the concept of open, which better reflects what the admin sees in this page: A tab for open budgets and a tab for finished budgets

In the specs, some investment were missing a heading_id, thus creating another unexpected budget By explicitly setting the heading_id we can control better which budgets are created in each test

Before we could have multiple current budgets, as we now only have one current_budget, some specs broke. As there is no need to display multiple budgets to Valuators, only the current budget is necessary, we can remove arrays and assume that only a single budget, the current budget, is displayed to Valuators

This spec used to pass, because even though there were no budgets, as Budget.current returned an array, it gracefully handled situations without budgets Now we assume that there can only be a single current budget, and so calling any method of budget will raise an exception unless there is a current budget present Valuators should not access this page when there is no budget present, however it might be wise to create an issue to cover this case, just in case

bertocq · 2018-01-15T23:34:02Z

app/models/budget.rb

@@ -30,8 +30,11 @@ class Budget < ActiveRecord::Base
 scope :balloting, -> { where(phase: "balloting") }
 scope :reviewing_ballots, -> { where(phase: "reviewing_ballots") }
 scope :finished, -> { where(phase: "finished") }
+ scope :open, -> { where.not(phase: "finished") }


I wonder if we should exclude as well 'drafting' budgets, as those should be totally hidden to the public.

I know 'drafting' phase wasn't included in this PR (but in one that I created) but just realized/discovered about this filter and that we may not be doing it right, and maybe would be a better idea to address this on a separate issue & PR if my assumptions are correct. Is that so?

Interesting, it makes sense at a code level, however at the interface level, it's not yet aplicable. As this scope is only used in the admin interface. Where there are only 2 tabs: open and close, which I think fits better with the current definition of open.

Maybe we can review this when we have a need to extend the budget's admin interface or the user facing interface due to having more budgets in different phases?

bertocq · 2018-01-15T23:37:34Z

app/views/management/budgets/print_investments.html.erb

- <tr id="<%= dom_id(budget) %>">
- <td><%= budget.name %></td>
- <td><%= budget.translated_phase %></td>
+ <tr id="<%= dom_id(@budget) %>">


I just realized that we may not be covering the scenario where @budget may not be present (may be nil). I'm thinking about new clean installations without a proper database seed, or that have cleaned up "dummy data" from seeds.

Same could happen at the file app/views/valuation/budgets/index.html.erb changed in this PR

I realize that we weren't covering that scenario neither before, and that we may just should open a new issue to address it instead of extending this one... what do you think?

Yes :) +1 to an issue for this 👌
These are the tests that where failing due to not having a budget, it would be a great starting point to implementa that issue 👍
34e0c23

bertocq

Great refactor! Although I've the following two questions, that may or not be valid, and we could be fixing in a second/third PR as it would be fixing stuff a bit out of the scope of this one's objective

bertocq

👏 so great to finally have this :) great work

voodoorai2000 · 2018-01-16T15:53:05Z

💃

Change concept of current budget to account for multiple budgets

bertocq approved these changes Jan 15, 2018

View reviewed changes

voodoorai2000 added 2 commits January 15, 2018 19:13

Make current_budget accessible in controller and views

be554a6

voodoorai2000 force-pushed the current_budget branch 3 times, most recently from 6b754c1 to c9d531d Compare January 15, 2018 20:49

voodoorai2000 added 4 commits January 15, 2018 22:01

Add Budget.open scope

3497809

Before Budget.current could return multiple budgets, now there can only be a single current_budget. Adding the concept of open, which better reflects what the admin sees in this page: A tab for open budgets and a tab for finished budgets

Display only current budget to Managers printing investments

01ef439

In the specs, some investment were missing a heading_id, thus creating another unexpected budget By explicitly setting the heading_id we can control better which budgets are created in each test

voodoorai2000 force-pushed the current_budget branch from c9d531d to 34e0c23 Compare January 15, 2018 21:02

bertocq reviewed Jan 15, 2018

View reviewed changes

bertocq approved these changes Jan 16, 2018

View reviewed changes

voodoorai2000 merged commit a215c9c into master Jan 16, 2018

voodoorai2000 deleted the current_budget branch January 16, 2018 15:53

voodoorai2000 restored the current_budget branch January 16, 2018 15:54

voodoorai2000 mentioned this pull request Jan 16, 2018

Change concept of current budget to account for multiple budgets AyuntamientoMadrid/consul#1118

Merged

voodoorai2000 deleted the current_budget branch January 16, 2018 16:02

voodoorai2000 restored the current_budget branch January 16, 2018 16:08

voodoorai2000 mentioned this pull request Jan 16, 2018

Add current_budget to management controllers #2331

Merged

bertocq mentioned this pull request Jan 17, 2018

Add "Current Budget" concept #2317

Closed

bertocq deleted the current_budget branch January 31, 2018 21:24

clairezed pushed a commit to CDJ11/CDJ that referenced this pull request Jun 26, 2018

Merge pull request consuldemocracy#2322 from consul/current_budget

e851dcc

Change concept of current budget to account for multiple budgets

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change concept of current budget to account for multiple budgets #2322

Change concept of current budget to account for multiple budgets #2322

voodoorai2000 commented Jan 15, 2018 •

edited

Loading

bertocq left a comment

bertocq Jan 15, 2018 •

edited

Loading

voodoorai2000 Jan 16, 2018

bertocq Jan 15, 2018 •

edited

Loading

voodoorai2000 Jan 16, 2018

bertocq left a comment

bertocq left a comment

voodoorai2000 commented Jan 16, 2018

Change concept of current budget to account for multiple budgets #2322

Change concept of current budget to account for multiple budgets #2322

Conversation

voodoorai2000 commented Jan 15, 2018 • edited Loading

Where

What

How

Screenshots

Test

Deployment

Warnings

bertocq left a comment

Choose a reason for hiding this comment

bertocq Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

voodoorai2000 Jan 16, 2018

Choose a reason for hiding this comment

bertocq Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

voodoorai2000 Jan 16, 2018

Choose a reason for hiding this comment

bertocq left a comment

Choose a reason for hiding this comment

bertocq left a comment

Choose a reason for hiding this comment

voodoorai2000 commented Jan 16, 2018

voodoorai2000 commented Jan 15, 2018 •

edited

Loading

bertocq Jan 15, 2018 •

edited

Loading

bertocq Jan 15, 2018 •

edited

Loading