When customizing CONSUL, one of the most common actions is adding a new
field to a form.
This requires modifying the permitted/allowed parameters. However, in
most cases, the method returning these parameters returned an instance
of `ActionController::Parameters`, so adding more parameters to it
wasn't easy.
So customizing the code required copying the method returning those
parameters and adding the new ones. For example:
```
def something_params
params.require(:something).permit(
:one_consul_attribute,
:another_consul_attribute,
:my_custom_attribute
)
end
```
This meant that, if the `something_params` method changed in CONSUL, the
customization of this method had to be updated as well.
So we're extracting the logic returning the parameters to a method which
returns an array. Now this code can be customized without copying the
original method:
```
alias_method :consul_allowed_params, :allowed_params
def allowed_params
consul_allowed_params + [:my_custom_attribute]
end
```
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.
We were seeing a timeout when queuing the 300.000+ emails to be
delivered
With this commit we are dealing asynchronously with this creations and
responding promptly, to avoid the web timeout of 30 seconds
Why:
Both Newsletters and Email Downloads need the same logic: To extract the
emails from all the users in the segment that have newsletter flag
active, removing all empty email values.
How:
1- UserSegments#user_segment_emails holds that repeated logic and is used
on both Newsletter & EmailDownload.
2- Rename Newsletter#list_of_recipients to list_of_recipient_emails as
it is more descriptive. There is no need to pass entire Users around,
only the emails are needed at Mailer#newsletter method.
3- Cleanup Newsletter#list_of_recipient_emails model spec scenario
Why:
Newsletters without a valid user segment can't be sent since there is no
recipient user email list.
How:
* Hiding the send button at the form
* Preventing an invalid newsletter from being delivered at the controller
Why:
Newsletter attribute `segment_recipient` is an integer to be used as
enum. There's no advantage to store a number instead of an string if the
ammount of elements in the table is not going to be huge, or we can take
advantage of using an enum.
Also maintaining both Newsletters enum paired with UserSegments::SEGMENTS
would be a maintenance burden.
How:
* Migration to change segment_recipient column from integer to string
* Removing enumeration from Newsletter model class
* Using UserSegments::SEGMENTS instead of Newsletter.segment_recipients
or integer values
