When configuring phases in a process, we were validating the start date
or the end date is present, the other date is present too.
However, in other parts of the application we were checking whether a
phase is enabled and then assumed its dates were present if the phase
was enabled. However, we weren't validating this behavior, so it was
possible to enable a phase and leaving its dates blank, causing the
application to crash.
So, as suggested by Alberto, we're changing the validation rule so
phase dates are mandatory when a phase is enabled.
With this rule, the old validation rules are not necessary. I've
considered leaving them in order to avoid database inconsistencies.
However, I realized records having a disabled phase with its start and
end dates have always been valid. This means applications checking for
the presence of these dates instead of checking whether the phase is
enabled have never worked properly.
We don't have to change the logic anywhere else because as mentioned we
were already checking phases are enabled before using their dates.
Before commit 28caabecd, it was clear which budgets were in draft mode
because their phase was "drafting".
Now the phase isn't "drafting" anymore, so we have to make it clear
somehow that the budget is a draft.
I'm using styles similar to the ones we added in commit 2f636eaf7 for
completed budgets but at the same time making them slightly different so
it's easy to differenciate completed and drafting budgets.
Particularly the line with `within "tr", text: "Finished budget" do` is
now easier to read.
This way we avoid a potential pitfall. Imagine that the factory which
creates a finished budget generated a budget with the name "COMPLETED
Budget 1". Then the test:
```
within "#budget_#{finished_budget.id}" do
expect(page).to have_content("COMPLETED")
end
```
Would pass even if we didn't add the text "COMPLETED" anywhere else,
because it would be included in the name of the budget.
We only need to define one `in_browser`, which is the one opening the
session as an administrator.
This change is done to simplify the code, although there's a small
chance it might also make the test stop failing in our CI. Sometimes in
our CI the first `visit` in the `in_browser(:admin)` block fails for
unknown reasons, rendering a blank page.
The controller provided by the `devise-security` gem which tests
password is expired does not execute the `before_action` we have in our
application controller. That means it doesn't set the current locale.
We were having issues in the tests checking this behavior if the
previous test had set the current locale to a different one. This meant
the process running the browser had one locale while the process running
the test had a different one, which resulted in a page in English (as
expected), only the flash message notifying users their password expired
was in a different language.
To reproduce this behavior, run:
```
rspec './spec/system/welcome_spec.rb[1:1:2:2:1]' spec/system/users_auth_spec.rb:623 --order defined
```
I'm not sure whether this is a bug or it's a problem with the tests. In
theory it might be possible to reproduce a similar behavior in
production due to what we mention about the controller not executing the
`set_current_locale` method. But I haven't been able to reproduce the
situation, particularly since the password expiration seems to be
checked exclusively at login time (that is, if you stay logged in for 10
years, your password doesn't seem to expire).
So for now I'm just making the tests pass by using the login form
instead of using `login_as`.
The link to edit the process is already present before clicking the
"All" link, which meant the test failed sometimes because Capybara might
try to click on the "Edit" link at the same time the page is changing
due to the click on the "All" link".
Due to this issue, this test has failed at least one in our CI [1].
[1] https://github.com/consul/consul/runs/2324773853
It's true that previously we didn't display the tag cloud on all phases
and so we added a test checking we did on all phases.
However, doing so makes tests really slow and prone to database
inconsistencies because the alter the database after the process running
the browser has started.
So now we're using a random phase in these tests to solve this issue.
We're also removing the `login_as(admin) if budget.drafting?` line
because we removed the drafting phase in commit 28caabecd.
We were checking content which was already present/absent before making
a certain request, so the expectations were not checking the request had
already finished. Our intention here is to check the page contents after
the request has finished.
We want to make sure the request is finished after clicking a button and
before visiting a different page, so we need to check the page has
changed.
Usually this shouldn't be a problem because most of our forms are sent
with regular HTTP requests instead of AJAX ones, so the `visit` method
wouldn't be called before the request is finished.
However, we're experiencing problems with certain version of
Chromedriver, and in general it's a good practice because we might send
forms using AJAX/Turbolinks in the future.
In general, we shold check the contents of the page instead of the
current path, since the contents of the page are what users experience.
In one test, the only reason we check the current path additionally to
the contents of the page is to make sure we're still in the management
section.
Checking just that we avoid querying the database after starting the
browser.
When we create a record like a debate or an event and we check the page
content, we want to make sure that today's date is present, since it's
the date where the record is supposed to have been created.
This way we avoid querying the database after the browser has been
started.
It's strange to create records without assigning them to a variable and
then query the database to fetch the very same records. Assigning them
to a variable makes the tests easier to understand.
Besides, this way we avoid querying the database after the browser has
started.
This way we avoid modifying the database in the middle of a system test
(after we've started the browser), which can lead to database
inconsistencies.
In the case of the reclassification specs we're simply removing part of
the test because that part is already tested by other specs.
Users don't care about database content; they care about what they see
on the screen.
Writing tests this way we also avoid potencial database inconsistencies
due to accessing the database after starting the browser.
Checking the database with methods like Activity.last does not test that
the record is present where it should be (first record of the table in
this case). In these tests there's only one record, though, so the order
doesn't matter that match.
However, calling methods like Activity.last generates a database query
after the process running the browser has been started, and this might
lead to inconsistent data.
We take the comment as a parameter instead of the user, since usually
people reply to comments and not to users.
We also remove one database query after the browser has started, since
we can use `debate_path(debate)`. It's also more clear why we're using
`debate_path` in the test; before these changes, we had to enter the
`reply_to` method to realize that we were replying on a debate.
What users care about isn't the database; they care about that reason
being displayed when administrators check the reason.
This way we also avoid accessing the database after the process running
the browser has been started.
System tests are about user experience, so instead of checking the slug
has been updated in the database, we check whether the page can be
accessed using the slug.
Note the budget group test is a bit different because the name of the
group isn't present in the budget group page.
We were checking the database, but users don't care about what's inside
the database; they care about what happens when they visit the page of a
record they've just restored.
This way we also avoid data inconsistency due to the process running the
test accessing the database after the process running the browser has
started.
We check the changes have been saved and we check recommendations have
been disabled after visiting the debates and proposals pages. The
latter helps us avoid accessing the database after the process running
the browser has been started.
These tests check what happens from the user's point of view. For
instance, we check that after disabling recommendations, they are not
shown. What happens in the database is not related to the user
experience.
Furthermore, checking the database after the browser has started is
proving to be a major source for inconsistent data in specs.
We were checking what happens in the database, but what's important is
what users experience after hiding content or blocking authors. Besides,
accessing the database in tests after the browser has started might lead
to database inconsistencies.
Just like we mentioned in commit 5f0c422eb, the filter "Mark as viewed"
doesn't work properly, so here's a case where the test would fail with
JavaScript not because the test is wrong, but due to a bug. The test was
passing simplify because there was a typo in the CSS selector, which was
supposed to select an element by ID but didn't have the "#" prefix.
For now we're keeping the test as it was, but eventually we'll have to
fix the bug.
We were adding a `visit` in a `before` block but then we started some
tests with another `visit`.
We also destroyed records in the database in between, which increased
the risk of database inconsistency since the process running the browser
had already been started.
Besides, some tests were wrong; they were visiting a page with the
browser, then destroying records in the database, and then checking the
page without reloading the browser. Since we aren't automatically
refreshing the affected areas of the page, obviously the page content
before and after destroying records is exactly the same, and the test
was passing because it's testing content that isn't there in any
situation.
We were updating the database after starting the browser to emulate the
behavior where a user logs in a day before the current request. We can
use `current_sign_in_at` instead and devise will automatically copy that
value to `last_sign_in_at` after users visit a page.
This way we avoid setting up the database after the process runnin the
browser has been started.
Changing the database after the process running the browser has started
is proving to be one of the reasons tests are failing sometimes, so
we're reducing the number of times were that happens. In this case, we
were changing a setting.
We were adding a `visit` in a `before` block but then we started the
search tests with another `visit`.
We also created records in the database in between, which increased the
risk of database inconsistency since the process running the browser had
already been started.
Just like we did in commit 0ec8878db, we remove the useless initial
request in the `before` filter since most tests started by visiting a
different URL.
We also reduce the risk of database inconsistency which comes with
setting up the database after the browser has been started.
Although it wasn't a real security concern because we were only calling
a `find` method based on the user input, it's a good practice to avoid
using constants based on user parameters.
Since we don't use the `find` method anymore but we still need to check
the associated record exists, we're changing the `followable` validation
in the `Follow` model to do exactly that.
