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

Small e-mail fixes #1923

Merged
merged 2 commits into from
Apr 29, 2016
Merged

Small e-mail fixes #1923

merged 2 commits into from
Apr 29, 2016

Conversation

mitar
Copy link
Contributor

@mitar mitar commented Apr 29, 2016

Just some things I noticed while reading through the code.

Not tested.

@garply
Copy link
Collaborator

garply commented Apr 29, 2016

Hey admins! Can one of you let me know if it's safe to build this pull request? See https://alpha.sandstorm.io/grain/kB7bofATTL2jGuKSFwmqdA/GithubPRCommands for a list of commands.

@kentonv
Copy link
Member

kentonv commented Apr 29, 2016

Hmm, it's clear the code does not match the comments, but I wonder if it is the code or the comments that is wrong. Maybe we actually intended to require that the grain specify an address matching one of the two, without any default? @jparyani do you remember?

(The unrelated inReplyTo change is clearly a bug, of course.)

@mitar
Copy link
Contributor Author

mitar commented Apr 29, 2016

In documentation it says:

When you send e-mail from a Sandstorm app, we allow you to set the "From" header either to your app's random address or to your verified Sandstorm login address.

It does not "feel" like you have to set it, but you are allowed to set it?

@kentonv
Copy link
Member

kentonv commented Apr 29, 2016

Yeah that line is really ambiguous, but the documentation was meant to be descriptive, not prescriptive, so that doesn't help us figure out what was the original intended behavior here...

@mitar
Copy link
Contributor Author

mitar commented Apr 29, 2016

How does app figure out what is its valid outgoing address? For example, how can a Meteor app know that? Is this easy? Then we can require it to set the right from address.

@kentonv
Copy link
Member

kentonv commented Apr 29, 2016

Apps normally call HackSessionContext#getPublicId() and then form the address as publicId@hostname.

https://github.com/sandstorm-io/sandstorm/blob/master/src/sandstorm/hack-session.capnp#L35

@kentonv
Copy link
Member

kentonv commented Apr 29, 2016

Or to get the user's address, HackSessionContext#getUserAddress():

https://github.com/sandstorm-io/sandstorm/blob/master/src/sandstorm/hack-session.capnp#L63

Keep in mind HackSessionContext is a born-deprecated API.

@mitar
Copy link
Contributor Author

mitar commented Apr 29, 2016

Hm, and that is not available for Meteor apps, no? Is it even possible to send e-mails from Meteor apps?

@kentonv
Copy link
Member

kentonv commented Apr 29, 2016

You can use node-capnp to make Cap'n Proto requests from Meteor apps.

The documentation describes how to get at HackSessionContext: https://docs.sandstorm.io/en/latest/developing/email-from-apps/

const grainAddress = session._getAddress();
const userAddress = session._getUserAddress();

// First check if we're changing the from address, and if so, move it to reply-to
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also removed this comment. It is not true at all.

@mitar
Copy link
Contributor Author

mitar commented Apr 29, 2016

There could be a Meteor package for that. ;-)

@mitar mitar mentioned this pull request Apr 29, 2016
@kentonv kentonv merged commit 85beb23 into sandstorm-io:master Apr 29, 2016
@kentonv
Copy link
Member

kentonv commented Apr 29, 2016

Thanks!

@mitar mitar deleted the small-email-fixes branch April 30, 2016 02:45
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.

3 participants