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

GDPR: Management command to send email to all gitcoin users #1252

Merged
merged 7 commits into from
May 24, 2018

Conversation

SaptakS
Copy link
Contributor

@SaptakS SaptakS commented May 24, 2018

Description

Management command so that we can send the GDPR policy update email to all the subscribed users of gitcoin

Checklist
  • linter status: 100% pass
  • changes don't break existing behavior
  • commit message follows commit guidelines
Affected core subsystem(s)

management

Testing
  1. Run ./manage.py gdpr_update_email --live
  2. The email will be sent if sendgrid is setup.

Screenshot of HTML:
screenshot-2018-5-24 zjqjro

Refers/Fixes

FIxes #1242

@SaptakS SaptakS requested a review from owocki May 24, 2018 11:52
@ghost ghost assigned SaptakS May 24, 2018
@ghost ghost added the in progress label May 24, 2018
@@ -0,0 +1,81 @@
'''
Copyright (C) 2017 Gitcoin Core

Choose a reason for hiding this comment

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

W291 trailing whitespace

from marketing.mails import gdpr_update
from marketing.models import EmailSubscriber

warnings.filterwarnings("ignore", category=DeprecationWarning)

Choose a reason for hiding this comment

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

W291 trailing whitespace

@SaptakS SaptakS force-pushed the gdpr/community-email-management branch from 9fb6e7c to 5a1f63a Compare May 24, 2018 12:08
@codecov
Copy link

codecov bot commented May 24, 2018

Codecov Report

Merging #1252 into master will decrease coverage by 0.2%.
The diff coverage is 5.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1252      +/-   ##
==========================================
- Coverage   31.57%   31.37%   -0.21%     
==========================================
  Files         118      119       +1     
  Lines        8186     8239      +53     
  Branches     1065     1072       +7     
==========================================
  Hits         2585     2585              
- Misses       5491     5544      +53     
  Partials      110      110
Impacted Files Coverage Δ
...marketing/management/commands/gdpr_update_email.py 0% <0%> (ø)
app/retail/emails.py 22.5% <28.57%> (+0.22%) ⬆️
app/marketing/mails.py 13% <8.33%> (-0.2%) ⬇️
app/app/settings.py 83.61% <0%> (-1.7%) ⬇️

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 84babc3...7ea2e8f. Read the comment docs.

<h1>{% trans "Gitcoin: Updated Terms & Policies" %}</h1>
<div style="text-align: left! important">
<p>
{% trans "Hello" %} {{subscriber}},
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note... we should get into the habit of using {{ var }} formatted variables in django templates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know. But since I saw this is the format followed most placed, so I thought of sticking with it to maintain uniformity. I literally changed from {{ var }} to {{var}} 😋

Copy link
Member

Choose a reason for hiding this comment

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

This is @mbeacom when me and @SaptakS try to help out :trollface:

not-helpful

Copy link
Contributor

Choose a reason for hiding this comment

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

😂

counter = 0
for to_email in email_list:
counter += 1
print("-sending {} / {}".format(counter, to_email))
Copy link
Contributor

Choose a reason for hiding this comment

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

We could remove .format here with: print('-sending, counter, '/', to_email) or print(f'-sending {counter} / {to_email}')

queryset = queryset.order_by('email')
email_list = set(queryset.values_list('email', flat=True))

print("got {} emails".format(len(email_list)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could eliminate .format here: print('got', len(email_list), 'emails') or `print(f'got {len(email_list)} emails')

)

def handle(self, *args, **options):

Copy link
Contributor

Choose a reason for hiding this comment

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

No new line between method and beginning logic/docstrings.

{% trans "As always, the entire Gitcoin team works every day to make Gitcoin a safe and trustworthy place to grow open source. We invite you to learn more about our outlined updates to our Policies and Terms of Use: " %}
</p>
<ul>
<li>{% blocktrans %}<a href="{{terms_of_use_link}}">Our updated Terms of Use </a>{% endblocktrans %}</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid issues with autogeneration of translations, I've been trying to avoid wrapping dynamic vars in blocktrans. We could update this to:
<li><a href="{{ terms_of_use_link }}">{% trans "Our updated Terms of Use" %} </a></li>

@@ -297,6 +297,22 @@ def render_bounty_startwork_expired(to_email, bounty, interest, time_delta_days)
return response_html, response_txt


def render_gdpr_update(to_email):

Copy link
Contributor

Choose a reason for hiding this comment

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

No new line here


params = {
'subscriber': get_or_save_email_subscriber(to_email, 'internal'),
'terms_of_use_link': '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these params have value to reverses of their relevant urls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I was about to comment. But all the links are still not active in the master branch. Hence I will add them once the other PRs get merged.

@SaptakS SaptakS force-pushed the gdpr/community-email-management branch from 5a1f63a to 6a296f8 Compare May 24, 2018 12:26
@SaptakS SaptakS force-pushed the gdpr/community-email-management branch from 6a296f8 to 82dbf51 Compare May 24, 2018 12:36
@ghost ghost assigned mbeacom May 24, 2018
mbeacom
mbeacom previously approved these changes May 24, 2018
def render_gdpr_update(to_email):
params = {
'subscriber': get_or_save_email_subscriber(to_email, 'internal'),
'terms_of_use_link': '', #TODO

Choose a reason for hiding this comment

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

E261 at least two spaces before inline comment
E262 inline comment should start with '# '

params = {
'subscriber': get_or_save_email_subscriber(to_email, 'internal'),
'terms_of_use_link': '', #TODO
'privacy_policy_link': '', #TODO

Choose a reason for hiding this comment

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

E261 at least two spaces before inline comment
E262 inline comment should start with '# '

'subscriber': get_or_save_email_subscriber(to_email, 'internal'),
'terms_of_use_link': '', #TODO
'privacy_policy_link': '', #TODO
'cookie_policy_link': '', #TODO

Choose a reason for hiding this comment

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

E261 at least two spaces before inline comment
E262 inline comment should start with '# '

@SaptakS SaptakS force-pushed the gdpr/community-email-management branch from 0260d0b to 86b9700 Compare May 24, 2018 13:40
@owocki
Copy link
Contributor

owocki commented May 24, 2018

note: we should not send this email to users who have previously opted out of newsletters.

that should mean that if their es.preferences['level'] is ['lite', 'lite1', 'regular', 'nothing'] we don't send it (in the old schema). in the new schema, if they receive roundup emails should dictate if they recevie these emails

@owocki
Copy link
Contributor

owocki commented May 24, 2018

i believe we need to send this today

def render_gdpr_update(to_email):
params = {
'subscriber': get_or_save_email_subscriber(to_email, 'internal'),
'terms_of_use_link': '', # TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

these need to be filled in

probably https://gitcoin.co/legal/xxxx

@SaptakS
Copy link
Contributor Author

SaptakS commented May 24, 2018

@owocki @mbeacom I have added links and also the suppression check same as the roundup, so should be good to go.

@SaptakS SaptakS force-pushed the gdpr/community-email-management branch from b26110d to 145d2c4 Compare May 24, 2018 20:19
@@ -23,6 +23,7 @@
from django.contrib.admin.views.decorators import staff_member_required
from django.http import HttpResponse
from django.shortcuts import redirect
from django.urls import reverse

Choose a reason for hiding this comment

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

F401 'django.urls.reverse' imported but unused

@SaptakS SaptakS force-pushed the gdpr/community-email-management branch from feeffaf to 7ea2e8f Compare May 24, 2018 20:35
@mbeacom mbeacom merged commit d665bb8 into gitcoinco:master May 24, 2018
@ghost ghost removed the in progress label May 24, 2018
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.

GDPR: Write Email To Gitcoin Community
5 participants