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.

Sign up for GitHub

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

Jump to bottom

Add unsubscribe from marketing email functionality #149

Merged
merged 1 commit into from
Mar 23, 2020
Merged

Add unsubscribe from marketing email functionality #149

merged 1 commit into from
Mar 23, 2020

Conversation

Fahminajjar
Copy link
Contributor

Description

  • Create UnsubscribedUser model to save unsubscribed users
  • Create a POST API endpoint to unsubscribe user from marketing emails

@Fahminajjar Fahminajjar self-assigned this Feb 25, 2020
@Fahminajjar Fahminajjar force-pushed the gdpr/marketing-emails branch 2 times, most recently from 0092e38 to 27a7e92 Compare February 25, 2020 14:03
ghost
ghost suggested changes Mar 4, 2020
View reviewed changes
Copy link

@ghost ghost left a comment •
edited by ghost
Loading

Choose a reason for hiding this comment

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

Thanks Fahmi. I would like to ask a couple of changes before moving on with this PR:

  • There are failed tests (ask Ali or Shadi for an account on Jenkins if you don't have one already), please fix them as the hawthorn release runs and passes the unit and integration tests of the edX platform and we'd like to maintain such build status.
  • Please refrain from creating new models (and migrations) in edX-owned apps. We should only create migrations in Edraak-owned apps (edraak_).

Both of those are totally new, as of Hawthorn. I'll reach out to you in Slack for more info.

image

lms/djangoapps/email_marketing/migrations/0011_unsubscribeduser.py Outdated Show resolved Hide resolved
lms/djangoapps/email_marketing/views.py Outdated Show resolved Hide resolved
@Fahminajjar
Copy link
Contributor Author

@OmarEdraak Thank you for the comments.

@Fahminajjar Fahminajjar force-pushed the gdpr/marketing-emails branch 2 times, most recently from 94ffe17 to 102d89f Compare March 8, 2020 13:08
ghost
ghost approved these changes Mar 9, 2020
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Amazing! I really like what you have done here. Thank you @Fahminajjar!

I have a couple of minor notes, please feel free to address them. Anyway, I think this is good to go.

lms/djangoapps/edraak_marketing_email/helpers.py Outdated Show resolved Hide resolved
lms/djangoapps/edraak_marketing_email/helpers.py Outdated Show resolved Hide resolved
lms/djangoapps/edraak_marketing_email/models.py Outdated Show resolved Hide resolved
lms/djangoapps/edraak_marketing_email/models.py Outdated Show resolved Hide resolved
lms/djangoapps/edraak_marketing_email/tests/test_views.py Show resolved Hide resolved
lms/djangoapps/edraak_marketing_email/tests/test_views.py Show resolved Hide resolved
lms/djangoapps/edraak_marketing_email/tests/test_views.py Show resolved Hide resolved
lms/djangoapps/edraak_marketing_email/views.py Show resolved Hide resolved
ghost
ghost reviewed Mar 9, 2020
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

One more note.

cms/envs/common.py Show resolved Hide resolved
@ghost
Copy link

ghost commented Mar 10, 2020 •
edited by ghost
Loading

Thanks @Fahminajjar for addressing those changes! Please fix the tests and feel free to merge.

@ghost
Copy link

ghost commented Mar 12, 2020 •
edited by ghost
Loading

@Fahminajjar Please let us know if there's any blocker to get this PR merged?

@Fahminajjar
Copy link
Contributor Author

It's ready to be merged.

@Fahminajjar
Add unsubscribe from marketing email functionality
3a0044f 
Create UnsubscribedUser model to save unsubscribed users
Create a POST API endpoint to unsubscribe user from marketing emails
@Fahminajjar Fahminajjar changed the base branch from master to dev/gdpr March 12, 2020 09:37
@ghost
Copy link

ghost commented Mar 13, 2020

Thanks @Fahminajjar. Looks like some unrelated tests are failing. No need to do anything now, I've restarted the tests run and it'll probably just fix itself.

@Salomari1987 Salomari1987 merged commit 5b55668 into dev/gdpr Mar 23, 2020
@ghost ghost deleted the gdpr/marketing-emails branch March 24, 2020 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shadinaif shadinaif shadinaif left review comments

@Salomari1987 Salomari1987 Awaiting requested review from Salomari1987

Assignees

@Fahminajjar Fahminajjar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Fahminajjar @shadinaif @Salomari1987