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

Email: Send pledge confirmation with authentication token. #46

Merged
merged 2 commits into from Oct 25, 2019

Conversation

@iandunn
Copy link
Member

iandunn commented Oct 25, 2019

The resolves #34 and #10, since they're interdependent, and neither can be fully understood or tested in isolation. Once this is in place, though, other issues like #12 can be resolved individually.

The primary test case is:

  • Create a new pledge
  • The pledge should be a draft, and the address should be unconfirmed
  • Open the email and click the link, you should see a success message
  • The pledge should now be publish, and the address should be confirmed

The unit tests describe some other test cases if you want to do additional testing.

I tested the emails from my w.org sandbox and they worked fine. I checked them against https://spamcheck.postmarkapp.com/ and https://www.mail-tester.com/ and got perfect scores.

There's some minor todo notes in the code, but given the tight deadline I think this is at a good place to merge and move on to other critical issues. We can circle back later to finish the small stuff.

Fixes #34.
Fixes #10.

Fixes #34.
Fixes #10.
@iandunn iandunn added this to the Create a pledge milestone Oct 25, 2019
@iandunn iandunn requested review from ryelle and coreymckrill Oct 25, 2019
Copy link
Contributor

coreymckrill left a comment

Tested, works as advertised. 👍 Would pledge again.

Some minor comments/suggestions, but this feels pretty solid to me.

plugins/wporg-5ftf/includes/pledge-meta.php Outdated Show resolved Hide resolved
plugins/wporg-5ftf/includes/pledge.php Outdated Show resolved Hide resolved
$auth_token = array(
'value' => wp_generate_password( 20, false ), // Similar to `get_password_reset_key()`.
// todo should encrypt at rest? core doesn't but others do
'expiration' => time() + ( 2 * HOUR_IN_SECONDS ),
);
Comment on lines 60 to 64

This comment has been minimized.

Copy link
@coreymckrill

coreymckrill Oct 25, 2019

Contributor

It might be useful to have this part in a separate function, so that an auth token could be generated outside of the context of a URL. I could see us maybe needing that later on when we get to managing existing pledges and need to send people back to the front end edit pledge form after they have submitted changes.

This comment has been minimized.

Copy link
@coreymckrill

coreymckrill Oct 25, 2019

Contributor

Also so that an auth token could be used in the context of a contributor confirmation as well. Or would this function already work for that?

This comment has been minimized.

Copy link
@iandunn

iandunn Oct 25, 2019

Author Member

🤔

I think the existing setup will work for contributor emails, since they're basically the same flow.

That's a good point about managing pledges, though. My previous thoughts were to still use this, but maybe add context to it to control that kind of thing; related discussion: https://github.com/WordPress/five-for-the-future/pull/46/files#r339103839.

I played around w/ modularizing it -- which is normally something I've very much in favor of -- but got a bit nervous about human error and mishandling the token. We're also on a tight deadline, and don't know what our future needs will look like, so my vote would be for leaving it as-is, and then reconsidering refactoring it when we have a tangible use case.

This comment has been minimized.

Copy link
@coreymckrill

coreymckrill Oct 25, 2019

Contributor

👍

);
// todo include a "this lnk will expire in 10 hours and after its used once" message too?
// probably, but what's the best way to do that DRYly?

This comment has been minimized.

Copy link
@coreymckrill

coreymckrill Oct 25, 2019

Contributor

function get_link_expiration_message( $hours ) {} ?

This comment has been minimized.

Copy link
@iandunn

iandunn Oct 25, 2019

Author Member

🤔 I'm thinking it'd be nice to maybe have a wrapper for getting the link and getting the expiration message, because theoretically they should always be used together, but it'd be easy for a dev to forget that or not realize it when adding a new message, so it'd be nice to have some kind of structural way to emphasize it.

Not really sure, though.

// todo when used to manage pledge, token will probably get deleted when viewing, and then they won't be able to save
// fix that when create the manage process, though. for now this works for confirming email address.
// maye pass a `context` param to this function, either 'view' or 'update', and only delete if context is 'update' ?
// make sure view and update functions checks to make sure have valid token, not create though

This comment has been minimized.

Copy link
@coreymckrill

coreymckrill Oct 25, 2019

Contributor

Maybe another context would be confirming a linked contributor?

This comment has been minimized.

Copy link
@iandunn

iandunn Oct 25, 2019

Author Member

I think that'd be the same update context as confirming a pledge, because they seem like they have the same requirements (just being used one page load, not for a session/flow across several page loads).

Were you thinking about it a different way, though?

This comment has been minimized.

Copy link
@coreymckrill

coreymckrill Oct 25, 2019

Contributor

I think you're right, they are the same context. I was getting thrown off by $pledge_id

plugins/wporg-5ftf/includes/email.php Show resolved Hide resolved
@iandunn iandunn merged commit 5ffca94 into production Oct 25, 2019
iandunn added a commit that referenced this pull request Oct 25, 2019
/*
* This is only intended to run when the front end form and wp-admin forms are submitted, not when posts are
* programmatically updated.
*/
if ( 'Submit Pledge' !== $post_action && 'editpost' !== $get_action ) {
return;
}
Comment on lines +191 to +197

This comment has been minimized.

Copy link
@coreymckrill

coreymckrill Oct 25, 2019

Contributor

@iandunn For some reason, this is preventing post meta values from being updated when the values are changed in the metabox on the Edit Pledge screen in wp-admin.

This comment has been minimized.

Copy link
@iandunn

iandunn Oct 26, 2019

Author Member

Huh, I thought it worked there when I tested, but I'll check.

This comment has been minimized.

Copy link
@iandunn

iandunn Oct 26, 2019

Author Member

Yup, it was. Fixed in ff9767f

iandunn added a commit that referenced this pull request Oct 26, 2019
This bug was introduced in 5ffca94, and prevented pledges from being updated when submitting via wp-admin.

See #46 (comment)
@ryelle ryelle deleted the email-auth branch Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.