Skip to content

feat: allow setting sent date on APPEND #174

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

Merged
merged 6 commits into from
Dec 15, 2020

Conversation

mblarsen
Copy link
Contributor

@mblarsen mblarsen commented Dec 12, 2020

References issue #60

Followed syntax of (date-time token) http://www.faqs.org/rfcs/rfc3501.html

I followed the pattern of append calling append_with_flags calling append_with_flags_and_date.

  • existing tests passes
  • update fn docs

This change is Reviewable

@mblarsen
Copy link
Contributor Author

mblarsen commented Dec 12, 2020

I tested the implementation on a small tool to migrate a massive mbox file through imap on fastmail.com. Emails appear with expected times.

There's no test for append so I was lazy and didn't add to it.

So yeah, ready for review @jonhoo :)

@mblarsen mblarsen marked this pull request as ready for review December 12, 2020 12:23
@jonhoo
Copy link
Owner

jonhoo commented Dec 12, 2020

Excellent, thank you! I left one small inline comment, but that's the only thing. It would be very nice to have an append test, but I'm not going to require it :p

Changed DateTime<Local> to DateTime<FixedOffset>

Local takes the timezone from the system. Utc sets to +0000 we seem to
be wanting one that remains fixed so that when formatted it is a
specified.
@mblarsen mblarsen force-pushed the feature/append-date branch from f0e5bf3 to e380177 Compare December 13, 2020 05:06
The test is temporarily disabled since the other append test doesn't
seem to clean up properly, so the `.len()` test may either be 1 or 2.

Also the date isn't reflected in either Greenmail or in the returned
envelope.
@mblarsen
Copy link
Contributor Author

Excellent, thank you! I left one small inline comment, but that's the only thing. It would be very nice to have an append test, but I'm not going to require it :p

Oh, my mistake hadn't seen the integration tests. I've added a test for the _and_date variant now.

However, I had to disable it for now since the date returned from Greenmail is the time of the append not the date given. The date you specify with APPEND is the internal date as far as I know. I'm not sure how to best test this as the returned envelop would have the sent date not the internal date.

I guess I could send to emails first one without _and_date then one with that has a date in the past, then do a fetch of them. I would assume the latter would be the first if it works.

Any thoughts?

@mblarsen
Copy link
Contributor Author

mblarsen commented Dec 13, 2020

@jonhoo
Copy link
Owner

jonhoo commented Dec 15, 2020

That's interesting -- if it doesn't modify the actual message date, how then is append with a date useful in practice? Can you extract it from the other fields of the message that aren't in the envelope?

Also, I had another thought over the weekend: I think this append API is getting pretty unwieldy. Given that we're going to be doing a breaking release soon anyway, I suggest we just make append be a single function that takes impl Into<AppendOptions> where

struct AppendOptions<'a> {
    flags: Option<&'a [Flag<'a>]>,
    date: Option<DateTime<FixedOffset>>,
}

And then have a couple of nice Into implementations for AppendOption. What do you think?

@mblarsen
Copy link
Contributor Author

That's interesting -- if it doesn't modify the actual message date, how then is append with a date useful in practice? Can you extract it from the other fields of the message that aren't in the envelope?

Yes, that was it. Fetch has internal_date and with that in play the tests pass.

date updated to Into<Option<...>> as you suggested.


The AppendOption change makes sense to me. I can make that in a separate PR as it would break the current API as far as I can tell.

You also mentioned (somewhere) that you take the path of a builder. Would the you then create an AppendCmd that implements AppendOption so that user can use the builder or AppendOption directly?

session.append(
  mbox,
  e.message_to_string().unwrap(),
  AppendCmd::new()
     .flag("\Seen")
     .internal_date(&date)
);

Or did you think to have a more generic command builder that was fed to something like session.execute(..)?

@jonhoo
Copy link
Owner

jonhoo commented Dec 15, 2020

Ah, yes, AppendCmd looks like a great idea! And a separate PR makes sense :)

@jonhoo jonhoo merged commit ee56c8e into jonhoo:master Dec 15, 2020
@mblarsen mblarsen deleted the feature/append-date branch December 16, 2020 14:37
@mblarsen mblarsen mentioned this pull request Dec 16, 2020
2 tasks
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.

2 participants