DevProgress / HillaryBNB

Home sharing app for Hillary Clinton supporters
Other
10 stars 13 forks source link

Unsubscribe from email #76

Closed durka closed 8 years ago

durka commented 8 years ago

Closes #72.

I added an unsubscribe link to the two daily emails. This goes to a new view with a warning (that your account will be deleted) and a button that says "Click here to unsubscribe". If you click it, your account is deleted (in the same way as the regular "Delete account" button).

It looks like this:

Unsubscribe view

and then clicking the button gets you back to:

After unsubscription

I do not know how to set up tests so I have not done so.

durka commented 8 years ago

Right now you can unsubscribe without logging in. This is achieved by reusing the email confirmation token, and putting that token in the email unsubscribe links instead of the user ID. I was discussing with @geraldhuff on Slack that this is better than using the ID, since it can't be guessed, but to be paranoid it can be... obtained, if someone is hacking emails (the Russians, obviously). So, more secure, but maybe unnecessary, would be to require you to log in before unsubscribing. Thoughts?

durka commented 8 years ago

Test added. It doesn't test that the link in the email is correct (which I did test by hand), just the flow after you have that URL.

dsjoerg commented 8 years ago

@durka I think unsubscribing without having to log in is better for us, net net. I'm less worried about getting hacked by people stealing emails than I am of our mails being spam-flagged by annoyed users.

durka commented 8 years ago

I agree, it just warrants caution because it does directly delete rows from the DB.

dsjoerg commented 8 years ago

Sorry for the mess, I started making minor language adjustments but that broke the test and I can't push to your branch so I did this: https://github.com/DevProgress/HillaryBNB/pull/80

dsjoerg commented 8 years ago

Will try this out in my dev branch and then merge in ~2 hrs

durka commented 8 years ago

Great, closing in favor of #80