Add geozones as user segments #2859
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
MariaCheca
changed the title
MariaCheca
force-pushed
the
backport_1579-user_segments_for_geozones
branch
3 times, most recently
from
a0ac5f4
to
5d0a400
Compare
javierm
force-pushed
the
backport_1579-user_segments_for_geozones
branch
from
5d0a400
to
9972ec3
Compare
houndci-bot
reviewed
May 29, 2019
javierm
changed the title
javierm
force-pushed
the
backport_1579-user_segments_for_geozones
branch
3 times, most recently
from
0b5b339
to
62d023a
Compare
houndci-bot
reviewed
May 30, 2019
javierm
force-pushed
the
backport_1579-user_segments_for_geozones
branch
from
62d023a
to
bcf2aaa
Compare
javierm
force-pushed
the
backport_1579-user_segments_for_geozones
branch
2 times, most recently
from
4c98389
to
38c3882
Compare
javierm
force-pushed
the
backport_1579-user_segments_for_geozones
branch
from
38c3882
to
3065e9a
Compare
javierm
force-pushed
the
backport_1579-user_segments_for_geozones
branch
3 times, most recently
from
ca9c1c2
to
a7604db
Compare
javierm
force-pushed
the
backport_1579-user_segments_for_geozones
branch
from
a7604db
to
2414b86
Compare
We're going to make it dynamic using the geozones. Besides, class methods can be overwritten using custom models, while constants can't be overwritten without getting a warning [1]. Makes the definition of segments with geozones a little cleaner. I think it’s worth it, compared to the slight memory gain of using a constant [2]. [1] warning: already initialized constant UserSegments::SEGMENTS [2] https://stackoverflow.com/questions/15903835/class-method-vs-constant-in-ruby-rails#answer-15903970
We generally use "#" to describe instance methods and "." to describe class methods.
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.
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.
javierm
force-pushed
the
backport_1579-user_segments_for_geozones
branch
from
2414b86
to
9bb8c20
Compare
We were testing the creation of newsletters and admin notifications for each existing segment, which IMHO is a bit overkill, considering how slow system tests are. So far we don't have any reasons to believe creating newsletters and admin notifications will only work for some user segments, so we're testing a random one instead. Running these tests on my machine is now about 15 seconds faster.
javierm
force-pushed
the
backport_1579-user_segments_for_geozones
branch
from
9bb8c20
to
18910d0
Compare
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.
taitus
approved these changes
Dec 20, 2021
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.
References
Objectives
Allow sending newsletters and admin notifications to users of a specific geozone.
Visual changes
Before these changes
After these changes
Notes for reviewers
There are some changes over the original pull requests used in Madrid's fork. The original pull requests were done with the idea that geozones were already in the database and geozones never change (which was the case in Madrid). So they used YAML files to get the geozone's name, they assumed the database already existed (and so new installations might crash when running
db:create) and they defined the geozone segments methods when theUserSegmentsclass was loaded (meaning that changing the database in production wouldn't have any effect until the server was restarted).Here we're supporting the case of new installations (where geozones aren't already defined) and installations adding or removing geozones as time goes by.