We were getting an exception in this case, which was OK I guess since
this shouldn't happen if the application is used in a normal way, but we
can simplify the code a little bit if we make the `recipients` code
return an empty list of users.
Note that the behavior of the `AdminNotification#list_of_recipients` and
`Newsletter#list_of_recipient_emails` methods is now slightly different;
previously they returned `nil` when given an invalid segment recipient,
while now they return an empty array. I haven't found a place where this
change is relevant. For example, in both of these models, the `deliver`
method used to raise an exception when given an invalid segment while
now it doesn't, but we always check the user segment is valid before
calling the `deliver` method anyway, so it doesn't really affect the
application.
We were getting a warning by CodeQL regarding a possible code injection
in the `send(segment)` code.
In practice, this wasn't a big deal because the `self.recipients` method
is only called in the admin section, meaning only admin users could try
to take advantage of the code injection, and because this code is rarely
called with an invalid segment due to several conditions in the code
checking that the user segment is valid, with the only exception being
the `generate_csv` action in the `Admin::EmailsDownloadController`.
In any case, now we're checking that the segment is valid before calling
the `send` method. Since now we're making sure that the segment is valid
in the `recipients` method itself, we can remove this check from methods
calling it.
The `parameterize` method uses the `I18n.transliterate` method, whose
documentation says:
```
I18n.transliterate("Ærøskøbing")
=> "AEroskobing"
I18n.transliterate("日本語")
=> "???"
```
That means we can't use it for dictionaries where characters don't have
a transliteration to the latin alphabet.
So we're changing the code in order to only transliterate characters
with a transliteration to the latin alphabet.
Note the first example ("Česká republika") already worked with the
previous code; the test has been added to make sure accented characters
are handled properly.
This way we don't have to use the `send` method in other places, like
the AdminNotification class, and we can change the internal
implementation at any point.
We're going to add geozones as user segments, so it's handy to have the
method in the UserSegments class.
We're also changing the `user_segment_emails` parameter name for
consistency and simplicity.
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're using `eq` and `match_array` in most places, but there were a few
places where we were still checking each element is included in the
array. This is a bit dangerous, because the array could have duplicate
elements, and we wouldn't detect them with `include`.
While there are other variables in these tests, they're not part of the
setup of the test, and so these ones can be removed while keeping the
code easy to read.
This fixes a validation error being raised in these specs, as we were
missing some attributes that are required. Using the factory trait the
missing attributes are correctly filled out.
The list of emails for sending newsletters will always be ordered by
user creation date, so it will be easier to debug and to know for
which users the email has been sent.
Why:
UserSegments are not only used for Newsletters or Email downloads, but
also for internal Global Notifications. We don't want to have that scope
hardcoded inside UserSegments as users that have opted-out from the
newsletter should still be recipients of global notifications.
How:
Removing the scope from the UserSegments `all_users` method that acts as
base for all the other segments. Including that `newsletter` scope only
on the places that is relevant:
* When listing recipients for a newsletter
* When downloading a listing emails that can be newsletter recipients
Also updated relevant tests
