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
Having exceptions is better than having silent bugs.
There are a few methods I've kept the same way they were.
The `RelatedContentScore#score_with_opposite` method is a bit peculiar:
it creates scores for both itself and the opposite related content,
which means the opposite related content will try to create the same
scores as well.
We've already got a test to check `Budget::Ballot#add_investment` when
creating a line fails ("Edge case voting a non-elegible investment").
Finally, the method `User#send_oauth_confirmation_instructions` doesn't
update the record when the email address isn't already present, leading
to the test "Try to register with the email of an already existing user,
when an unconfirmed email was provided by oauth" fo fail if we raise an
exception for an invalid user. That's because updating a user's email
doesn't update the database automatically, but instead a confirmation
email is sent.
There are also a few false positives for classes which don't have bang
methods (like the GraphQL classes) or destroying attachments.
For these reasons, I'm adding the rule with a "Refactor" severity,
meaning it's a rule we can break if necessary.
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 condition `params[:page_id] != 0` didn't work properly when editing
the homepage because in that case the parameter was `nil`, and the line
`SiteCustomization::Page.find(@card.site_customization_page_id)` raised
an exception because it couldn't find a page with a `nil` ID.
Fixing the issue while maintaining the check against `0` lead to complex
code, and so allowing `nil` in the database and assuming cards with no
`site_customization_page_id` belonged in the homepage seemed to be the
easiest solution.
Note there is some funkiness going on with class loadings
Had to create a `feed` and `widget_feed` table even though in this
first version the Widget::Feed includes only uses ActiveModel instead
of ActiveRecord, otherwise some specs failed
We’ll figure it out and clean up 😌
