WIP: User unsubscribe 286 #288
Conversation
groovecoder
force-pushed
the
user-unsubscribe-286
branch
from
81ad5fa to
ca55824
Compare
controllers/user.js
Outdated
|
|
||
|
|
||
| async function postUnsubscribe(req, res) { | ||
| const unSubscribedUser = await DB.removeSubscriber(req.body.token, req.body.emailHash); |
There was a problem hiding this comment.
nit: This should be unsubscribedUser or the method should be postUnSubscribe(). It feels like we're mixing styles here slightly.
db/DB.js
Outdated
| "verification_token": token, | ||
| "sha1": emailSha1, | ||
| }); | ||
| return res[0]; |
There was a problem hiding this comment.
I know we've discussed briefly in the past, but should this be res[0] or should we try appending a .first() to the above SELECT query.
public/js/monitor.js
Outdated
| @@ -193,6 +193,11 @@ const postData = (url, data = {}) => { | |||
| }; | |||
|
|
|||
| function handleFormSubmits(formEvent) { | |||
| console.log("handleFormSubmits, formEvent: ", formEvent); | |||
There was a problem hiding this comment.
nit: remove debug console.log()
views/layouts/email.hbs
Outdated
| @@ -27,5 +27,6 @@ | |||
| </tr> | |||
| </table> | |||
|
|
|||
| You got this email because you signed up at {{ AppConstants.SERVER_URL }}. <a href="{{ unsubscribeUrl }}" target="_blank">Unsubscribe</a> | |||
There was a problem hiding this comment.
not sure if we need the "noreferrer" stuff here, since I think this is coming from an email (but I can't imagine it hurts).
| <div class="unsubscribe-message-wrapper"> | ||
| <div class="unsubscribe-message"> | ||
| {{#if unsubscribed }} | ||
| <h4>You have unsubscribed.</h4> |
There was a problem hiding this comment.
May need somebody to verify wording here. Have I unsubscribed, or have I been unsubscribed?
There was a problem hiding this comment.
Yeah the email and page text and designs are all just WIP right now. This PR is just to get them into place so @lesleyjanenorton can make them better.
| <h4>You have unsubscribed.</h4> | ||
| {{ else }} | ||
| <h4>Are you sure you want to unsubscribe?</h4> | ||
| <form action="/user/unsubscribe" method="post" id="unsubscribe-form"> |
There was a problem hiding this comment.
May need clarification here on if we need to add the data-no-csrf novalidate flags here. I noticed we have them in:
There was a problem hiding this comment.
Yeah the email and page text and designs are all just WIP right now. This PR is just to get them into place so @lesleyjanenorton can make them better.
groovecoder
force-pushed
the
user-unsubscribe-286
branch
2 times, most recently
from
fdd9f6c to
f88d34b
Compare
|
All comments addresses except the couple that will be fixed with final email designs & HTML. |
|
Not sure if you want both commits in this PR, or if you want to remove first one to avoid merging in the refresh breaches code as well |
groovecoder
force-pushed
the
user-unsubscribe-286
branch
from
f88d34b to
4455f95
Compare
|
Removed the breaches endpoint commit. |
| const sha1 = getSha1(email); | ||
|
|
||
| return await this._getSha1EntryAndDo(sha1, async aEntry => { |
There was a problem hiding this comment.
I guess outside of the scope of this PR, but...
Why does it take 2 queries to delete a user? By proxying this._getSha1EntryAndDo(sha1), we call knex("subscribers").where("sha1", sha1), and then here we requery the same table by knex("subscribers").where("id", "=", aEntry.id).del()
There was a problem hiding this comment.
Yes, another refactor I'd like to perform is to remove the _getSha1EntryAndDo function completely.
This is a very ugly start but I'd like to get a review.