The next warning is flooding the tests log:
`Options passed to Node#set but the RackTest driver
doesn't support any - ignoring`
As the rack test driver does not support any option, we
get rid of all `default_set_options` before each system
spec that uses this driver.
- Allow to define a link (text and url) on budget form for render on the budget
header.
- Improve styles
Co-authored-by: Senén Rodero Rodríguez <senenrodero@gmail.com>
When users created a budget and made a typo, they could use the link to
go back to edit a budget. However, after doing so, they were out of the
budget creation process.
So we're now letting users go back to edit the budget, fix any mistakes
they might have made, and then continue to groups.
So now there's no need to edit each phase individually to enable/disable
them.
We aren't doing the same thing in the form to edit a budget because we
aren't sure about possible usability issues. On one hand, in some tables
we automatically update records when we mark a checkbox, so users might
expect that. On the other hand, having a checkbox in the middle of a
form which updates the database automatically is counter-intuitive,
particularly when right below that table there are other checkboxes
which don't update the database until the form is submitted.
So, either way, chances are users would think they've updated the phases
(or kept them intact) while the opposite would be true.
In the form within the wizard to create a budget that problem isn't that
important because there aren't any other fields in the form and it's
pretty intuitive that what users do will have no effect until they press
the "Finish" button.
Co-Authored-By: Julian Nicolas Herrero <microweb10@gmail.com>
Note we're keeping this section's original design (which had one button
to add a new group which after being pressed was replaced by a button to
cancel) but we aren't using Foundation's `data-toggle` because there
were a couple of usability and accessibility issues.
First, using `data-toggle` multiple times and applying it to multiple
elements led to the "cancel" button not being available after submitting
a form with errors. Fixing it made the code more complicated.
Second, the "Add new group" button always had the `aria-expanded`
attribute set to "true", so my screen reader was announcing the button
as expanded even when it wasn't. I didn't manage to fix it using
`data-toggle`.
Finally, after pressing either the "Add new group" and "Cancel" buttons,
the keyboard focus was lost since the elements disappeared.
So we're simplifying the HTML and adding some custom JavaScript to be
able to handle the focus and manually setting the `aria-expanded`
attribute.
Co-Authored-By: Javi Martín <javim@elretirao.net>
Co-Authored-By: Julian Herrero <microweb10@gmail.com>
In this case, the duration of the budget cannot be determined, and the
application was crashing when trying to do so.
Now we're just returning `nil` as duration.
There's an edge case where the current phase of the budget was disabled.
In this case, the application was crashing.
I'm not sure what we should do regarding this case. Is it OK to allow
disabling the current phase? Is it OK to allow selecting a disabled
phase as the current phase?
Since I'm not sure about it, for now I'm leaving it the same way it was.
Co-authored-by: Julian Herrero <microweb10@gmail.com>
We don't allow deleting a budget with associated investments. However,
we allow deleting a budget with associated administrators and valuators.
This results in a foreign key violation error:
PG::ForeignKeyViolation: ERROR: update or delete on table "budgets"
violates foreign key constraint "fk_rails_c847a52b1d" on table
"budget_administrators"
Using the `dependent: :destroy` option when defining the relationship,
we remove the association records when removing the budget.
As a bonus, we reduce the number of Rubocop offenses regarding the
`Rails/HasManyOrHasOneDependent` rule. Only 72 to go! :)
Some users might not be able to touch the icon due to a motor
disability. Other users might think the "Menu" text is part of the
button and try to touch it instead.
Making the "Menu" text part of the button makes it easier to show/hide
this menu. Besides, it lets screen reader users with a small screen hear
the word "Menu" associated to the button.
We could simplify the HTML a bit more but Foundation's `hamburger` mixin
uses the `::after` element with `position: absolute`, so we can't apply
it directly to the button without making the CSS more complex.
In commit 905ac48bb we activated exceptions when assets were not found,
in order to detect places where we were trying to load non-existent
images.
We got an exception for that reason: we were loading images based on the
current locale, but for some locales there was no images.
We're now using fallbacks and loading another image when the original
one isn't available.
Note we're copying the English images to images with a generic name for
the case where there's no fallback with an image. We're copying the
files instead of using symbolic links to make sure they can be
overwritten independently in other CONSUL installations.
Also note we're updating the HTML so the section gets the ID instead of
the header. That way the system test is simple.
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.
