We didn't add any validation rules to the card model. At the very least,
the title should be mandatory.
The fact that the label field is marked as optional in the form but the
other fields are not probably means description and link should be
mandatory as well. However, since there might be institutions using
cards with descriptions but no link or cards with links but no
description, so we're keeping these fields optional for compatibility
reasons. We might change our minds in the future, though.
By using real XML responses developers will be able to understand better
how the integration works (the data flow), and the correspondency between
`remote_census` settings and their place at a real XML response.
As `stubbed_responses` methods were removed from the model layer now the
stubbing part should be managed from the test environment code so also
added a new helper module `RemoteCensusSetup` that can be used anywhere
where we need to call the web service.
Co-Authored-By: Javi Martín <javim@elretirao.net>
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.
In the past, we couldn't use `polymorphic_path` in many places. For
instance, `polymorphic_path(budget, investment)` would return
`budget_budget_investment_path`, while in our routes we had defined
`budget_investment_path`.
With the `resolve` method, introduced in Rails 5.1, we can use symbols
to define we want it to use `investment` instead of `budget_investment`.
It also works with nested resources, so now we can write
`polymorphic_path(investment)`.
This makes the code for `resource_hierarchy_for` almost impossible to
understand. I reached this result after having a look at the internals
of the `resolve` method in order to get its results and then remove the
symbols we include.
Note using this method will not make admin routes compatible with
`polymorphic_path`. Quoting from the Rails documentation:
> This custom behavior only applies to simple polymorphic URLs where a
> single model instance is passed and not more complicated forms, e.g:
> [example showing admin routes won't work]
Also note that now the `admin_polymorphic_path` method will not work for
every model due to inconsistencies in our admin routes. For instance, we
define `groups` and `budget_investments`; we should either use the
`budget_` prefix in all places or remove it everywhere. Right now the
code only works for items with the prefix; it isn't a big deal because
we never call it with an item without the prefix.
Finally, for unknown reasons some routing tests fail if we use
`polymorphic_path`, so we need to redefine that method in those tests
and force the `only_path: true` option.
With chromedriver >= 80, the tests are freezing sometimes, particularly
when the same editor is loaded again.
We don't know whether it's a CKEditor issue or a chromedriver issue. In
the past we've had some errors related to CKEditor trying to load the
same instance twice and we aren't sure they have been fixed since we
could never reproduce them.
It could be a coincidence, though. If we modify the views so the only
content of the `<body>` tag is a textarea with the `html-area` class,
chromedriver freezes even if we only access the page once. So maybe
we're only detecting the problem on the second visit because the second
request is faster than the first one.
Since chromedriver no longer hangs after this change, we don't have to
force any chromedriver version anymore.
After upgrading to chromedriver 80, tests checking CKEditor's content
were causing chromedriver to hang. That's why we were configuring
webdrivers to use an older chromedriver.
Version 80 of chromedriver introduced several issues regarding frames.
Debugging shows in this case chromedriver froze when we used `setData`
and then `within_frame`. Since adding a `sleep` call made it work, we
think `within_frame` was being executed before `setData` had finished.
The fact that `setData` causes the browser to enter the frame having
CKEditor is probably the reason.
Even though the `setData` method provides a callback when it's finished,
configuring it so the rest of the Ruby code isn't executed until that
happens leads to complex code. Using Capybara's `set` to fill in the
editor is IMHO a bit easier to understand.
After this change, since we're using a method provided by Capybara
instead of executing asynchronous JavaScript code, we don't have to
check CKEditor has been filled anymore. The "Admin Active polls add"
test, which failed on my machine without that check, now passes.
If we don't use the `exact` option, tests will pass even if filling in
CKEditor adds the content twice or adds the new content to the existing
content, which has actually happened and has gone mostly unnoticed while
testing several ways to fill in CKEditor with Capybara (particularly,
when using Capybara's `send_keys` method). The problem was detected by
just one test, which checked the original content wasn't present anymore
after updating a record.
I incorrectly used "text" as variable name in commit 2cdc6a1b. In
similar places, we use `label`. We also use named parameters when only
`with:` is provided.
Some specs involving CKEditor were failing sometimes in the Rails 5.1
branch. The reason why these specs pass with Rails 5.0 but fail with
Rails 5.1 are unknown. On my machine the tests pass when precompiling
the assets, which makes me think it's related to the way Rails handles
them, but it might have nothing to do with it.
The only (apparently) 100% reliable solution I've found is to wait for
CKEditor to load before trying to fill it in. After running the tests on
my machine hundreds of time, I didn't get a single failure.
It looks like sometimes, particularly when the first thing we do after
loading a page is filling the CKEditor fields and submitting the form,
CKEditor doesn't have enough time to format the text, and so it's sent
as plain text instead of HTML. This behaviour can be reproduced on my
local machine after upgrading to Rails 5.1, with the test "Admin Active
polls Add" failing 100% of the time.
Checking CKEditor has been filled in correctly solves the issue.
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.
We're using `eq` and `match_array` in most places, but there were a few
places where we were still checking each element is included in the
array. This is a bit dangerous, because the array could have duplicate
elements, and we wouldn't detect them with `include`.
These feature tests were taking too long, we can't run them for every
single model.
I'm taking the approach of using one different model for each test, but
in theory only using a few models covering every possible scenario
would be enough.
Settings are stored in the database, and so any changes to the settings
done during the tests are automatically rolled back between one test and
the next one.
There were also a few places where we weren't using an `after` block but
changing the setting at the end of the test.
After adding proposal translatable fields to forms, they will be generated with nested translations names and ids so we can no longer
use standard id locator.
Using input label text to fill in fields works with both types of forms.
