Now that we have the option of voting in multiple headings per group,
the method of voting in a “different heading assigned” has become
deprecated and thus removed
Changing a group’s `to_param` to return the slug instead of the id,
breaks many tests in the user facing interface
We should use slugs in upstream soon, but it should be done in a
separate PR, bringing the whole slug implementation from Madrid’s fork
and the corresponding test coverage
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:
User with an empty email value (nil) should not appear in the recipient
list for a given UserSegment at Newsletters or Email Downloads.
How:
Using Enumerable#compact and Enumerable#select to filter out empty emails
Increasing Email Download feature spec and Newsletter model spec to cover
all possible scenarios including the nil email one.
Why:
A Newsletter can only be sent if the are available user recipient emails
and that means the `segment_recipient` value actually corresponds to a
function on the UserSegments class.
We could rely on the UserSegments::SEGMENTS constant as the list of
possible user segments functions that a Newsletter can use to gather
emails, so any value not included in that hash would not be valid.
But to be 100% sure the newsletter can get a recipients_list we should
just check if the UserSegments class has a method with same name as the
`segment_recipient` value.
How:
* Adding an validation method that checks if UserSegment has a method
with same name as the `segment_recipient` value.
* Adding an scenario to the Newsletter model spec to check this
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
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
As Budget::Investment has two relationships over commentable polymorphic
relationship, the counter_cache is counting the sum of both comments and
valuations.
We don't show valuations count anywhere, only the (public) comments so
we just use comments.count in this case
Why:
Only admins or valuators (for those investments they've assigned) can
create internal valuation comments on them.
How:
* Creating a new `comment_valuation` ability for admins and valuators in
the same manner the `valuate` ability works.
* Adding a validation at Comment model for those with `valuation` flag
active that checks if the author can make a valuation comment on the
commentable, as well as the respective active record error messages.
This will prevent comments from being created at a controller level as
well.
* Improving comment factory trait `valuation` to have an associated
investment, author that is a valuator and setting the valuator on the
valuators list of the investment
How:
Using a local variable at partials to set a hidden true/false value for
`valuation` parameter on the comment creation form.
Allowing that new param at the comment controller and using it when
building a new Comment.
Why:
Budget Investments already has an existing `comments` relation that is
on use. We need to keep that relation unaltered after adding the
internal valuation comments, that means scoping the relation to only
public comments (non valuation ones) so existing code using it will
remain working as expected.
A new second relation will be needed to explicitly ask for valuation
comments only where needed, again scoping to valuation comments.
How:
Adding a second `valuations` relationship and filtering on both
with the new `valuation` flag from Comment model.
Why:
Internal valuation comments are only for admins and valuators,
not for the public view.
How:
Adding a `not_valuations` scope and use it at the `public_for_api` one
