Using pg_search 2.0.1 with Rails 5.2 results in deprecation warnings:
DEPRECATION WARNING: Dangerous query method (method whose arguments used
as raw SQL) called with non-attribute argument(s):
"pg_search_978c2f8941354cf552831b.rank DESC, \"tags\".\"id\" ASC".
Non-attribute arguments will be disallowed in Rails 6.0. This method
should not be called with user-provided values, such as request
parameters or model attributes. Known-safe values can be passed by
wrapping them in Arel.sql().
We're not upgrading to the latest pg_search because it only supports
ActiveRecord >= 5.2.
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
Recent versions introduce the `Layout/SpaceAroundMethodCallOperator`,
which we are going to use. We aren't upgrading to the latest rubocop
version because it conflicts with the version of Capybara we're using
and because it isn't supported by Hound.
Some rules have been renamed:
Layout/IndentAssignment is now Layout/AssignmentIndentation
Layout/IndentHeredoc is now Layout/HeredocIndentation
Layout/LeadingBlankLines is now Layout/LeadingEmptyLines
Layout/Tab is now Layout/IndentationStyle
Layout/TrailingBlankLines is now Layout/TrailingEmptyLines
Lint/StringConversionInInterpolation is now Lint/RedundantStringCoercion
Metrics/LineLength is now Layout/LineLength
Note after upgrading we get a new "offense" in the `StartWith` rule, so
we're changing the code in order to fix it.
Rails 5.1 updated the `method_source` dependency, which is incompatible
with pry 0.12.x (which some developers are using), and upgrading pry and
pry-byebug requires a more recent version of byebug.
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.
Note we need to upgrade the bullet gem, although another option would be
to remove it completely.
Now we don't need the rubocop rules for deprecated methods, since using
them will raise an error and we'll be notified immediately.
While the browser gem is great, we don't need it in this case for such a
simple usage.
There are a few really small differences between this code and the old
one: matching `/MSIE/` will return true for Opera 12 and false for
certain versions of IE11. Since we're only rendering a comment for IE8
and below, we don't care about IE11, and Opera 12 is six years old and
its users won't be affected by the comment.
Note we're still using the browser gem because ahoy_matey depends on it,
but now it's an indirect dependency.
This version solves a security issue:
https://ckeditor.com/cke4/release/CKEditor-4.11.0
Note this version adds a `ckeditor/samples` folder, which is
automatically added to the application's assets manifest even if we
remove all CKEditor references in our application. One of the files in
that folder makes ExecJS raise a syntax error, causing every page to
raise a 500 error.
This feature wasn't properly tested nor reviewed, and after reviewing
several pull requests with a similar status and considering this pull
request is related to the public area of the web, we've decided to
remove it before releasing version 1.1.
This commit reverts commit 4f50e67a.
Our manual implementation had a few issues. In particular, it didn't
track changes related to associations, which became more of an issue
when we made investments translatable.
Using audited gives us more functionality while at the same time
simplifies our code. However, it adds one more external dependency to
our project.
The reason for choosing audited over paper trail is audited seems to
make it easier to handle associations.
Puma is the server we use in the development environment, so this way we
don't need to maintain two servers. Furthermore, puma seems to offer a
few advantages over unicorn (like multithreading) and no disadvantages.
Rails and performance cops have been extracted to separate gems.
Note in the past we had to add these lines in order to activate Rails
cops:
```
Rails:
Enabled: true
```
But we didn't do it, and so Rails cops were ignored.
With the new version, it's enough to require `rubocop-rails`.
