Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

a bunch of fixes to the daily email PR #6728

Merged
merged 13 commits into from
May 28, 2020
Merged

Conversation

owocki
Copy link
Contributor

@owocki owocki commented May 27, 2020

makes a bunch of changes to the new daily email so that itll actually look good in prod:

  1. adds the ability to add arbitrary dates (for workshops, livestreams, grant start dates, etc)
  2. adds links to activity feed
  3. hackathon visibility filter
  4. progressively email, starting with 20% of the userbase.. even if they have no bounties.
  5. adds founder announcements to th etop
  6. speed logs

tested at http://localhost:8000/_administration/email/new_bounty_daily

@owocki
Copy link
Contributor Author

owocki commented May 27, 2020

this is ready for review, would like to get it in if possible to make usre the first new daily email ppl get is good.

note: i cant run the linter due to a few upstream issues @sebastiantf @gitcoinco/engineers


app/assets/v2/css/town_square.css
 1300:1  ✖  Unexpected duplicate selector ".dark-mode .alert-info", first used at line 1281                                 no-duplicate-selectors
 1304:1  ✖  Unexpected duplicate selector ".dark-mode .web3modal-provider-name", first used at line 1285                    no-duplicate-selectors
 1307:1  ✖  Unexpected duplicate selector ".dark-mode .gc-search-box input", first used at line 1288                        no-duplicate-selectors
 1310:1  ✖  Expected selector ".dark-mode .gc-search-result__content p" to come before selector "#get-started p"            no-descending-specificity
 1310:1  ✖  Expected selector ".dark-mode .gc-search-result__content p" to come before selector "#get-started a p"          no-descending-specificity
 1310:1  ✖  Unexpected duplicate selector ".dark-mode .gc-search-result__content p", first used at line 1090                no-duplicate-selectors
 1313:1  ✖  Unexpected duplicate selector ".dark-mode .gc-search-result__content, .dark-mode .gc-search-box, .dark-mode     no-duplicate-selectors
            .web3modal-provider-container, .dark-mode .web3modal-provider-wrapper, .dark-mode .web3modal-modal-card",
            first used at line 1291

url = models.URLField(db_index=True)
comment = models.TextField(max_length=255, default='', blank=True)

@property
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo in the future, make grants rounds, hackathons, workshops auto create these objects.

upcoming_events.append({
'event': upcoming_grant,
'title': upcoming_grant.title,
'image_url': upcoming_grant.logo.url if upcoming_grant.logo else f'{settings.STATIC_URL}v2/images/emails/grants-neg.png',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didnt make any sense to show grants by their clr calc date IMHO.

if we want to include trending grants in the future.. we should do it in its own section. @sebastiantf lmk if you waant to work on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebastiantf another thing i could use help on is that the email takes about 2s to calculate. happy to write a v2 ticket where you do the above and also profile/improve the performance of it so we can send 30k/day (scaling up to 100k in the next year)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didnt make any sense to show grants by their clr calc date IMHO.

if we want to include trending grants in the future.. we should do it in its own section. @sebastiantf lmk if you waant to work on that.

Yeah sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logging the perf issues here #6729

a few more things i want to do...

  • add to subject line: upcoming dates, num notifications, num announcements, etc
  • what else?? will know more as this ships.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grants

@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

Merging #6728 into master will increase coverage by 0.01%.
The diff coverage is 27.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6728      +/-   ##
==========================================
+ Coverage   26.67%   26.69%   +0.01%     
==========================================
  Files         293      293              
  Lines       27817    27863      +46     
  Branches     4109     4112       +3     
==========================================
+ Hits         7420     7437      +17     
- Misses      20132    20156      +24     
- Partials      265      270       +5     
Impacted Files Coverage Δ
app/marketing/mails.py 11.16% <0.00%> (-0.12%) ⬇️
app/marketing/utils.py 29.03% <ø> (-0.23%) ⬇️
app/townsquare/models.py 57.95% <ø> (ø)
app/townsquare/utils.py 15.78% <0.00%> (-0.88%) ⬇️
app/retail/emails.py 21.77% <25.00%> (+0.15%) ⬆️
app/marketing/views.py 12.04% <33.33%> (+0.39%) ⬆️
...arketing/management/commands/new_bounties_email.py 49.20% <36.36%> (+1.83%) ⬆️
app/marketing/models.py 64.03% <72.72%> (-0.02%) ⬇️
app/marketing/admin.py 84.41% <100.00%> (+0.20%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e115f0...7514ef1. Read the comment docs.

@owocki
Copy link
Contributor Author

owocki commented May 27, 2020

looks like upstream linter issues have been fixed. thanks @octavioamu

<div class="dropdown-menu dropdown-menu-right shadow font-smaller-4 px-0">
{% if can_pin %}
<a class="pin_activity mr-2 dropdown-item px-3 font-smaller-5" data-toggle="tooltip" title="Pinned post will display at the top of the feed." href=#
data-url="{{row.url}}"
<a class="pin_activity mr-2 dropdown-item px-3 font-smaller-5" data-toggle="tooltip" title="Pinned post will display at the top of the feed." hrefo=#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hrefo ?

<a class="pin_activity mr-2 dropdown-item px-3 font-smaller-5" data-toggle="tooltip" title="Pinned post will display at the top of the feed." href=#
data-url="{{row.url}}"
<a class="pin_activity mr-2 dropdown-item px-3 font-smaller-5" data-toggle="tooltip" title="Pinned post will display at the top of the feed." hrefo=#
data-url="{{rw.url}}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rw ? :P

@@ -287,7 +287,7 @@
>
<span class="action {% if row.metadata.liked %}open{%endif%}">
{% if for_email %}
<img class="fa-icon-png" src=""{% static "v2/images/emails/heart-regular.png" %}">
<img class="fa-icon-png" src="{{base_url}}static/v2/images/emails/heart-regular.png">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did {% static not work ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -152,7 +196,7 @@ <h3>{% trans "Matching your profile" %}</h3>
{% endif %}

<p>
{% trans "You are receiving this email because your email address is on the notification list" %}. <a href="{% url 'email_settings' subscriber.priv %}" style="text-decoration: underline;">{% trans "Manage Email Settings" %}</a>.
{% trans "You are receiving this email bc you are subscribed to daily emails." %} <a href="{% url 'email_settings' subscriber.priv %}" style="text-decoration: underline;">{% trans "Manage Email Settings" %}</a> | <a href="{% url 'email_settings' subscriber.priv %}?type=new_bounty_notifications" style="text-decoration: underline;">{% trans "Unsubscribe" %}</a>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bc ?

@thelostone-mc thelostone-mc merged commit afe56d7 into master May 28, 2020
@thelostone-mc thelostone-mc deleted the kevin/new_daily_email branch June 27, 2020 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants