This rule was added in rubocop-rspec 1.39.0. The `visible: false` option
is equivalent to `visible: :all`, but we generally use it meaning
`visible: :hidden` for positive expectations and `visible: :all` for
negative expectations.
The only exceptations are tests where we count the number of map icons
present. I'm assuming in this case we care about the number of map icons
independently on whether they are currently shown in the map, so I'm
keeping the `visible: :all` behavior in this case.
By default, Capybara only finds visible elements, so adding the
`visible: true` option is usually redundant.
We were using it sometimes to make it an obvious contrast with another
test using `visible: false`. However, from the user's perspective, we
don't care whether the element has been removed from the DOM or has been
hidden, so we can just test that the visible selector can't be found.
Besides, using `visible: false` means the test will also pass if the
element is present and visible. However, we want the test to fail if the
element is visible. That's why a couple of JavaScript-dependant tests
were passing even when JavaScript was disabled.
These tests were supposed to check the link to vote is hidden when users
don't have permission to vote. However, they aren't testing that, since
the `visible: false` option also matches visible elements. The links are
actually considered visible since they're displayed by the browser;
there's just another element on top of them.
Using `obscured: true` instead of `visible: false` solves the issue.
However, while the `obscured` option is true when the element is hidden
by another element, it's also true when the element is not currently
visible in the browser window, so in some cases we need to scroll so the
condition is effective.
This rule was added in Rubocop 0.91.0. A similar rule named
LeakyConstantDeclaration was added in rubocop-rspec 1.34.0.
Note using the FILENAMES constant did not result in an offense using the
ConstantDefinitionInBlock rule but did result in an offense using the
LeakyConstantDeclaration rule. I've simplified the code to get rid of
the constant; not sure why we were adding a constant with `||=` in the
middle of a spec.
This rule was added in Rubocop 0.89.0. However, there are some false
positives when we don't use interpolation but simply concatenate in
order to avoid long lines. Even if there weren't false positives, there
are places where we concatenate to emphasize the point that we're adding
a certain character to a text.
We might reconsider this rule in the future, since we generally prefer
interpolation over concatenation.
The original devise_security_extension gem has not been maintained for
years. Its last release was version 0.10.0, and wasn't compatible with
Rails 5, and so we were using its master branch.
Since the gem was unmaintained, it was forked as devise-security and the
aforementioned master branch was released as version 0.10.1. This
version wasn't published in Rubygems, though, so we're now using the
first version that was published in Rubygems and had a release
announment [1].
Dependabot will probably open a pull request to upgrade to the latest
version, but for now I'm trying to keep the devise-security gem as
similar as the version we were using to make sure they're compatible,
particularly considering we're monkey-patching some of the modules
provided by this gem.
[1] https://github.com/devise-security/devise-security/releases/tag/v0.11.1
Using `travel` we go to `Time.now + interval`. If the application's time
zone changes due to seasonal clock changes during that interval, we
might travel to a time which is not the time we intended to travel to.
Example:
On a system using the UTC time zone, it's 9AM on October 25 (Friday).
Since by default CONSUL uses the Madrid time zone, it means the
application's time is 11AM.
We use `travel` to advance three days. That means we go to 9AM UTC on
October 28 (Monday). The application's time will be 10AM due to the
seasonal clock change, so we haven't fully traveled three days in
application's time.
To reproduce locally, run:
```
TZ=UTC rspec spec/system/proposal_notifications_spec.rb:410
```
Using `travel_to` with `3.days.from_now`, which uses the application's
time zone and so it will travel to October 28 at 11AM, solves the
problem.
There were places where we had two links pointing to the same place; one
link would be the name/title of a record, and one link would be under
the "actions" column.
This is confusing, since users would probably expect these links to
point to different places (which is what happens in other tables in the
admin section) and might try to click one of them and then the other
one and be surprised when they found out both of them go to the same
page.
This way we can remove duplication and simplify the code in the view.
Note we're not using the "within" method in the tests to access a row,
because it doesn't seem to work in components tests when passing the
`text:` option.
In the past we were using some <div> tags surrounding table action
links in order to guarantee these links wouldn't be wider that their
cell's space and wouldn't expand over two lines.
However, while these links didn't expand over two lines, at certain
resolutions the width of their text exceeded the width of the links,
causing part of the text to be outside their borders.
This behavior was also inconsistent: some tables had these <div> tags,
and some tables didn't.
Since we've now introduced the table actions component, the code is more
consistent and we're getting rid of these <div> tags. So now we're again
facing the issue where links could expand over two lines.
Using a flex layout solves this issue and considerably improves the
layout at lower resolutions.
This partial was going to get too complex since in some places we've got
different texts, different URLs or different confirmation messages.
While we should probably try to be more consistent and that would make
the partial work in most cases, there'll always be some exceptions, and
using a partial (with, perhaps, some helper methods) will become messy
really quickly.
While Rails provides a lot of functionality by default, there's one
missing piece which is present in frameworks like Django or Phoenix: the
so-called "view models", or "components".
It isn't easy to extract methods in a standard Rails view/partial, since
extracting them to a helper will make them available to all views, and
so two helper methods can't have the same name. It's also hard to
organize the code in modules, and due to that it's hard to figure out
where a certain helper method is supposed to be called from.
Furthermore, object-oriented techniques like inheritance can't be
applied, and so in CONSUL customizing views is harder that customizing
models.
Components fix all these issues, and work the way Ruby objects usually
do.
Components are also a pattern whose popularity has increased a lot in
the last few years, with JavaScript frameworks like React using them
heavily. While React's components aren't exactly the same as the
components we're going to use, the concept is really similar.
I've always liked the idea of components. However, there wasn't a stable
gem we could safely use. The most popular gem (cells) hasn't been
maintained for years, and we have to be very careful choosing which gems
CONSUL should depend on.
The view_component gem is maintained by GitHub, which is as a guarantee
of future maintenance as it can be (not counting the Rails core team),
and its usage started growing after RailsConf 2019. While that's
certainly not a huge amount of time, it's not that we're using an
experimental gem either.
There's currently a conflict between view_component and wicked_pdf.
We're adding a monkey-patch with the fix until it's merged in
wicked_pdf.
