Tags and help links can be edited, but aren't used anywhere. Since we
don't know what the intended behavior was, I'm removing them for now.
My best guess is tags were supposed to be used so investments for a
budget can only be assigned tags present in the budget. Achieving that
behavior wouldn't be a trivial task.
We need to add a hidden field for each group of check boxes, so if we
don't check anything, the hidden field is sent to the server, indicating
nothing was selected. Without the hidden field, the server will not know
anything has been done to the check boxes.
The easiest way to do it is using `collection_check_boxes`, which also
adds labels to every check box.
They weren't caught by `i18n-tasks` because there are places were we use
`t("budgets.edit.#{variable}"`, which marks as used all translations
starting with `budgets.edit`
We were using two different systems to set translations in JavaScript:
to set the text for languages, we were using data attributes, and to set
the text for staff members, we were using AJAX calls.
I find data attributes keep the code more simple, since there's no need
to define an extra route and controller action. Furthermore, the user
experience is better because response times are faster.
So now both places use data attributes.
Unfortunately this feature wasn't properly reviewed and tested, and it
had many bugs, some of them critical and hard to fix, like validations
being skipped in concurrent requests.
So we're removing it before releasing version 1.1. We might add it back
in the future if we manage to solve the critical issues.
This commit reverts commit 836f9ba7.
The `and` and `or` keywords are not equivalent to `&&` and `||` and its
use is counterintuitive. Here's an example
```
good = true && false # good if false
bad = true and false # bad is true
```
The reason is `and` and `or` are control flow operators. So the code:
```
bad = true and false
```
Is equivalent to:
```
if bad = true
false
end
```
We were inconsistent on this one. I consider it particularly useful when
a method starts with a `return` statement.
In other cases, we probably shouldn't have a guard rule in the middle of
a method in any case, but that's a different refactoring.
We were very inconsistent regarding these rules.
Personally I prefer no empty lines around blocks, clases, etc... as
recommended by the Ruby style guide [1], and they're the default values
in rubocop, so those are the settings I'm applying.
The exception is the `private` access modifier, since we were leaving
empty lines around it most of the time. That's the default rubocop rule
as well. Personally I don't have a strong preference about this one.
[1] https://rubystyle.guide/#empty-lines-around-bodies
We were already using `find_by` most of the time.
Since there are false positives related to our `find_by_slug_or_id!` and
`find_by_manger_login` methods, which cannot be replaced with `find_by`,
I'm adding it indicating the "refactor" severity.
This will be the default behaviour in Rails 5.1, and it's a much better
approach.
I've checked the code and luckily there doesn't seem to be a single
place where we could accidentally stop the callback chain by returning
false in (for example) a `before_save` callback.
The jQuery html() function does not filter <script> tags, so if somehow
an attacker introduced a <script> in the translation, we would be
vulnerable to a XSS attack.
Note using $.parseHTML wouldn't solve the problem, since it doesn't
filter attributes in image tags.
Since changing the text of the part which doesn't have the count wasn't
very clean, I've added another <span> tag for the part with the
description, and so we can use jQuery's text() function to replace it.
We're going to upgrade our ruby version, and we need these tasks.
Note we now get a warning caused by `rvm1:install:ruby` invoking
`deploy:updating`. It doesn't seem to be an issue because we don't add
any hooks to `deploy:updating`, and neither do the rest of the gems we
use.
Old CONSUL nginx configurations will probably have a reference to a
unicorn socket. Making that file a symbolic link to a puma socket makes
it possible for the application to keep working without updating the
nginx configuration file.
Puma was adding commands to `rvm_map_bins`, which meant RMV1 wasn't
using the default value of `rvm1_map_bins`.
Changing the order we use to require `rmv1/capistrano3` and
`capistrano/puma` did not fix the issue.
Puma is the server we use in the development environment, so this way we
don't need to maintain two servers. Furthermore, puma seems to offer a
few advantages over unicorn (like multithreading) and no disadvantages.
Using the `_html` suffix in an i18n key is the same as using `html_safe`
on it, which means that translation could potentially be used for XSS
attacks.
Using the `_html` suffix automatically marks texts as HTML safe, so
doing so on sanitized texts is redundant.
Note flash texts are not sanitized the moment they are generated, but
are sanitized when displayed in the view.
Although this translation has HTML, we aren't marking them as HTML safe
since we're using `I18n.t` instead of Rails' helper `t` method. So using
the `_html` suffix is counterintuitive in this case.
There's a slight chance an attribute like an author's name might contain
an attempt to perform XSS attacks. So, instead of marking the whole text
as HTML safe, we can sanitize it.
Also note I'm removing the `_html` suffix in the i18n key, since it's
got the same effect as using `html_safe`.
This way we can simplify the way we generate form fields. In some cases,
we also use the human attribute in table headers, which IMHO makes
sense.
I haven't moved all of them: for example, sometimes a label is
different depending on whether it's shown to administrators, valuators,
or users. And I haven't touched the ones related to devise, since I
wasn't sure about possible side effects.
Note I've also removed placeholders when they had the same text as their
labels, since they weren't helpful. On the contrary, the added redundant
text to the form, potentially distracting users.
We were monkey-patching FoundationRailsHelper::Formbuilder, which made
form customization difficult. We can inherit from it, which is the
standard way of extending what an existing class does, and make our form
the default one.
