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

records form submission records for GDPR purposes + mailchimp permi-delete #1279

Merged
merged 10 commits into from
May 29, 2018

Conversation

owocki
Copy link
Contributor

@owocki owocki commented May 25, 2018

Description

records form submission records for GDPR purposes
also does a permi-delete in mailchimp when users account is closed

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

settings/profiles

Testing

tested it

Refers/Fixes

Self, Emails

@ghost ghost assigned owocki May 25, 2018
@ghost ghost added the in progress label May 25, 2018
@owocki owocki requested review from mbeacom and SaptakS May 25, 2018 21:43
@owocki
Copy link
Contributor Author

owocki commented May 25, 2018

@mbeacom @SaptakS this is a starter PR to register the form submission in which we contain / modify consent from our users.

we could either

  1. merge this now and add the cookie modal records to another PR
  2. add in in the cookie modal records (from email with carl in consensys legal) to this PR

@codecov
Copy link

codecov bot commented May 25, 2018

Codecov Report

Merging #1279 into master will increase coverage by 0.01%.
The diff coverage is 26.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1279      +/-   ##
==========================================
+ Coverage   31.29%   31.31%   +0.01%     
==========================================
  Files         122      122              
  Lines        8521     8535      +14     
  Branches     1116     1114       -2     
==========================================
+ Hits         2667     2673       +6     
- Misses       5743     5751       +8     
  Partials      111      111
Impacted Files Coverage Δ
app/marketing/management/commands/sync_mail.py 0% <0%> (ø) ⬆️
app/marketing/models.py 66.66% <100%> (+0.22%) ⬆️
app/dashboard/models.py 59.84% <100%> (+0.04%) ⬆️
app/marketing/views.py 16.34% <20%> (+0.81%) ⬆️

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 4bfbbc7...7e7d376. Read the comment docs.

@SaptakS
Copy link
Contributor

SaptakS commented May 28, 2018

I will add the cookie consent to this PR if that's okay.

@owocki
Copy link
Contributor Author

owocki commented May 28, 2018

@SaptakS sounds great

@owocki owocki changed the title records form submission records for GDPR purposes records form submission records for GDPR purposes + mailchimp permi-delete May 28, 2018
@ghost ghost assigned mbeacom May 28, 2018
client.lists.members.delete(
list_id=settings.MAILCHIMP_LIST_ID,
subscriber_hash=subscriber_hash,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

When we start running automatic linting/styling checks, the indentation on these parentheses will be automatically moved up to the previous line or the indent will move inward by one with the trailing ,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. maybe i should download a sublime linter.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mbeacom do we follow a strict PEP8 or flake8?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mbeacom as mentioned in PEP-8, this might be still correct indentation I think:

The closing brace/bracket/parenthesis on multiline constructs may either line up under the first non-whitespace character of the last line of list

or it may be lined up under the first character of the line that starts the multiline construct, as in:

Copy link
Contributor

Choose a reason for hiding this comment

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

flake8 - It's technically legal, but most autocorrection utilities are going to adjust the indentation (yapf, autopep8, etc).

Copy link
Contributor

Choose a reason for hiding this comment

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

subscriber_hash=subscriber_hash,
)
except Exception as e:
print(e)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use a logger here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

list_id=settings.MAILCHIMP_LIST_ID,
subscriber_hash=subscriber_hash,
)
except Exception as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

Which exception(s) are we expecting here? IndexError and KeyError per line 432 and probably a MailChimp connection error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any exception. the point is that any exception that happens shouldn't cause the page to 500

@@ -153,6 +155,15 @@ def privacy_settings(request):
return TemplateResponse(request, 'settings/privacy.html', context)


def record_form_submission(request, obj, _type):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename _type to record_type or submission_type to avoid confusion as a private var?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@@ -50,6 +51,9 @@
from retail.helpers import get_ip


logger = logging.getLogger(__name__)

Choose a reason for hiding this comment

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

F821 undefined name 'logging'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just fixed

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 - I think we're still waiting on cookie changes here, though?

@SaptakS
Copy link
Contributor

SaptakS commented May 29, 2018

@mbeacom since we are thinking of migrating from cookielaw, I think I will handle that separately from this PR. So it's good to be merged.

@owocki owocki merged commit 5a28232 into master May 29, 2018
@ghost ghost removed the in progress label May 29, 2018
@mbeacom mbeacom deleted the kevin/form_submission_records branch May 30, 2018 14:10
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.

4 participants