This way it's easier to test; changing it will also be easier.
During my experiments I made a mistake which wasn't covered by the test
suite. We're adding a test for this case.
Note we're using `i18n_content` in the component instead of `content`
because there's already a `content` method provided by ViewComponent.
When we try to register with omniauth and the email or username already exists,
we use the finish_signup and do_finish_signup actions to allow the user to choose
another email or username.
The do_finish_signup action of the registration controller calls the
send_oauth_confirmation_instructions method which is responsible for sending the
confirmation email.
In this method we were only validating the case that the email is duplicated. Now
we add one more condition that allows us to send the instructions for the case in
which we have had to change our username.
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.
This column wasn't used in any released Consul version since it was only
used during development. For the same reason, the task to migrate the
information in the `link` column to the `links` table isn't needed
either.
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.
We were already doing so for debates and investments.
We probably never noticed because this is an edge case that requires
enabling a feature, people adding comments, and then disabling the
feature.
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/
The same way we don't render empty regular tags since commit 4d27bbeba.
This way we avoid adding an empty `<div class="sdg-tag-list">` tag,
which might have associated styles (in custom CONSUL installation
styles, for instance) and thus break the layout
Based on an EPS file downloaded from The Global Goals page [1].
Although in that page there are icons for other languages we support and
that we've only got in PNG format, the Spanish ones are the only ones
which are similar to the official PNG ones provided by the UN or
UN-related organizations like UNRIC. Icons in other languages (like
Chinese, French or Russian) are not that similar to the official PNG
icons and their quality is (in my humble opinion) lower.
Since SVG icons are smaller and can be compressed, users browsing the
page in Spanish will have to download about 80KB for the SDG icons,
instead of the 240KB they needed to download when using PNGs.
[1] https://www.globalgoals.org/resources
These icons have been downloaded from The Global Goals page [1]. English
is the official language of this page and the only one containing all
the information.
Since SVG icons are smaller and can be compressed, users browsing the
page in English will have to download about 45KB for the SDG icons,
instead of the 250KB they needed to download when using PNGs.
[1] https://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 were testing the URL of the image changes to `missing.png`, but
actually that's confusing because the image record is now invalid and so
its changes can't be saved. That means that, when rendered in the
browser, the image won't render the `missing.png` image but will try to
render the destroyed one.
If we want to render the `missing.png` image when the attachment has
been destroyed, we need to remove the attachment presence validation or
change the `url` method so it detects when an attachment is missing.
We're already using a custom controller to handle direct uploads.
Besides, as mentioned by one of Active Storage co-authors [1], the
Active Storage DirectUploadsController doesn't provide any
authentication or validation at all, meaning anyone could create blobs
in our database by posting to `/rails/active_storage/direct_uploads`.
The response there could be then used to upload any file (again, without
validation) to `/rails/active_storage/disk/`.
For now, we're monkey-patching the controllers in order to send
unauthorized responses, since we aren't using these routes. If we ever
enable direct uploads with Active Storage, we'll have to add some sort
of authentication.
Similar upload solutions like CKEditor don't have this issue since their
controllers inherit from `ApplicationController` (which includes
authorization rules), while Active Storage controllers inherit from
`ActionController::Base`.
[1] https://discuss.rubyonrails.org/t/activestorage-direct-uploads-safe-by-default-how-to-make-it-safe/74863/2
There could be inconsistencies in the database and an attachment might
have a `record_id` pointing to a record which no longer exist. We were
getting an exception in this case.
Just like we add the `storage_` prefix for new records so we can use
both Active Storage and Paperclip at the same time.
Now the migration actually works, at least for basic cases.
In order to migrate existing files from Paperclip to ActiveStorage, we
need Paperclip to find out the files associated to existing database
records. So we can't simply replace Paperclip with ActiveStorage.
That's why it's usually recommended [1] to first run the migration and
then replace Paperclip with ActiveStorage using two consecutive
deployments.
However, in our case we can't rely on two consecutive deployments
because we have to make an easy process so existing CONSUL installations
don't run into any issues. We can't just release version 1.4.0 and 1.5.0
and day and ask everyone to upgrade twice on the same day.
Instead, we're following a different plan:
* We're going to provide a Rake task (which will require Paperclip) to
migrate existing files
* We still use Paperclip to generate link and image tags
* New files are handled using both Paperclip and ActiveStorage; that
way, when we make the switch, we won't have to migrate them, and in
the meantime they'll be accessible thanks to Paperclip
* After we make the switch, we'll update the `name` column in the active
storage attachments tables in order to remove the `storage_` prefix
Regarding our handling of new files, the exception are cached
attachments. Since those attachments are temporary files used while
submitting a form and we have to delete them afterwards, we're only
handling them with Paperclip. We'll handle these ones in version 1.5.0.
Note the task creating the dev seeds was failing after these changes
with an `ActiveStorage::IntegrityError` exception because we were
opening some files without closing them. If the same file was attached
twice, it failed the second time.
We're solving it by closing the files with `File.open` and a block. Even
though we didn't get any errors, we're doing the same thing in the
`Attachable` concern because it's a good practice to close files after
we're done with them.
Also note we have to change the CKEditor Active Storage code so it's
compatible with Paperclip. In this case, I haven't been able to write a
test to confirm the attachment exists; I was getting the same
`ActiveStorage::IntegrityError` mentioned above.
Finally, we're updating the site customization image controller to use
`update` so the image and the attachment are updated within the same
transaction. This is also what we do in most controllers.
[1] https://www.youtube.com/watch?v=tZ_WNUytO9o
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).
By default, Rails disables submit inputs (<input type="submit">) when
they're pressed so we avoid a double-submission when users click the
button twice.
However, Rails does not disable submit buttons (<button type="submit">)
when they're pressed. This means there's a chance users might press the
button several times. Even if most our table actions are idempotent, it
might cause certain issues. For instance, pressing the "Delete" button
twice means the second request might raise an
`ActiveRecord::RecordNotFound` exception.
Disabling the button also gives feedback to users, letting them know
they've correctly clicked the button.
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.
This way screen reader users will know which record they're going to
access when focusing on a link to a certain action. Otherwise they'd
hear something like "Edit, link", and they wouldn't know which record
they'll end up editing if they follow the link.
This way it will be easier to change the behavior of all table actions,
like adding ARIA attributes. In the past, when we changed the behavior
of the `link_to` method, we had to change all table action classes.
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".
