In some places, we were accidentally creating records after the browser
started because we weren't executing a `let` block before starting the
browser with a `visit`, but were executing the `let` block after that.
These blocks are no longer used:
* `allowed_phase_list` isn't used since commit 04605d5d5
* `level_two_user` isn't used since commit 26d14cbd0
* `heading` in `budgets/stats_spec` was added in c2457e36a but never
used
* `translatable` was added in 44d137a4c but it's overwritten in all the
contexts.
* `annotation` isn't used since commit 79d00e7b9
* `admin` in `tags/budget_investments_spec` isn't used since 8a2e15980
* `budget` in `welcome_spec` was added in 87be6f302 but never used
After a `visit`, we were checking for content or filling in fields that
were already there before the `visit`, so we weren't 100% sure that the
request had finished before the test continued.
In the case of the verification tests, we were clicking the submit
buttons over and over without and then checking or interacting with
elements that were already there. Even though the button was disabled
between requests, meaning there wouldn't be simultaneous requests, it
was possible to interact with a form field before waiting for the
request to finish.
Some of these tests have recently failed on our CI, and it might be
because of that:
```
1) Admin budgets Edit Changing name for current locale will update the
slug if budget is in draft phase
Failure/Error: raise ex, cause: cause
Selenium::WebDriver::Error::UnknownError:
unknown error: unhandled inspector error: {"code":-32000,
"message":"Node with given id does not belong to the document"}
(Session info: chrome=134.0.6998.35)
1) Budgets creation wizard Creation of a multiple-headings budget by
steps
Failure/Error: expect(page).to have_content "Heading created
successfully!"
Selenium::WebDriver::Error::UnknownError:
unknown error: unhandled inspector error: {"code":-32000,
"message":"Node with given id does not belong to the document"}
(Session info: chrome=134.0.6998.35)
1) Custom information texts Show custom texts instead of default ones
Failure/Error: raise ex, cause: cause
Selenium::WebDriver::Error::UnknownError:
unknown error: unhandled inspector error: {"code":-32000,
"message":"Node with given id does not belong to the document"}
(Session info: chrome=134.0.6998.35)
1) Users Regular authentication Sign in Avoid username-email collisions
Failure/Error: raise ex, cause: cause
Selenium::WebDriver::Error::UnknownError:
unknown error: unhandled inspector error: {"code":-32000,
"message":"Node with given id does not belong to the document"}
(Session info: chrome=134.0.6998.35)
2) Verify Letter Code verification 6 tries allowed
Failure/Error: raise ex, cause: cause
Selenium::WebDriver::Error::UnknownError:
unknown error: unhandled inspector error: {"code":-32000,
"message":"Node with given id does not belong to the document"}
(Session info: chrome=134.0.6998.35)
2) Valuation budget investments Valuate Finish valuation
Failure/Error: raise ex, cause: cause
Selenium::WebDriver::Error::UnknownError:
unknown error: unhandled inspector error: {"code":-32000,
"message":"Node with given id does not belong to the document"}
(Session info: chrome=134.0.6998.35)
1) Users Delete a level 2 user account from document verification page
Failure/Error: raise ex, cause: cause
Selenium::WebDriver::Error::UnknownError:
unknown error: unhandled inspector error: {"code":-32000,
"message":"Node with given id does not belong to the document"}
(Session info: chrome=134.0.6998.35)
```
Quoting from commit 57bda006b5:
> When clicking the button "Search", the link "newest" is already
> present, so capybara might click the "newest" link before the "Search"
> request is finished, leading to unexpected results.
>
> Checking the page to make sure the "Search" request has finished
> before clicking the "newest" link solves the problem.
We're doing the same thing for the other tests that click the "Search"
button and then clicked on a link. We're also making sure the language
selector changes before clicking it again.
There were many cases where we were clicking on a link or (most of the
time) a button and then calling the `visit` method. In the past, it
worked just fine because clicking on buttons usually results in non-AJAX
requests, meaning that the test waited for the request to finish before
continuing.
That's no longer the case, though. In the last few months/years (not
sure since when) we're getting sporadic failures because the test
doesn't wait for the request to finish before making another request
with the `visit` method. This sometimes results in flaky tests.
Some of these tests have recently failed in our CI. Here are a few
examples (note the numbers don't follow an order because these tests
failed in different jobs):
```
1) Admin edit translatable records Current locale translation does not
exist For ActivePoll Shows first available fallback
Failure/Error: expect(page).to have_content "Sondage en Français"
expected to find text "Sondage en Français" in "Language: \n
\nEnglish\nDeutsch\nEspañol\nFrançais\nNederlands\nPortuguês
brasileiro\n中文\n Go back to CONSUL DEMOCRACY\nCONSUL
DEMOCRACY\nADMINISTRATION\nMenu\nNotifications\nMy content\nMy
account\nSign out\nProposals\nDebates\nComments\nPolls\n
Collaborative Legislation\nParticipatory budgets\nVoting booths
\nSignature Sheets\nMessages to users\nSite content\nModerated
content\nProfiles\nStatistics\nSettings\nProposals dashboard\n×
\nPolls description updated successfully.\nList of polls\nPolls
description\nCreate poll\nThere are no polls."
2) Public area translatable records Existing records Update a
translation With valid data Changes the existing translation
Failure/Error: expect(page).to have_field "Debate title",
with: "Title in English"
expected to find field "Debate title" that is not disabled but
there were no matches
2) Admin collaborative legislation Update Edit milestones summary
Failure/Error: expect(page).to have_content "There is still a long
journey ahead of us"
expected to find text "There is still a long journey ahead of us"
in "Language: \n
\nEnglish\nDeutsch\nEspañol\nFrançais\nNederlands\nPortuguês
brasileiro\n中文\n Go back to CONSUL DEMOCRACY\nCONSUL
DEMOCRACY\nADMINISTRATION\nMenu\nNotifications\nMy content\nMy
account\nSign out\nProposals\nDebates\nComments\nPolls\n
Collaborative Legislation\nParticipatory budgets\nVoting booths
\nSignature Sheets\nMessages to users\nSite content\nModerated
content\nProfiles\nStatistics\nSettings\nProposals dashboard\n×
\nProcess updated successfully. Click to visit\nBack\nAn example
legislation process\nInformation\nHomepage\nDebate\nProposals\n
Drafting\nFollowing\n1 language in use\nCurrent language\n
English\nSummary\n Format\n ◢\n Milestone\nManage progress
bars\nDon't have defined milestones\nCreate new milestone".
(However, it was found 1 time including non-visible text.)
3) Admin collaborative legislation SDG related list create Collaborative
Legislation with sdg related list
Failure/Error:
within("tr", text: "Legislation process with SDG related content") do
expect(page).to have_css "td", exact_text: "17"
end
Capybara::ElementNotFound:
Unable to find css "tr"
4) Valuation budget investments Valuate Feasibility can be marked as
pending
Failure/Error: expect(find("#budget_investment_feasibility_undecided"))
.not_to be_checked
Capybara::ElementNotFound:
Unable to find css "#budget_investment_feasibility_undecided"
3) Custom information texts Show custom texts instead of default ones
Failure/Error:
within("#section_help") do
expect(page).to have_content "Custom help with debates"
expect(page).not_to have_content "Help with debates"
end
4) Admin budgets Update Deselect all selected staff
Failure/Error: expect(page).to have_link "Select administrators"
expected to find link "Select administrators" but there were no
matches
3) Admin polls SDG related list edit poll with sdg related list
Failure/Error:
within("tr", text: "Upcoming poll with SDG related content") do
expect(page).to have_css "td", exact_text: "17"
end
Capybara::ElementNotFound:
Unable to find css "tr"
4) Admin polls SDG related list create poll with sdg related list
Failure/Error:
within("tr", text: "Upcoming poll with SDG related content") do
expect(page).to have_css "td", exact_text: "17"
end
Capybara::ElementNotFound:
Unable to find css "tr"
5) Admin custom images Image is replaced on admin newsletters
Failure/Error:
within(".newsletter-body-content") do
expect(page).to have_css("img[src*='logo_email_custom.png']")
end
Capybara::ElementNotFound:
Unable to find css ".newsletter-body-content"
6) Admin custom images Image is replaced on front views
Failure/Error:
within("#map") do
expect(page).to
have_css("img[src*='custom_map.jpg'][alt='Districts list']")
end
Capybara::ElementNotFound:
Unable to find css "#map"
```
We were rendering one label and many textarea fields for that label.
This meant that, when switching to a different language, the label
wasn't correctly associated with the textarea.
So we're now rendering one label for each textarea. We could use
`aria-label` or `aria-labelledby` instead, but using a label offers some
advantages like the fact that clicking on the label makes the textarea
take the focus.
The absence of labels in these controls made them hard to use,
particularly for people who use screen readers.
Note we're removing the "Choose language" prompt, since we always
automatically choose a language and not choosing a language doesn't
really make sense. The only scenario where the prompt was used took
place when all languages had been removed but, in that case, the "Choose
language" prompt was misleading because there were no languages to
choose from.
Having a class named `Poll::Question::Answer` and another class named
`Poll::Answer` was so confusing that no developer working on the project
has ever been capable of remembering which is which for more than a few
seconds.
Furthermore, we're planning to add open answers to polls, and we might
add a reference from the `poll_answers` table to the
`poll_question_answers` table to property differentiate between open
answers and closed answers. Having yet another thing named answer would
be more than what our brains can handle (we know it because we did this
once in a prototype).
So we're renaming `Poll::Question::Answer` to `Poll::Question::Option`.
Hopefully that'll make it easier to remember. The name is also (more or
less) consistent with the `Legislation::QuestionOption` class, which is
similar.
We aren't changing the table or columns names for now in order to avoid
possible issues when upgrading (old code running with the new database
tables/columns after running the migrations but before deployment has
finished, for instance). We might do it in the future.
I've tried not to change the internationalization keys either so
existing translations would still be valid. However, since we have to
change the keys in `activerecord.yml` so methods like
`human_attribute_name` keep working, I'm also changing them in places
where similar keys were used (like `poll_question_answer` or
`poll/question/answer`).
Note that it isn't clear whether we should use `option` or
`question_option` in some cases. In order to keep things simple, we're
using `option` where we were using `answer` and `question_option` where
we were using `question_answer`.
Also note we're adding tests for the admin menu component, since at
first I forgot to change the `answers` reference there and all tests
passed.
Adding, modifiying, and/or deleting questions for an already started
poll is far away from being democratic and can lead to unwanted side
effects like missing votes in the results or stats.
So, from now on, only modifiying questions will be possible only if
the poll has not started yet.
We need to update a couple of tests because a poll is created in the
tests with a timestamp that includes nanoseconds and in the form to edit
the time of the poll the nanoseconds are not sent, meaning it was
detected as a change.
- 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>
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.
JavaScript is used by about 98% of web users, so by testing without it
enabled, we're only testing that the application works for a very
reduced number of users.
We proceeded this way in the past because CONSUL started using Rails 4.2
and truncating the database between JavaScript tests with database
cleaner, which made these tests terribly slow.
When we upgraded to Rails 5.1 and introduced system tests, we started
using database transactions in JavaScript tests, making these tests much
faster. So now we can use JavaScript tests everywhere without critically
slowing down our test suite.
We were repeating the same code over and over (with a few variants) to
setup tests which require an administrator. We can use a tag and
simplify the code.
It worked differently after upgrading to jQuery 3. According to the
jQuery upgrade guide:
> It is almost always a mistake to use .removeAttr("checked") on a DOM
> element. The only time it might be useful is if the DOM is later going
> to be serialized back to an HTML string. In all other cases,
> .prop("checked", false) should be used instead.
These routes are solved in a different way because of an inconsistency:
we define `groups` and `budget_investments`; we should either use the
`budget_` prefix in all places or remove it everywhere.
We can now share code using `polymorphic_path` even with these models.
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.
