Since we're only doing the convertion from bytes to megabytes in one
place, IMHO adding an extra method makes the code harder to read.
This way we don't have do include the DocumentsHelper in the specs
anymore, reducing the risk of possible method naming collisions.
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
In general, we always use relative URLs (using `_path`), but sometimes
we were accidentally using absolute URLs (using `_url`). It's been
reported i might cause some isuses if accepting both HTTP and HTTPS
connections, although we've never seen the case.
In any case, this change makes the code more consistent and makes the
generated HTML cleaner.
The name `html_safe` is very confusing, and many developers (including
me a few years ago) think what that method does is convert the HTML
contents to safe content. It's actually quite the opposite: it marks the
string as safe, so the HTML inside it isn't stripped out by Rails.
In some cases we were marking strings as safe because we wanted to add
some HTML. However, it meant the whole string was considered safe, and
not just the contents which were under our control.
In particular, some translations added by admins to the database or
through crowding were marked as safe, when it wasn't necessarily the
case.
Although AFAIK crowdin checks for potential cross-site scripting
attacks, it's a good practice to sanitize parts of a string potentially
out of our control before marking the string as 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.
