Bumps [rubocop-rails](https://github.com/rubocop/rubocop-rails) from 2.20.2 to 2.21.2.
- [Release notes](https://github.com/rubocop/rubocop-rails/releases)
- [Changelog](https://github.com/rubocop/rubocop-rails/blob/master/CHANGELOG.md)
- [Commits](https://github.com/rubocop/rubocop-rails/compare/v2.20.2...v2.21.2)
---
updated-dependencies:
- dependency-name: rubocop-rails
dependency-type: direct:development
update-type: version-update:semver-minor
...
Note version 2.21.0 relaxes the default `Include` path for
`Rails/FindEach`, and so this version can find and correct offenses
outside the `app/models/` folder [1].
Also note this version replaces `unless something.include?` with `if
something.exclude?`; since we don't use the `exclude?` method anywhere,
we're removing the `include?` method from the list of methods checked by
this cop.
Finally, the Rails/HttpStatus method now returns a false positive when
rendering a dashboard partial and passing the `status` variable. In
order to avoid this issue, we could change the name of the local
variable or move the partial to a component, but for now we're simply
excluding these files for this cop.
[1] https://github.com/rubocop/rubocop-rails/pull/1059/commits/0066b3505
Signed-off-by: dependabot[bot] <support@github.com>
RSpec/FilePath is deprecated since rubocop-rspec 2.24.0 and will be
removed in rubocop-rspec 3.0 in favor of RSpec/SpecFilePathFormat and
RSpec/SpecFilePathSuffix.
In order to the display a warn text on the last attempt
before the account is locked, we need update
config.paranoid to false as the devise documentation
explains.
Adding "config.paranoid: false" implies further changes
to the code, so for now we unncomment the default value
"config.last_attempt_warning = true" and update it to false.
As it seems that adding complexity to the password is something that
might be wanted from the Consul applications, we added the necessary
changes to allow it.
In this version we simply:
- Uncomment the configuration variable "password_complexity"
- Set this variable without any restrictions
- Adapt the application so that everything still works normally.
One of the things that had to be done to adapt the application
was to remove the overwriting of the "self.included" method.
The original idea of overwriting the "self.included" method seems
to be the possibility of being able to overwrite the
:current_equal_password_validation validation.
The problem comes from the fact that by only calling that validation,
the rest of the validations that are defined (in this case
"password_complexity") are no longer applied.
It seems like a good idea to remove the overwrite of the "self.included"
method to allow all the defined validations to be applied and simply
overwrite the :current_equal_password_validation method so that
everything behaves the same.
:allow_passwords_equal_to_email configuration has been enabled too,
in order to allow existing records with this configuration.
Another change made was to uncomment the line:
and to keep everything working the same set the value to false:
config.email_validation = false.
This change has had to be made because in the documentation of
devise-security it says the following:
In other words, if we want to use the :secure_validatable module
we have to enable this configuration even if its value is "false".
If we kept the configuration variable commented out:
The following error appears:
"uninitialized constant Devise::Models::SecureValidatable::EmailValidator".
So it has been verified that if before making any change we
decommented the line and added the value of "false", the application
worked as normal.
In these commits ffe9ac70d8def3 we updated the devise-security version.
In these versions the 'password_regex' configuration key and some comments
were changed.
We update this file in order to use the new configuration key 'password_complexity' and keep comments updated.
We were already opening them in the same window because we were
accidentall sanitizing the `target` attribute, but now we're making the
point more explicit.
This will help search engines know these links point to external sites
and it'll make it possible to style these links using the
`[rel~=external]` selector. AFAIK, assistive techonologies don't use
this attribute to notify people about external links, though.
This is a funny one, because we were accidentally opening them in the
same window without intending to do so since commit 928312e21, since the
`sanitize` method removes the `target` attribute. So the test we're
adding already passed without these changes.
Just like we're doing with other external links.
We already mention that it's an external video, so there's no need to
explicitly indicate it in the link.
As mentioned in earlier commits, opening external links in a new
tab/window results in usability and accessibility issues.
Since these links are usually at the top or bottom of the page and
contain icons of well-known sites, IMHO there's no need to even notify
people that these are external links.
Since we're no longer using the `shared.target_blank` translation inside
a sentence, we can remove the space and parenthesis in the translations.
We were doing it this way because managers usually have the management
section open at all times. However, this might not always be the case,
and by opening links in a new tab, we're taking control away from them.
If managers would like to keep the management section open, they can
open the link in a new tab, and if they open it in the same tab, they
can go back to the management section by either clicking the browser's
back button or clicking on the navigation link to the management
section.
Note that, unlike what we did in the admin section we're opening links
to budget investments on the same tab. There are two reasons for it; the
first one is that, in this case, there are no filters in the moderation
section that are lost after editing an investment, and the second one is
that, in this context, administrators usually don't go to the investment
in order to edit it, so they can just check something and use the
browser's back button to go back.
In the admin section, when clicking on a link that leads to a page in
the public area, sometimes the page was opened in the same window and
sometimes it would open in a new window, with no clear criteria
regarding when either scenario would take place.
This was really confusing, so now we're more consistent and open
(almost) every link in the same window. The main reason behind it is
simple: if we add `target: _blank`, people who want to open those links
in the same window can no longer do so, so we're taking control away
from them. However, if we don't add this attribute, people can choose
whether to open the link on the same tab or to open it on a new one,
since all browsers implement a method to do so.
More reasons behind this decision can be found in "Opening Links in New
Browser Windows and Tabs" [1].
We're keeping some exceptions, though:
* Opening the link to edit an investment on the same tab would result in
losing all the investment filters already applied when searching for
investments, so until we implement a way to keep these filters, we're
also opening the link to edit an investment in a new tab
* For now, we're also opening links to download files in a new window;
we'll deal with this case in the future
[1] https://www.nngroup.com/articles/new-browser-windows-and-tabs/
We were opening these links in a new tab/window because we assume they
were external links.
But, on the one hand, we don't even know whether these links are
external, since they could also point to URLs from our site. And, on the
other hand, opening external links in new windows results in usability
issues as well [1, 2].
On top of that, old browsers have security issues when opening links in
new tabs unless we add `rel="noopener"` [3], and we aren't doing so.
[1] https://www.nngroup.com/articles/new-browser-windows-and-tabs
[2] https://css-tricks.com/use-target_blank
[3] https://mathiasbynens.github.io/rel-noopener/
Out of the usability issues I've experienced when using Consul
Democracy, the biggest one has arguably been the fact that the link to
edit a proposal opens in a new tab. I guess the reasoning behind it is
that the page to edit a proposal is not part of the proposals dashboard,
but what the hell! Imagine if every link to edit something opened in a
new tab...
So we're reducing the impact of this nonsense by opening most dashboard
links in the same window; for now, we're still opening in a new window
links to download files and links that might point to external websites.
We'll address those ones in the future.
The previous version worked fine when using Capistrano to deploy a new
release, but it did not work for other commands, like `cap npm install`
or `cap puma:start`, as they do not create a release. Therefore, the
`git:create_release` was never called, and the `map_node_bins` was not
invoked.
Since we need node binaries available to the deploy user when executing
any process that runs the application, like puma, delayed_job, and rake
tasks, among others, it makes sense to load FNM to use the correct Node
version in the same places where Capistrano loads RVM to load the correct
Ruby version.
With dynamic loading of the `fnm_setup_command`, we get the correct path
when deploying a new release and also when running any other capistrano
command that does not deploy a new release.
Most screen readers don't notify when a link is about to open in a new
window [1], so we're indicating it, like we were already doing in most
places with similar links.
We could also add a visual indicator, but since links inside labels
already have accessibility issues, giving more attention to these links
might make matters worse.
[1] https://www.powermapper.com/tests/screen-readers/navigation/a-target-blank/
We used to open these links in new tabs, but accidentally stopped doing
so in commit 75a28fafc.
While, in general, automatically opening a link in a new tab/window is a
bad idea, the exception comes when people are filling in a form and
there are links to pages that contain information which will help them
fill in a form.
There are mainly two advantages of this approach. First, it makes less
likely for people to accidentally lose the information they were filling
in. And, second, having both the form and a help page open at the same
time can make it easier to fill in the form.
However, opening these links in new tabs also has disadvantages, like
taking control away from people or making it harder to navigate through
pages when using a mobile phone.
So this is a compromise solution.
IMHO the best solution would be to completely remove this checkbox on
forms that require registration. Other than the fact that people have
already agreed with these terms when registering (although I guess these
terms might have changed since then) and that approximately 0% of the
population will read the conditions every time they agree with them,
there's the fact that these links are inside a label and people might
accidentally click on them while trying to click on the label in order
to check the checkbox.
I guess the idea is that these conditions might have changed since the
moment people registered. In a fair world, checking "I agree" would have
no legal meaning because it's unreasonable to expect that people will
read these conditions every time they fill in a form, so whatever we're
trying to do here would be pointless.
But, since I'm not sure about the legal implications of removing this
field in a world where you have to inform people that websites requiring
identification use cookies, for now the field will stay where it is.
In some places, we were using `blank` instead of `_blank`. Most browsers
treat `blank` like `_blank`, though, so most people didn't experience
any difference.
In another place, we were incorrectly passing the `target` option inside
an `options:` hash, resulting in invalid HTML.
Quoting usability experts Jakob Nielsen and Anna Kaley [1]:
> [Opening PDF files in new tabs] is problematic, because it assumes
> users will always do the exact same things with certain file formats,
> which isn’t always the case.
There are many examples of this situation. For example, some people
(myself included) configure their browser so it downloads PDF files
instead of opening them in the browser. In this situation, a new tab is
opened, a blank page is displayed, the file is downloaded, and then
either the tab is closed or the blank page needs to be manually closed.
The end result is really annoying.
Other situations include people who use a mobile phone browser, where
navigating through tabs is generally much harder than doing so on a
desktop browser.
But IMHO the most important point is: every browser already provides a
way to open "regular" links in a new tab, so people can choose what to
do, but if we decide to open the link in a new tab, we take control away
from them, and people who'd like to open the link in the same tab might
feel frustrated.
In these cases, the links either say "download" or include the word
"PDF", so people know in advance that they're going to download/open a
PDF file, and so we're giving them information and, by removing the
`target` attribute, we're giving them control over their browser so they
can choose what's convenient for them.
[1] https://www.nngroup.com/articles/new-browser-windows-and-tabs
