Admin newsletter email refactor #2474
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
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: 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
Newsletters with an invalid user segment should display a warning on show and index. Also those newsletters should not be sent.
Instead of the name of the User Segment we'll display an error message
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
When a newsletter doesn't have a valid user segment for the recipients list, the number of users should be 0.
Simple refactor to avoid creating unnecessary variables and make it easier to read.
clairezed
pushed a commit
to CDJ11/CDJ
that referenced
this pull request
Jun 26, 2018
…email_refactor Admin newsletter email refactor
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Where
What
This issue is a backport of a refactor on Newsletters & Email downloads
How & Screenshots & Test
Best check the PR at madrid's fork AyuntamientoMadrid#1285
Deployment
As usual
Warnings
None