-
Notifications
You must be signed in to change notification settings - Fork 45
Conversation
Current Aviator status
This PR was merged using Aviator. See the real-time status of this PR on the Aviator webapp. Use the Aviator Chrome Extension to see the status of your PR within GitHub.
|
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.
... with a small translator function which converts
Postable
toMessageable
...
This would be better. I personally would have done this conversion as there are more gotchas in the eml conversion. We could always separate it out in case we find that Postable
is not a subset.
Looking at the interface, these seem to be the functions that are in Postable
but not in Messageable
:
GetConversationThreadId()(*string)
GetInReplyTo()(Postable)
GetNewParticipants()([]Recipientable)
I'll however leave that decision up to you. The PR looks good to me.
Thanks @meain . I'll punt this for now. Here's a tech debt ticket. #5155 I had one question about the message -> EML code here.
Any reason we are using sent date time here and not received date time or last modified date time? |
The spec mentions it to be the "sent" datetime. The idea is to have the datetime where it was "created". 3.6.1. The Origination Date Field The origination date field consists of the field name "Date" followed orig-date = "Date:" date-time CRLF The origination date specifies the date and time at which the creator |
Thanks Abin! I'll change it to |
b0a8b31
to
f4c14c1
Compare
ec729fb
to
7097373
Compare
00c9c6a
to
30160b4
Compare
Quality Gate failedFailed conditions 37.4% Duplication on New Code (required ≤ 3%) |
Postable
to EML converter code for group mailboxes and associated tests.Messageable
code at some point with a small translator function which convertsPostable
toMessageable
. Reason being that posts are a subset of messages. Although I feel it's a bit premature right now. Once we have tested this in prod a bit, and we know for a fact that posts are indeed a true subset of messages, we can do this convergence.Does this PR need a docs update or release note?
Type of change
Issue(s)
Test Plan