Note we're excluding a few files:
* Configuration files that weren't generated by us
* Migration files that weren't generated by us
* The Gemfile, since it includes an important comment that must be on
the same line as the gem declaration
* The Budget::Stats class, since the heading statistics are a mess and
having shorter lines would require a lot of refactoring
For the HashAlignment rule, we're using the default `key` style (keys
are aligned and values aren't) instead of the `table` style (both keys
and values are aligned) because, even if we used both in the
application, we used the `key` style a lot more. Furthermore, the
`table` style looks strange in places where there are both very long and
very short keys and sometimes we weren't even consistent with the
`table` style, aligning some keys without aligning other keys.
Ideally we could align hashes to "either key or table", so developers
can decide whether keeping the symmetry of the code is worth it in a
case-per-case basis, but Rubocop doesn't allow this option.
When accessing the valuation area, we were only displaying the
investments directly assigned to the current valuator, but we weren't
displaying the investments assigned to that valuator's group.
Using the `assigned_investments_ids` method, which takes the valuator
group into account, solves the issue.
We've also found an issue on our development machines: since we don't
have a unique index per `investment_id` and `valuator_id` in the
`budget_valuator_assignments` table, we've found duplicate records on
this table. When that happened, we were displaying the same investment
several times.
Since now we no longer join this table in the query returning the
investment, this issue is also solved, and we're adding a test for it.
We can now remove the call to the `distinct` method when calculating the
number of investments per heading.
To get the heading where a user voted, we were relying on the
`balloted_heading_id` field.
Our guess is this was done so the total number of users is the same as
the sum of users who voted on a heading. That is, if 2000 people voted
just on the "All city" heading, 1000 voted just on the "North district"
heading, and 500 people voted on both, instead of showing "3500 people
voted in total, 2500 voted in all city, 1500 voted in north district",
we show something like "3500 people voted in total, 2250 voted in all
city, and 1250 voted in north district".
However, this approach has some disadvantages.
The first disadvantage is, the stats aren't correct. In the case above,
2500 voted on the "All city heading", so the statistics for this heading
don't show reality.
The second one is we weren't considering the last heading where users
voted inside the budget being displayed, but the last heading where
users voted, period. That means that, if all the people above voted on a
later budget, the stats for the budget above would become "3500 people
voted in total, 0 voted in all city, and 0 voted in north district".
That also means we were including headings from previous budgets in the
statistics for more recent budgets when people hadn't voted on the
recent ones.
So we're removing the `balloted_heading_id` since its data is lost once
people vote on a new budget. And, in order to show the right stats and
simplify the code, we're no longer trying to add votes just to one
heading when users vote on several headings.
Co-Authored-By: Julian Nicolas Herrero <microweb10@gmail.com>
We were testing that the `calculate_winners` method does not take the
price into account; we can do the same in a model test. We already have
a different system test to check that the price isn't displayed in the
view.
Avoid displaying the price in admin budget headings section
and avoid fill the field 'price' in admin budget headings form
when the budget has been checked with hide_money field.
We're not adding the rule because it would apply the current line length
rule of 110 characters per line. We still haven't decided whether we'll
keep that rule or make lines shorter so they're easier to read,
particularly when vertically splitting the editor window.
So, for now, I'm applying the rule to lines which are about 90
characters long.
In the form of creating a new investment was hiding the name of the
group if it had only one heading, but could be confusing to users if
there are, for example, five different groups of one heading.
The solution:
- If the budget has one group and one heading, the heading selector is
hidden.
- If the budget has one group and more than one heading, the group name
is hidden.
- If the budget has more than one group, the group name appears
regardless of the number of headings.
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.
This way we can simplify the code and don't have to rely on `.try`
statements which are confusing and so we don't allow them in the
`Rails/SafeNavigation` Rubocop rule.
With two concurrent requests, it's possible to create two ballot lines
when only one of them should be created.
The reason is the code validating the line is not thread safe:
```
if ballot.amount_available(investment.heading) < investment.price.to_i
errors.add(:money, "insufficient funds")
end
```
If the second request executes this code after the first request has
executed it but before the first request has saved the record to the
database, both records will pass this validation and both will be saved
to the database.
So we need to introduce a lock. Now when the second request tries to
lock the ballot, it finds it's already locked by the first request, and
will wait for the transaction of the first request to finish before
checking whether there are sufficient funds.
Note we need to disable transactions during the test; otherwise the
second thread will wait for the first one to finish.
Also note that we need to update a couple of tests because records are
reloaded when they're locked.
In one case, reloading the ballot causes `ballot.user` to be `nil`,
since the user is hidden. So we hide the user after creating all its
associated records (which is the scenario that would take place in real
life).
In the other case, reloading the ballot causes `ballot.user` to reload
as well. So we need to reload the user object used in the test too so it
gets the updates done on `ballot.user`.
I haven't been able to reproduce this behavior in a system test. The
following test works with Rails 5.0, but it stopped working when we
moved to system tests in commit 9427f014. After that commit, for reasons
I haven't been able to debug (reintroducing truncation with
DatabaseClaner didn't seem to affect this test, and neither did
increasing the number of threads in Puma), the two AJAX requests
executed here are no longer simultaneous; the second request waits for
the first one to finish.
scenario "Race conditions with simultaneous requests", :js do
allow_any_instance_of(Budget::Ballot::Line).to receive(:check_sufficient_funds) do |object|
allow(object).to receive(:check_sufficient_funds).and_call_original
object.check_sufficient_funds
sleep 0.3
end
["First", "Second"].each do |title|
create(:budget_investment, :selected,
heading: california,
price: california.price,
title: title
)
end
login_as(user)
visit budget_investments_path(budget, heading_id: california.id)
within(".budget-investment", text: "First") { click_link "Vote" }
within(".budget-investment", text: "Second") { click_link "Vote" }
expect(page).to have_link "Remove vote"
expect(Budget::Ballot::Line.count).to eq 1
end
This method is deprecated in Rails 5.1 because its behavior will be
different in `before` and `after` callbacks.
We're replacing the deprecated `attribute_changed?` and `attribute_was`
methods with `saved_change_to_attribute?` and
`attribute_before_last_save` during `after_save` callbacks.
https://github.com/rails/rails/pull/32835/
Rails 5.1 introduced certain changes in the way a record is touched when
the counter cache option is enabled in a belongs to association.
We need to upgrade acts-as-taggable-on so it keeps changing the
`updated_at` attribute when a new tag is added to a record.
Note we now need to reload the records in some cases to get the
`context_tag_list` method to return what we expect. Methods like
`context_tags` however work properly with no need to reload the record.
When an investment had been assigned a user tag and a valuation tag with
the same name, it appeared twice when filtering by tag.
This is because by design, in order to provide compatibility with scopes
using "select" or "distinct", the method `tagged_with` doesn't select
unique records.
Forcing the query to return unique records solves the issue.
These filters were only returning investments with valuation open, but
we don't want to do that since the time we changed the interface in
order to allow users to apply several filters at the same time.
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.
Using the `_html` suffix automatically marks texts as HTML safe, so
doing so on sanitized texts is redundant.
Note flash texts are not sanitized the moment they are generated, but
are sanitized when displayed in the view.
These variables can be considered a block, and so removing them doesn't
make the test much harder to undestand.
Sometimes these variables formed the setup, sometimes they formed an
isolated part of the setup, and sometimes they were the part of the test
that made the test different from other tests.
In the scenario where we want to test scopes and use `match_array`, we
usually declare variables we never use, which raises a warning in the
Ruby interpreter (since the main cause for an unused variable is a
typo).
So I've decided to just split the tests into cases where every record is
returned and cases were no records are returned, just like we do in
other places.
There are several other options we've considered:
1. Don't declare unused variables, but declare the ones we use
2. Prefix unused variables with un underscore
3. Declare just one variable being an array containing all elements, and
access the elements using Array#[]
4. Don't declare any variables, and compare results against attributes
such as titles
None of these options was met with enthusiasm.
