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 reconsenet from EU residents #1249

Merged
merged 3 commits into from
May 24, 2018
Merged

GDPR reconsenet from EU residents #1249

merged 3 commits into from
May 24, 2018

Conversation

owocki
Copy link
Contributor

@owocki owocki commented May 23, 2018

Description

GDPR reconsenet from EU residents

screen shot 2018-05-23 at 5 31 37 pm

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

email

Testing

tested the mgmt command and in browser

Refers/Fixes

#1233

@@ -0,0 +1,56 @@
'''
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_reconsent
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

@@ -167,6 +168,17 @@ def render_new_bounty(to_email, bounties, old_bounties):

return response_html, response_txt

def render_gdpr_reconsent(to_email):

Choose a reason for hiding this comment

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

E302 expected 2 blank lines, found 1

@staff_member_required
def roundup(request):
response_html, _, _ = render_new_bounty_roundup(settings.CONTACT_EMAIL)
return HttpResponse(response_html)

@staff_member_required

Choose a reason for hiding this comment

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

E302 expected 2 blank lines, found 1

@codecov
Copy link

codecov bot commented May 23, 2018

Codecov Report

Merging #1249 into master will decrease coverage by 0.11%.
The diff coverage is 7.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1249      +/-   ##
==========================================
- Coverage    31.4%   31.28%   -0.12%     
==========================================
  Files         119      120       +1     
  Lines        8238     8279      +41     
  Branches     1072     1075       +3     
==========================================
+ Hits         2587     2590       +3     
- Misses       5541     5579      +38     
  Partials      110      110
Impacted Files Coverage Δ
app/app/urls.py 94.28% <ø> (ø) ⬆️
...marketing/management/commands/gdpr_reconsent_eu.py 0% <0%> (ø)
app/marketing/mails.py 13.07% <16.66%> (+0.07%) ⬆️
app/retail/emails.py 22.11% <22.22%> (ø) ⬆️

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 d665bb8...1b44e19. Read the comment docs.


queryset = EmailSubscriber.objects.all()

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

Choose a reason for hiding this comment

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

We can replace this with print('got', queryset.count(), 'emails') or print(f'got {queryset.count()} emails') to avoid the overhead of len() and utilize print() argument handling.

We hope that our content is useful to you. If you'd like to continue hearing from us, please update your subscription settings.
</p>
<p>
<a class="button" href="https://gitcoin.co/settings/email/{{subscriber.priv}}">
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 use reverse with request path here, so we don't have to update urls throughout the codebase if a path changes.

print("got {} emails".format(len(queryset)))

counter = 0
for es in queryset:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be able to use for counter, es in enumerate(queryset) here

app/app/urls.py Outdated
@@ -202,6 +202,7 @@
url(r'^_administration/email/bounty_feedback$', retail.emails.bounty_feedback, name='bounty_feedback'),
url(r'^_administration/email/start_work_expire_warning$', retail.emails.start_work_expire_warning, name='start_work_expire_warning'),
url(r'^_administration/email/start_work_expired$', retail.emails.start_work_expired, name='start_work_expired'),
url(r'^_administration/email/gdpr_reconsent$', retail.emails.gdpr_reconsent, name='gdpr_reconsent'),
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll eventually need to update every url() to the new methods: re_path() and path() - It'll be easier if we use those for new urls being added. Can we update this to re_path() ?

@@ -0,0 +1,56 @@
'''
Copy link
Contributor

Choose a reason for hiding this comment

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

''' can have unexpected consequences in py files. I've been trying to move to """ without the additional indent to pass pep-257 checks so we can ultimately auto-enforce this.

@ghost ghost assigned mbeacom May 24, 2018
Copy link
Contributor

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

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

lgtm

@mbeacom mbeacom merged commit fd726a7 into master May 24, 2018
@ghost ghost removed the in progress label May 24, 2018
@mbeacom mbeacom deleted the GDPR-notice branch May 24, 2018 22:07
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