Note we don't cast negative votes when users remove their support. That
way we provide compatibility for institutions who have implemented real
negative votes (in case there are / will be any), and we also keep the
database meaningful: it's not that users downvoted something; they
simply removed their upvote.
Co-Authored-By: Javi Martín <javim@elretirao.net>
Co-Authored-By: Julian Nicolas Herrero <microweb10@gmail.com>
Since we're going to add an action to remove supports, having a separate
controller makes things easier.
Note there was a strange piece of code which assumed users were not
verified if they couldn't vote investments. Now the code is also
strange, since it assumes users are not verified if they can't create
votes. We might need to revisit these conditions if our logic changes in
the future.
It was hard to notice what was going on when supporting one investment
which was at the bottom of the investment index page.
I wonder whether we should add the title of the investment to this text;
I'm not doing so because we don't do that anywhere else.
If I'm right, the `investment_votes` instance variable only exists to
avoid several database queries to get whether the current user has
supported each of the investments.
However, that doesn't make much sense when only one investment is shown.
In this case, the number of queries stays the same, and so we can
simplify the code by rendering the component with an optional parameter.
Using links combined with JavaScript to generate POST requests to the
browser has a few issues.
An obvious one is that it doesn't work for users without JavaScript
enabled (which lately I've noticed are more common than I thought, even
though I've been one of them for years). These users will reach a 404
page.
Since CONSUL isn't currently designed to work without JavaScript
enabled, let's ignore this one for now.
A not so obvious issue is screen reader users might expect the link to
take them somewhere instead of performing an action (in this case,
sending a form to the server).
There might be more issues I'm unaware of. Quoting DHH [1]:
"Turning ahrefs into POSTs is a bit of an anti-pattern, especially for
a11y reasons. Better to use button_to with a styling."
So we're using a button instead. This way we can also simplify the code
and make the button disabled for unidentified users, which automatically
makes it impossible to focus it using the keyboard.
A possible disadvantage of using `button_to` is it will create a form
tag which will be announced to screen readers as a form landmark. I've
tested it with my screen reader and everything worked fine for me, but
some screen reader users might interact with these forms in a different
way and find it confusing, particularly in the case where the button is
disabled.
With this change, we're only changing links for buttons in one place.
There are other places where we should do similar changes.
[1] See issue 33 in https://github.com/hotwired/turbo-rails/
We meant to check that the link to support a different investment didn't
have an alert dialog. However, we were testing by trying to support the
same investment twice, and here there was no link at all, so the
assertion "there's no link with a dialog" always passed even when the
application didn't behave properly.
Now we check there's a link there, and that link doesn't have an
associated alert dialog.
In regular system tests, we prefer testing against the text of the link
because it's easier to recognize which link we're talking about.
I was wondering whether we should remove the `title` attribute
completely and use just the `aria-label` attribute. Quoting The Paciello
Group blog [1]:
"If you want to hide content from mobile and tablet users as well as
assistive tech users and keyboard only users, use the title attribute."
However, there's a case where mouse users might find the title attribute
useful. When visiting an investment page, the connection between the
"Support" link and the investment is not as clear as it is in the
investments index page, so it might not be clear what you're supporting.
As mentioned, though, this information will only be relevant for mouse
users, who nowadays represent less than half of our users.
[1] https://www.tpgi.com/using-the-html-title-attribute-updated/
This way blind screen reader users will know which project they're
supporting. In a list of investments, context might not be clear when a
link saying "Support" or "Support this project" is announced, but a link
saying "Support Renovate sidewalks in Main Street" is less ambiguous.
When identified users accessed the investments page, we were using the
`aria-hidden` attribute to hide this link from screen readers since
unidentified users can't support investments.
However, the link was still focusable using keyboard navigation. This
resulted in screen reader users reaching the link and being able to
click it, but getting no feedback at all about where they were or what
they were clicking on.
This is why the fourth ARIA rule says: "Do not use role="presentation" or
aria-hidden="true" on a focusable element" [1].
So we're replacing the link with a non-interactive element instead, like
we do in other places like proposals.
The accessibility of this part of the page for unidentified users still
has some issues; here we're only improving it up to a certain point.
[1] https://www.w3.org/TR/using-aria/#4thrule
The `js-` prefix (which I admit I'm not fond of) is usually used to
indicate the class is used by JavaScript files, not for using it in test
files. And in all the other similar tests, we're using the `in-favor`
class instead of the `js-in-favor` class.
This reverts commit 83fe74d53.
When supporting an investment in the management section through the
investment view, we were accessing an action in the public investments
controller. This meant the manager was the one supporting the investment
(as they'd be the `current_user` in this controller) and not the managed
user.
In the case of groups with many headings, voting the first time requires
a confirmation and then a regular (non-AJAX) request takes place. In
this case, users were redirected to the public area instead of remaining
in the management area.
Using the proper URL to vote solves the problem.
Note there was a comment about one of these tests failing in Travis.
Most probably the test failed because there was no expectation after
clicking the link with the investment title, so the "Support" button
(which is also present in the index page) was clicked before the
investment view was loaded.
The key was declared twice and so the first one ("Support this project")
was overwritten.
We're grouping all keys related to the investment list together in order
to reduce the chances of this issue happening again (or, at least, in
this part of the code).
Note one of the tests dealing with random results is a bit flaky; since
it's a permutation selecting 7 objects out of 12, it will fail about
once every 4 million times. We think this is acceptable.
Co-Authored-By: Julian Nicolas Herrero <microweb10@gmail.com>
In the form of creating a new investment was hiding the name of the
group if it had only one heading, but could be confusing to users if
there are, for example, five different groups of one heading.
The solution:
- If the budget has one group and one heading, the heading selector is
hidden.
- If the budget has one group and more than one heading, the group name
is hidden.
- If the budget has more than one group, the group name appears
regardless of the number of headings.
Note we're using an extra `<span>` element but we could use a CSS grid
layout instead. We're not using it because browser compatibility is only
94.56% at the time of writing.
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>
