We weren't showing the details of answers without a description, even if
they had images, videos or documents. Some users found that behavior
unexpected since the description isn't a mandatory field and so they
left it blank, but they added images to that answer and they didn't
appear on the poll page.
Note we had a condition not to show the title of an answer when it had
no description. I think that condition was redundant because answers
without a description weren't loaded in the first place. Anyway, that
condition doesn't make sense anymore because we're displaying answers
with images but no description.
Adding the error to the translation means Rails will automatically show
the error message in the view.
We're also adding a test to make sure the error message is correctly
displayed.
We now have duplication in the validation rules, however. Validating
translatable attributes is still a bit of mess.
In commit baaec3a29 we started using the JavaScript driver in tests
dealing with the user menu, and made all tests pass.
However, we didn't update some officing tests that were also passing
when there was a bug in the code. That's because now that these tests
use a JavaScript driver, the link to "Polling officers" is never present
before pressing the "Menu" item. So tests checking the link isn't
present when loading the page always pass.
It could be argued that seeing which proposals a user follows is a good
indicator of which proposals a user has supported, since we're
automatically creating follows for supported proposals since commit
74fbde09f. So now, we're extending the `public_interests` funcionality,
so it only shows elements users are following if they've enabled it.
This is an improvement over using the `public_activity` attribute in two
ways:
* The `public_interests` attribute is disabled by default, so by default
other users won't be able to see what a user is following
* Who has created proposals/debates/investments/comments is public
information, while who is following which elements is not; so enabling
`public_activity` shouldn't imply potentially private information should
be displayed as well
We've considered removing the `public_interests` attribute completely
and just hiding the "following" page for everyone except its owner, but
keeping it provides more compatibility with existing installations.
The `click_link` part did nothing other than scrolling to the element,
since in these cases we've got a same-page link and the element it links
to is already on the page. Programmers reading the test would expect the
link to load the page or change to a different tab and would think the
element it links to wasn't there before clicking the link (at least I
did).
Since targets didn't have a title but only a long description, every
form allowing to select targets was pretty much unusable: we either
displayed just the code or the whole description.
Now, with a concise title, it's easier to find and select the desired
target.
The titles have been copied from The Global Goals page [1].
Note we're using the `short_title` I18n key for the `title` method and
the `long_title` I18n key for the `long_title` method. We can't use
`title` as I18n key instead of `short_title` because it would affect
existing translations.
[1] https://www.globalgoals.org/
These tests were checking the URLs of documents and images pointed to
the URL generated by the `attachment.url` method. In order to do so, we
were running database queries after starting the process running the
browser, which is sometimes causing database inconsistencies when
running the tests.
So I'm simply removing the URL check. The tests are slightly less useful
now, but it isn't like they were 100% right in the first place. After
all, if the `attachment.url` method wasn't working properly, the tests
were still passing.
We ended the test with two requests and no expectations. Debugging shows
sometimes the test was hanging forever [1], probably due to simultaenous
requests.
So now we're adding expectations after each request.
[1] https://github.com/consul/consul/runs/3687898744
These fields have no label associated to them. While it's more or less
obvious for sighted users that these fields are associated with the
table cell next to them, visually impaired users might not get that
association when using the screen reader in forms mode.
Note we're using `aria-label` instead of `aria-labelledby`. IMHO in this
case `aria-labelledby` is the superior method because it guarantees the
text is present for both sighted and visually impaired users. However,
testing for fields with no label other than the one provided by
`aria-labelledby` is hard since Capybara has no support for this
attribute.
So we're using `aria-label` and testing the presence of the text on the
page (with the `within "tr", text:` statements) as well as the ARIA
label (with the `fill_in` statements).
Having to wait for a whole page refresh after updating each setting was
painful when modifying several settings.
Even though the navigation is updated immediately to reflect which
sections have been enabled/disabled, there's one gotcha. Changing the
"SDG" setting will not update the user menu (which contains a link to
SDG content) nor the "SDG Configuration" tab; refreshing the page will
be necessary to check these changes. The same happens with the map and
remote census tabs. So in these cases we're making an exception and
sending the form. We might find a better solution in the future.
For this reason, we aren't using the `switch` ARIA role. Some users
might not expect a switch control to refresh the page, just like they
usually don't expect checkboxes to refresh the page. Furthermore, screen
reader support for the `switch` role seems to be inconsistent. For
instance, NVDA with Chrome announces the control as a checkbox instead
of a switch.
Note AJAX is only used for feature settings. Other settings are still
updated with regular HTTP requests.
Since we're now using AJAX requests, we have to make sure to add an
expectation in the homepage tests in order to make sure the request has
finished before starting a new one.
We were using buttons with the "Enable" and "Disable" texts to
enable/disable settings. However, when machine learning settings were
introduced in commit 4d27bbeba, a switch control was introduced to
enable/disable them.
In order to keep the interface consistent, we're now using switch
controls in other sections where settings are enabled/disabled. We can
even use the same code in the machine learning settings as well.
We're also removing the confirmation dialog to enable/disable a setting,
since the dialog is really annoying when changing several settings and
this action can be undone immediately. The only setting which might need
a confirmation is the "Skip user verification" one; we might add it in
the future. Removing the confirmation here doesn't make things worse,
though; the "Are you sure?" confirmation dialog was also pretty useless
and users would most likely blindly accept it.
Note Capybara doesn't support finding a button by its `aria-labelledby`
atrribute. Ideally we'd write `click_button "Participatory budgeting"`
instead of `click_button "Yes"`, since from the user's point of view the
"Yes" or "No" texts aren't button labels but indicators of the status of
the setting. This makes the code a little brittle since tests would pass
even if the element referenced by `aria-labelledby` didn't exist.
There were duplicate IDs and the `lines` attribute doesn't do anything
for textareas (I guess it was accidentally used instead of the `rows`
attribute; I'm just removing it so the page looks the same way it did
until now).
Even though the `value` field didn't generate duplicate IDs, we're also
changing it because we usually set an element with the `dom_id` of a
record when it contains the whole information about a record, and not
just one piece of it. For instance, in some places we assign this ID to
the table row related to the record.
Sometimes tests were hanging indefinitely. Debugging shows that in some
cases it's due to submitting a form before the AJAX request to get
proposals, debates or investments suggestions is finished, since having
an AJAX and a non-AJAX request at the same time when running the test
sometimes leads to unexpected results.
In our case, we were having many timeouts in Github Actions in the
branches where we use both ActiveStorage and Paperclip to store files
(based on pull request 4598). I can reproduce it in those branches
running the following test ("Should show new image after successful
creation with one uploaded file"), although only when my laptop isn't
plugged (!!):
```
rspec './spec/system/proposals_spec.rb[1:33:1:14]'
```
Since we didn't have a proper way to know the AJAX request had finished,
we're adding a `suggest-success` class to the element showing the
suggestions when that happens. Then in the tests we can look for that
class after filling in the title of a proposal, debate or investments.
Just for clarity's sake, we're also adding the `suggest-loading` class
when the suggestions are loading.
In order not to have expectations everywhere about the suggestions,
we're extracting methods to fill in those titles in the tests. Note we
aren't using these methods in the "edit" actions (suggestions are not
showing when editing) or in tests with the `no_js` tag (since
suggestions only work with JavaScript).
Links acting like buttons have a few disadvantages.
First, screen readers will announce them as "links". Screen reader users
usually associate links with "things that get you somewhere" and buttons
with "things that perform an action". So when something like "Delete,
link" is announced, they'll probably think this is a link which will
take them to another page where they can delete a record.
Furthermore, the URL of the link for the "destroy" action might be the
same as the URL for the "show" action (only one is accessed with a
DELETE request and the other one with a GET request). That means screen
readers could announce the link like "Delete, visited link", which is
very confusing.
They also won't work when opening links in a new tab, since opening
links in a new tab always results in a GET request to the URL the link
points to.
Finally, submit buttons work without JavaScript enabled, so they'll work
even if the JavaScript in the page hasn't loaded (for whatever reason).
For all these reasons (and probably many more), using a button to send
forms is IMHO superior to using links.
There's one disadvantage, though. Using `button_to` we create a <form>
tag, which means we'll generate invalid HTML if the table is inside
another form. If we run into this issue, we need to use `button_tag`
with a `form` attribute and then generate a form somewhere else inside
the HTML (with `content_for`).
Note we're using `button_to` with a block so it generates a <button>
tag. Using it in a different way the text would result in an <input />
tag, and input elements can't have pseudocontent added via CSS.
The following code could be a starting point to use the `button_tag`
with a `form` attribute. One advantage of this approach is screen
readers wouldn't announce "leaving form" while navigating through these
buttons. However, it doesn't work in Internet Explorer.
```
ERB:
<% content_for(:hidden_content, form_tag(path, form_options) {}) %>
<%= button_tag text, button_options %>
Ruby:
def form_id
path.gsub("/", "_")
end
def form_options
{ id: form_id, method: options[:method] }
end
def button_options
html_options.except(:method).merge(form: form_id)
end
Layout:
<%= content_for :hidden_content %> # Right before the `</body>`
```
The message "Are you sure?" is usually followed by blindly clicking
"Yes" without really thinking about what one is doing. So we're
including a bit more information about what's about to happen. That way
it's more likely users will notice it when they accidentally click the
wrong button.
Ideally we would offer the option to undo every common action and then
we wouldn't have to ask for confirmation. But since that isn't the case,
for now we're adding a better confirmation message.
Note we're removing the `resource_name` parameter from the translation
to confirm the action of deleting a record. The reason is, in many
languages it only makes sense to add the model name when it's got an
associated article, and, unlike in English (where "the" is used for
every word), that article is different depending on the noun it's
related to. So we'd have to provide a translation like "name with
article, when singular" for every model.
The complexity of these translations could scalate quickly. And, given
the context, IMHO it isn't essential to add the resouce name. When we're
in the proposals index and there's a proposal named "Improve XYZ", and
we click on "Delete" and see a message saying "This action will delete
XYZ", it is implied that XYZ is a proposal.
So instead we're changing the message so it works for every record with
no need of noun-dependent articles.
When users see a label saying "With the text" and an input field, they
don't usually need a placeholder saying "Write the text". On the
contrary, this text adds noise and is hard to read due to the low
contrast between the color of the placeholder and the color of the
field, making the text an unnecessary distraction.
User testing has shown this filter isn't really useful and sometimes
makes users wonder what it's about. This is particularly true in CONSUL
installations which don't change the default values (most of them),
since users will see a filter with options like "Official position 1".
We're not adding the rule because it would apply the current line length
rule of 110 characters per line. We still haven't decided whether we'll
keep that rule or make lines shorter so they're easier to read,
particularly when vertically splitting the editor window.
So, for now, I'm applying the rule to lines which are about 90
characters long.
We forgot to use it in one place, and we've found out other institutions
using CONSUL whose developers aren't so familiar with Ruby also break
this rule, so it might be better to add it explicitly.
We were clicking links and visiting pages without checking the previous
request had already finished. This might cause concurrent requests,
leading to unpredictable results.
It might be the reason why this test failed once when running our
continuous integration [1].
[1] https://github.com/consul/consul/runs/3295502777
The test was failing sometimes, probably because the "Edit" link within
the "An example legislation process" row is already present before
clicking the "All" link. This can lead to simultaneous requests.
Just removing the unnecessary click on the "All" link solves the issue.
When render the investment list component with the link "see all
investments", now we redirect to groups index page when a budget has
multiple headings.
In general, slow system tests requiring no interaction from the user are
good candidates to be moved to component tests because component tests
are much faster.
In this case, the system tests were also updating the database after
starting the browser, which might cause concurrency issues. We could
split the test and have one system test per phase, but IMHO there's no
need.
We're still having a couple of system tests for the happy path, in order
to make sure users actually see the list of investments.
While we use Pronto to detect offenses in the lines changed in our pull
request, sometimes our changes introduce offenses in other lines, and we
don't detect them.
In commit 0488b3735, we removed the only usage of the `heading` method
in a test, which caused a `RSpec/LetSetup` offense.
In commit 287c48873, we changed some lines from `fill_in` to
`fill_in_ckeditor`. Some of these lines were aligned with the following
ones, which after that change had extra spacing for no reason.
Finally, in commit 8d38ed58c we added a line before two lines which had
their equals signs aligned. Since, after adding this line, the block was
no longer aligned, there was no reason for the extra space in one of the
lines.
