So now:
* In the first few phases, no filters are shown (just like before)
* During the valuation phase, we show "Active" and "Unfeasible"
* During the final voting, we show "Active" (which now refers to the
selected investments), "Not selected for the final voting" and
"Unfeasible"
* When the budget is finished, we show "Winners", "Not selected for the
final voting" and "Unfeasible"
Now each investment is shown in one (and only one) of the filters
(except when the budget is finished; in this case we don't show selected
investments which didn't win), and we remove the confusing "Not
unfeasible" filter by only showing it during the valuation phase (before
filters are selected) and renaming it to "Active". We also rearrange the
filters so the default one for each phase is shown first.
The idea of using the "Active" text for investments which can be
selected during the selection phase and voted during the final voting is
experimental. Right now, for simplicity, since we assume filters will
always use the same text, we're removing the "Active" filter when the
budget is finished, since having both "Winners" and "Active" filters
would be confusing.
Before the "valuating" phase, all investments have undecided feasibility
and none have been selected, so the filters would return no results
(except the "not_unfeasible" one, which would return everything).
We removed it in commit c322b2c4a because it was hard to know the
difference between "Feasible" and "Not unfeasible". We're renaming the
"Not unfeasible" filter instead.
We're also moving the "selected" filter so it appears before the
"unselected" filter, just like the "feasible" filter appears before the
"unfeasible" filter.
This rule was added in rubocop-rails 2.11.0.
Although we prevent I18n locale leaking between tests by setting it
before each test, the `with_locale` method makes the scope of the locale
change more obvious.
- Allow to define a link (text and url) on budget form for render on the budget
header.
- Improve styles
Co-authored-by: Senén Rodero Rodríguez <senenrodero@gmail.com>
We don't allow deleting a budget with associated investments. However,
we allow deleting a budget with associated administrators and valuators.
This results in a foreign key violation error:
PG::ForeignKeyViolation: ERROR: update or delete on table "budgets"
violates foreign key constraint "fk_rails_c847a52b1d" on table
"budget_administrators"
Using the `dependent: :destroy` option when defining the relationship,
we remove the association records when removing the budget.
As a bonus, we reduce the number of Rubocop offenses regarding the
`Rails/HasManyOrHasOneDependent` rule. Only 72 to go! :)
Previously the draft mode was a phase of the PB, but that had some
limitations.
Now the phase drafting disappears and therefore the PB can have the
status published or not published (in draft mode).
That will give more flexibility in order to navigate through the
different phases and see how it looks for administrators before
publishing the PB and everybody can see.
By default, the PB is always created in draft mode, so it gives you
the flexibility to adjust and modify anything before publishing it.
When defining abilities, scopes cover more cases because they can be
used to check permissions for a record and to filter a collection. Ruby
blocks can only be used to check permissions for a record.
Note the `Budget::Phase.kind_or_later` name sounds funny, probably
because we use the word "phase" for both an an attribute in the budgets
table and an object associated with the budget, and so naming methods
for a budget phase is a bit tricky.
Since budgets now have milestone tags, the name of this method was
confusing and will conflict with the name generated by acts_as_taggable.
Note the new name could be improved too.
We were very inconsistent regarding these rules.
Personally I prefer no empty lines around blocks, clases, etc... as
recommended by the Ruby style guide [1], and they're the default values
in rubocop, so those are the settings I'm applying.
The exception is the `private` access modifier, since we were leaving
empty lines around it most of the time. That's the default rubocop rule
as well. Personally I don't have a strong preference about this one.
[1] https://rubystyle.guide/#empty-lines-around-bodies
Having exceptions is better than having silent bugs.
There are a few methods I've kept the same way they were.
The `RelatedContentScore#score_with_opposite` method is a bit peculiar:
it creates scores for both itself and the opposite related content,
which means the opposite related content will try to create the same
scores as well.
We've already got a test to check `Budget::Ballot#add_investment` when
creating a line fails ("Edge case voting a non-elegible investment").
Finally, the method `User#send_oauth_confirmation_instructions` doesn't
update the record when the email address isn't already present, leading
to the test "Try to register with the email of an already existing user,
when an unconfirmed email was provided by oauth" fo fail if we raise an
exception for an invalid user. That's because updating a user's email
doesn't update the database automatically, but instead a confirmation
email is sent.
There are also a few false positives for classes which don't have bang
methods (like the GraphQL classes) or destroying attachments.
For these reasons, I'm adding the rule with a "Refactor" severity,
meaning it's a rule we can break if necessary.
Sanitizing descriptions before saving a record has a few drawbacks:
1. It makes the application rely on data being safe in the database. If
somehow dangerous data enters the database, the application will be
vulnerable to XSS attacks
2. It makes the code complicated
3. It isn't backwards compatible; if we decide to disallow a certain
HTML tag in the future, we'd need to sanitize existing data.
On the other hand, sanitizing the data in the view means we don't need
to triple-check dangerous HTML has already been stripped when we see the
method `auto_link_already_sanitized_html`, since now every time we use
it we sanitize the text in the same line we call this method.
We could also sanitize the data twice, both when saving to the database
and when displaying values in the view. However, doing so wouldn't make
the application safer, since we sanitize text introduced through
textarea fields but we don't sanitize text introduced through input
fields.
Finally, we could also overwrite the `description` method so it
sanitizes the text. But we're already introducing Globalize which
overwrites that method, and overwriting it again is a bit too confusing
in my humble opinion. It can also lead to hard-to-debug behaviour.
Assigning a variable to each budget we declare results in useless
assignments. We could just delete the three useless assignments and
leave the fourth one, but I find the code easier to read if we use the
name of the budgets to differenciate between them. This way we also keep
the code vertically aligned.
So now we test in depth at the model level, and can be a bit more
relaxed about integration tests for translations.
Note we're defining some extra factories to make sure all translatable
attributes with presence validation rules are mandatory. This way we can
simplify the way we obtain required fields, using `required_attribute?`.
Otherwise, fields having an `unless` condition in their presence
validation rules would count as mandatory even when they're not.
Now factories define default headings for investments, so there's no
need to create a group and a heading to create an investment.
Likewise, in order to create a heading it isn't necessary to specify a
group anymore; specifying the budget is enough.
It's possible that there are more similar cases we haven't simplified
yet; I'm only addressing the obvious ones.
We already configure `I18n.locale` and we reset Globalize's fallbacks
before every test.
On the other hand, RSpec automatically resets anything which is stub
with `allow`, so there's no need to use `and_call_original` in an
`after` block.
This implementation is a bit more robust because we don't have to change
any of the "or_later?" methods if we add or remove a new phase.
We could also use metaprogramming to reduce code duplication in these
methods. So far, I've decided to keep the code simple since the
duplication seems reasonable.
Slugs should only be updated on certain conditions, we need a trait that
meets that conditions and the name of the trait passed as a mandatory &
named argument on the sluggable concern
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
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
