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.
Our `namespace` helper returns a string. However, Rails version 5.2.4.6
doesn't allow strings as arguments to polymorphic_path [1]
Since returning a symbol in our `namespace` helper would break other
places in the application, we're converting it to a symbol in the
methods calling `polymorphic_path`.
[1] https://github.com/advisories/GHSA-hjg4-8q5f-x6fm
On small screens, sometimes the bottom of the footer didn't have the
footer's background color.
I'm not sure why the `min-height` rule affects this outcome. However,
since this rule usually results in footer with quite a bit of empty
space at the bottom, we can simpliy remove the rule and use padding to
guarantee there's a bit of space between the text in the footer and the
bottom of the screen.
We were getting an error since we started using the postgres 9.6 image:
```
Attaching to app_1, database_1
database_1 | Error: Database is uninitialized and superuser password is not specified.
database_1 | You must specify POSTGRES_PASSWORD to a non-empty value for the
database_1 | superuser. For example, "-e POSTGRES_PASSWORD=password" on "docker run".
database_1 |
database_1 | You may also use "POSTGRES_HOST_AUTH_METHOD=trust" to allow all
database_1 | connections without a password. This is *not* recommended.
database_1 |
database_1 | See PostgreSQL documentation about "trust":
database_1 | https://www.postgresql.org/docs/current/auth-trust.html
```
Version 9.4 hasn't been maintained since February 2020, so we aren't
supporting it either. And we might start using `UPSERT` instead of
`find_or_create`, which was introduced in PostgreSQL 9.5.
We're still supporting PostgreSQL 9.5 even if it's also unmaintained
because it has only been officially unmaintained for a couple of months.
In order to ensure compatibility with existing CONSUL installations, we
disabled all settings related to SDG. However, we also made it much
harder to enable SDG globally on the site, since administrators first
had to enable the SDG feature and then enable it for each process.
Most people will expect SDG is enabled for all processes once they
enable the SDG feature, so that's what we're doing. They can of course
disable specific processes should they wish to do so.
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.
Since the target branch was in a different repository, the action failed
since it couldn't find the reference.
The code here is based on a recent change in Pronto [1] and with a comparison
between the repo.url property of pull_request.head and pull_request.base
to determine if the pull request was created from a forked repository
[1] https://github.com/prontolabs/pronto/commit/4fe28418b6
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.
