-
Notifications
You must be signed in to change notification settings - Fork 82
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
New MessageSubmission object for sending mail #99
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to overall like this proposal, which is more explicit.
spec/mail/messagedelivery.mdown
Outdated
- **messageId**: `String` | ||
The id of the message to send. This is immutable. | ||
- **threadId**: `String` | ||
The thread id of the message to send. This is immutable and set by the server based on the message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we detail what based on the message means?
spec/mail/messagedelivery.mdown
Outdated
|
||
If the *envelope* property is `null` or omitted on creation, the server MUST generate this from the referenced message as follows: | ||
|
||
- **mailfrom**: The email in the *From* header, if present. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Case : mailFrom
spec/mail/messagedelivery.mdown
Outdated
If the *envelope* property is `null` or omitted on creation, the server MUST generate this from the referenced message as follows: | ||
|
||
- **mailfrom**: The email in the *From* header, if present. | ||
- **rcptto**: The deduplicated list of emails from the *To*, *Cc* and *Bcc* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idem, there is a case issue
spec/mail/messagedelivery.mdown
Outdated
|
||
- **sendAt**: `Date|Duration|null` | ||
The date to release the message for delivery. This is immutable. The server MUST override this property to the current date-time if the value given by the client is omitted, `null`, or in the past, or if the server does not support delayed sending. If a duration is specified, the server MUST set this property to the time of this duration from "now". | ||
- **maybeCanUnsend**: `Boolean` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the naming awkward... I would prefer isCancellable or something like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trouble is it's only "maybe" cancellable; we can never guarantee it's going to be possible to unsound the message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO then sentMaybeCancellable better reflects this. It is the canUnsend that looks strange to me.
spec/mail/messagedelivery.mdown
Outdated
- **maybeCanUnsend**: `Boolean` | ||
If `true`, attempting to unsend the message by destroying the MessageDelivery object, *might* succeed. This is server set. On systems that do not support unsending, this will always be `false`. On systems that do this, it will start as `true`, and MAY transition to `false` when the server knows it definitely cannot recall the message, but MAY just remain `true`. | ||
|
||
For efficiency, a server MAY destroy MessageDelivery objects a certain amount of time after the message is successfully sent. For very basic SMTP proxies, this MAY be immediately after creation, as it has no way to assign a real id and return the information again if fetched later. For more advanced systems, this might be a week or two later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case we need a isSent method or something similar, no?
spec/mail/messagedelivery.mdown
Outdated
|
||
If the creation succeeds, the message will be sent to the recipients given in the envelope *rcptTo* parameter. The server MUST remove any *Bcc* header present on the message during delivery. The server MAY add or remove other headers from the submitted message during delivery. | ||
|
||
The message being sent does not have to be a draft, for example when "redirecting" an existing message to a different email address. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be stated above.
spec/mail/messagedelivery.mdown
Outdated
|
||
The message being sent does not have to be a draft, for example when "redirecting" an existing message to a different email address. | ||
|
||
Once the MessageDelivery is created, the referenced message MAY be deleted in the mailstore. This MUST NOT change the behaviour of the message delivery (i.e. it does not cancel a future send). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need it to be an explicit property of MessageDelivery...
spec/mail/messagedelivery.mdown
Outdated
The id of the message to send. This is immutable. | ||
- **threadId**: `String` | ||
The thread id of the message to send. This is immutable and set by the server based on the message. | ||
- **envelope**: `Envelope|null` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not find where you define the envelope object...
spec/mail/messagedeliverylist.mdown
Outdated
|
||
## getMessageDeliveriesList | ||
|
||
Standard JMAP getList method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not very clear to a newcomer...
spec/mail/messagedelivery.mdown
Outdated
After the *messagesDeliveriesSet* response is emitted, if the *update* object is not empty, an implicit call is made to *setMessages*, with the following arguments: | ||
|
||
- **accountId**: Same as for the call to *setMessageDeliveries*. | ||
- **update**: The *update* object as calculated above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do not define what an update is. In my opinion, the knowledge of the required update should be explicitly embedded to MessageDelivery.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The update object is defined in the paragraph above:
For each successful create/destroy, the server should see if there is an entry with the same id in the onCreateUpdateMessage/onDestroyUpdateMessage properties respectively. If so, add an entry to an (initially empty) update object, with the key being the messageId on the MessageDelivery object that was created/destroyed, and the value being the value from the onCreateUpdateMessage/onDestroyUpdateMessage object.
I've tried to clarify the wording a bit. Hopefully some of the more gnarly bits are clearer now. |
spec/mail/messagedelivery.mdown
Outdated
This is how a message may be unsent. | ||
|
||
- **onCreateUpdateMessage**: `String[Message]` | ||
A map of *creation id* to an object containing properties to update on the Message object referenced by the MessageDelivery if the create succeeds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tell me if I'm wrong: Here the Message only needs to be a subset of Message Mutable properties, hence Keywords, right? **Keywords is the only mutable unit of a message, right? Then why do we need to pass more when a bag of keyword is enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MailboxIds are also mutable. IMO we should explain here only mutable property should be changed, and we should list them.
spec/mail/messagedelivery.mdown
Outdated
- **onCreateUpdateMessage**: `String[Message]` | ||
A map of *creation id* to an object containing properties to update on the Message object referenced by the MessageDelivery if the create succeeds. | ||
- **onDestroyUpdateMessage**: `String[Message]` | ||
A map of *MessageDelivery id* to an object containing properties to update on the Message object referenced by the MessageDelivery if the destroy succeeds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating a message is cool.
We should be also able to delete them. IE: delete draft upon send. Then maybe we need a: onDeleteDeleteMessage String
As stated on the ML, the envelope needs to specify (as optional) the identity to be used by the sender (http://jmap.io/spec-mail.html#identities) |
3d76751
to
7089f3f
Compare
spec/mail/message.mdown
Outdated
- **mailboxIds**: This property MUST be included. The value MUST include the id of either the mailbox with `role == "drafts"` (to save a draft) or the mailbox with `role == "outbox"` (to send the message). If this mailbox does not have `mustBeOnlyMailbox == true`, others may be included too. | ||
- **keywords**: This property MUST be included. It MUST include the `$Draft` keyword and SHOULD also include `$Seen`. | ||
- **mailboxIds**: This property MUST be included, and contain at least one mailbox. | ||
- **keywords**: This property MUST be included. If the message is a draft, it SHOULD include the `$Draft` and `$Seen` keywords. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"If the message is a draft..." is a bit redundant given that the section name is "Saving a draft".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. The title of the section should really be just Creating a message, but should reference saving a draft the primary example.
spec/mail/message.mdown
Outdated
@@ -276,12 +273,12 @@ The properties of the Message object submitted for creation MUST conform to the | |||
- **bcc**: Optional. Overrides a "Bcc" in the *headers*. | |||
- **replyTo**: Optional. Overrides a "Reply-To" in the *headers*. | |||
- **subject**: Optional. Defaults to the empty string (`""`). | |||
- **date**: Optional. If included, the server SHOULD wait until this time to send the message (once moved to the outbox mailbox). Until it is sent, the send may be cancelled by moving the message back out of the outbox mailbox. If the date is in the past, the message must be sent immediately. A client may find out if the server supports delayed sending by querying the server's *capabilities* object (see section 1). | |||
- **date**: Optional. Overrides a "Date" in the *headers*. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- headers's keys are supposed to be lowercased, so this should probably say "date".
- What does "overrides" mean here? I'm reading it as (assuming that the server stores mails in RFC5322 format) the server is supposed to replace the "date" header's value with whatever is supplied here (after converting it from RFC 3339 to RFC 5322 format) before storing it on disk. Is the intention here to be able to supply the original headers, but fix up the date without having to parse 5322-style dates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Yep, I'll lower case these.
- The date/to/cc/bcc/from/replyTo fields are all parsed representations that could also be represented in raw form in the headers property. This is simply specifying what the server should do if the client supplies both for some reason: Take the parsed one and blat the one in the "headers" property. I'll rewrite this description to be clearer.
spec/mail/messagesubmission.mdown
Outdated
|
||
This is how a message is sent. | ||
|
||
- **update**: `String[MessageSubmission]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should update also allow null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, will fix.
spec/mail/messagesubmission.mdown
Outdated
|
||
### Destroying a MessageSubmission object | ||
|
||
Destroying a MessageSubmission object MUST NOT affect the delivies it represents. It purely removes the record of the message submission. The JMAP server MAY reject attempts by a client to destroy a MessageSubmission with a `forbidden` error. The server MAY automatically destroy MessageSubmission objects after a certain time or in response to other triggers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sanity check: So, a valid implementation of message submission is:
- setMessageSubmissions takes the content, and (if valid) synchronously hands off the mail to the MTA
- getMessageSubmissions always returns notFound
- getMessageSubmissionUpdates always returns cannotCalculateChanges
- getMessageSubmissionLists always returns an empty array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quality client will need to deal with responses 2-4 regardless of what the spec says so it's not a big ask of client authors to allow this. The tricky part with standard design is to balance deployability with mandatory functionality and predictability. I believe we want to allow this server behavior to simplify deployability. In addition, if JMAP is playing the role of mail submission API in a larger cloud server, this behavior is just fine. However, this server behavior will result in an inferior experience for a client that supports status/recall of sent messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeffpc: Yes, this is a valid (very basic) implementation. As cjnewman said, clients have to be able to deal with these responses anyway.
spec/mail/messagesubmission.mdown
Outdated
- **email**: `String` | ||
The email address being represented by the object. This as a "Mailbox" as used in the Reverse-path or Foward-path of the MAIL FROM or RCPT TO command in [@!RFC5321 | ||
- **parameters**: `Object|null` | ||
Any parameters to send with the email (either mail-parameter or rcpt-parameter as appropriate, as specified in [@!RFC5321]). If supplied, each key in the object is a parameter name, and the value the parameter value. In both cases, any xtext or unitext encodings are removed ([@!RFC3461], [@!RFC6533]) and JSON string encoding applied. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RFC 5321, section 4.1.2 defines a SMTP parameter as
esmtp-param = esmtp-keyword ["=" esmtp-value]
I guess a SMTP parameter without value is encoded in JMAP as an object key with value null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I'll add this explicitly to the spec.
No description provided.