Skip to content

Use Mailgun as default implementation for reset password and email verification. #250

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

Closed
wants to merge 8 commits into from

Conversation

taylorstine
Copy link
Contributor

This PR merges the logic from #187 and #191 to implement a reset password and verify email process similar to the original Parse implementation.

You can either pass in Mailgun configurations like this to get behavior similar to the default parse service: https://gist.github.com/taylorstine/8ac855dc1b6f9ef7eca4

Or write a custom MailAdapter to use a service other than Mailgun, like this: https://gist.github.com/taylorstine/d446491b76b1c8af2e30

@jamiechapman @gfosco

.four_oh_four #emoji {
position: relative;
top: 40px;
background-image: url('https://www.parse.com/images/404/sprite.png');

Choose a reason for hiding this comment

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

We may want to host the image within parse-served itself maybe? Chances are in a years time this image URL probably won't work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 sounds good

@jamiechapman
Copy link

@taylorstine 👍 Fantastic work! Really like how this has shaped up.

var database = DatabaseAdapter.getDatabaseConnection(appId);

return function (req, res) {
var mount = req.protocol + '://' + req.get('host') + req.baseUrl;

Choose a reason for hiding this comment

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

Check out the recently merged #132 PR for mount URL.

@facebook-github-bot
Copy link

@taylorstine updated the pull request.

@taylorstine
Copy link
Contributor Author

Updated based on @maysale01 suggestion.

Now you can must sent the verifyEmails property to true in the server constructor.

Also I changed the name of MailAdapter to MailAdapterStore and added MailAdapter as a prototype superclass for MailgunAdapter, since in my mind this makes more sense. The MailAdapter is actually implementing the logic of sending the mail, while the MailAdapterStore maintains a reference to the MailAdapter. I know this goes against the grain of the implementation of DatabaseAdapter and ExportAdapter, so if necessary I can change it.

With this change, a client can extend the MailgunAdapter class so they can just change the email sent, rather than having to extend the MailAdapter class and being forced to implement send email.

So you can extend MailAdapter to use your own service like this

Or you can extend MailgunAdapter so that you can send a custom email like this

Or you can do this, which will result in an error, because verifyEmails is set to true, but no mail configuration is set.

@jamiechapman

@taylorstine
Copy link
Contributor Author

Also removed parse.com images links

@facebook-github-bot
Copy link

@taylorstine updated the pull request.

@gnz00
Copy link

gnz00 commented Feb 5, 2016

@taylorstine Right on, +1.

In regards to the MailAdapterStore, I think that's the correct approach. The database and files adapters need to be abstracted.

I'm in favor of the following:

let server = new ParseServer({ ... });

server.get('mailProvider').getAdapter();
server.get('databaseProvider').getAdapter();
server.get('filesProvider').getAdapter();
server.get('cacheProvider').getAdapter();

Where Provider serves the purpose of your MailAdapterStore (pedantry I know).

@gnz00
Copy link

gnz00 commented Feb 5, 2016

// interfaces/ServiceProvider.js
class ServiceProvider {
    setAdapter() {
        throw new Error('A provider must implement setAdapter');
    }

    getAdapter() {
        throw new Error('A provider must implement getAdapter');
    }
}
// classes/BaseServiceProvider.js
class BaseServiceProvider extends ServiceProviderInterace {
    constructor(adapter) {
        if (adapter instanceof BaseServiceAdapter){
            this._adapter = adapter;
        } else {
            throw new Error('Adapter must be a subclass of BaseServiceAdapter');
        }
    }

    setAdapter(adapter) {
        this._adapter = adapter;
    }

    getAdapter() {
        return this._adapter;
    }
}
// classes/ParseServer.js
import { default as DEFAULT_MAIL_ADAPTER } from 'adapters/MailgunAdapter';
import { default as DEFAULT_DATABASE_ADAPTER } from 'adapters/ExportAdapter'
import { default as DEFAULT_FILES_ADAPTER } from 'adapters/GridStoreAdapter';
import { default as DEFAULT_CACHE_ADAPTER } from 'adapters/MemoryCacheAdapter'; 

class ParseServer {
    constructor(args) {
        this._mailProvider = new BaseServiceProvider(args.MailAdapter || DEFAULT_MAIL_ADAPTER);
        this._databaseProvider = new BaseServiceProvider(args.DatabaseAdapter || DEFAULT_DATABASE_ADAPTER);
        this._filesProvider = new BaseServiceProvider(args.FilesAdapter || DEFAULT_FILES_ADAPTER);
        this._cacheProvider = new BaseServiceProvider(args.CacheAdapter || DEFAULT_CACHE_ADAPTER);
    }

    get mailProvider() {
        return this._mailProvider;
    }

    // And so forth
    ...
}

}
this.data.emailVerified = false;
this.data.perishableToken = rack();
this.data.emailVerifyToken = rack();
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you add those in the default _User schema? to prevent accidental overrides.

@drew-gross
Copy link
Contributor

Lets consolidate discussion in #275

@taylorstine
Copy link
Contributor Author

So, should we close this, or are we still considering merging it?

@drew-gross
Copy link
Contributor

It seems like we are pretty close to coming to a conclusion on what the (mail)adapter interface should look like. I think we can wait for that discussion come to a conclusion before closing anything.

@gfosco gfosco mentioned this pull request Feb 20, 2016
@taylorstine taylorstine mentioned this pull request Feb 24, 2016
@gfosco
Copy link
Contributor

gfosco commented Feb 26, 2016

Closing this, looks like the eventual solution will come from issue 275.

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.

7 participants