GH-27: Master based email #105

mattleibow · 2018-03-22T17:46:45Z

Description of Change

Describe your changes here.

Bugs Fixed

Related to issue Email API's #27

Provide links to issues here. Ensure that a GitHub issue was created for your feature or bug fix before sending PR.

API Changes

public static partial class Email {
    public static Task ComposeAsync();
    public static Task ComposeAsync(string subject, string body, params string[] to);
    public static Task ComposeAsync(EmailMessage message);
}

public class EmailMessage {
    public EmailMessage();
    public EmailMessage(string subject, string body, params string[] to);

    public string Subject { get; set; }
    public string Body { get; set; }
    public EmailBodyFormat BodyFormat { get; set; } // this is only supported on iOS/Android
    public List<string> To { get; set; }
    public List<string> Cc { get; set; }
    public List<string> Bcc { get; set; }
}

public enum EmailBodyFormat {
    PlainText,
    Html
}

Behavioral Changes

Adding the email API.

PR Checklist

Has tests (if omitted, state reason in description)
Has samples (if omitted, state reason in description)
Rebased on top of master at time of PR
Changes adhere to coding standard
Updated documentation (see walkthrough)

UWP OK

# Conflicts: # Samples/Caboodle.Samples/ViewModel/HomeViewModel.cs

mattleibow · 2018-03-27T22:12:22Z

Caboodle/Caboodle.csproj

+    <None Remove="Email\Email.netstandard.cs" />
+    <None Remove="Email\Email.shared.cs" />
+    <None Remove="Email\EmailMessage.shared.cs" />
+  </ItemGroup>


This can be removed.

mattleibow · 2018-03-27T22:13:42Z

Caboodle/Email/Email.android.cs

+        public static bool IsSendSupported
+        {
+            get;
+        }


This technically has no explicit value (default to false). Also, this must be internal until we decide to expose it.

mattleibow · 2018-03-27T22:13:50Z

Caboodle/Email/Email.android.cs

+    public static partial class Email
+    {
+        /// Indicating whether Compose Dialog is available
+        public static bool IsComposeSupported


This must be internal until we decide to expose it.

mattleibow · 2018-03-27T22:14:20Z

Caboodle/Email/Email.android.cs

+            string body = null,
+            string bodymimetype = null,
+            string[] attachmentspaths = null
+            )


this must be on the previous line.

mattleibow · 2018-03-27T22:14:58Z

Caboodle/Email/Email.android.cs

+            // Numerous apps can respond to ActionSend
+            var i = new Intent(Intent.ActionSend, uri);
+            i.SetData(uri);
+            i.SetType("plain/text");    // i.SetType("message/rfc822");


This TODO looks like something is to be done? Or is it safe to go?

mattleibow · 2018-03-27T22:31:01Z

DeviceTests/Caboodle.DeviceTests.Shared/Email_Tests.cs

+        {
+            Email.ComposeAsync(
+                new string[] { "moljac@mailinator.com", "moljac01@mailinator.com" },
+                new string[] { "moljac02@mailinator.com" },


Maybe use Xamarin-y or example.org emails?

mattleibow · 2018-03-27T22:31:18Z

Samples/Caboodle.Samples/Content-Mail/caboodle-mail.md

@@ -0,0 +1 @@
+


What is this file for?

mattleibow · 2018-03-27T22:31:31Z

Samples/Caboodle.Samples/View/EmailPage.xaml

+             x:Class="Caboodle.Samples.View.EmailPage"
+             xmlns:viewmodels="clr-namespace:Caboodle.Samples.ViewModel"
+             Title="Email"
+             >


Put this on the same line

mattleibow · 2018-03-27T22:31:50Z

Samples/Caboodle.Samples/View/EmailPage.xaml.cs

+namespace Caboodle.Samples.View
+{
+	[XamlCompilation(XamlCompilationOptions.Compile)]
+	public partial class EmailPage : ContentPage


This is already in the app.xaml.cs

mattleibow · 2018-03-27T22:33:38Z

Samples/Caboodle.Samples/ViewModel/EmailViewModel.cs

+            // For testing mailboxes (w/o registration) go to:
+            // https://www.mailinator.com
+            tomails = string.Join(",", new string[] { "caboodle@mailinator.com", "caboodle01@mailinator.com" });
+            ccmails = string.Join(";", new string[] { "caboodle@mailinator.com", "caboodle02@mailinator.com" });


I don't think we should use third party tools - maybe there is a testing tool that just accepts everything - maybe an example.org type of thing?

jamesmontemagno · 2018-03-28T19:58:33Z

I think this API needs to be looked at. I am not sure if we should really have attachments in V1 at all, seems complicated.

80% use case is....
Email.Send(string subject, string body, string sendto) ?

Thoughts on this.. or maybe just have overloads?

James

Redth · 2018-03-28T21:51:27Z

I might be odd but most of my uses cases require attachments. I don't usually want to just send text in an email, I want to attach a file to export.

…ent app

mattleibow · 2018-03-28T22:33:41Z

@Redth I am just going to get the emails working again, and then I'll add attachments back.

mattleibow · 2018-03-28T23:52:42Z

The attachments logic may be harder than just a simple URI. Android will probably need an implementation of FileProvider and we do need to support sharing internal files or even streams.

We could get inspiration from here:

jamesmontemagno · 2018-03-30T22:49:37Z

Caboodle/Email/Email.uwp.cs

+
+        static void Sync(List<string> recipients, IList<EmailRecipient> nativeRecipients)
+        {
+            if (recipients != null)


can we just return if null? cleaner and anti-arrow :)

jamesmontemagno · 2018-03-30T22:50:15Z

Caboodle/PhoneDialer/PhoneDialer.netstandard.cs

@@ -2,8 +2,7 @@
 {
    public static partial class PhoneDialer
    {
-        internal static bool IsSupported =>
-            throw new NotImplementedInReferenceAssemblyException();
+        internal static bool IsSupported => false;


Should we be doing this in all?

I was just going out on a limb here and have the IsSupported bits NOT throw ever so that we can write nicer tests. But I don't really know if we will ever do this or how we should handle this idea.

jamesmontemagno · 2018-04-03T20:53:24Z

What's left on this? Docs?

mattleibow · 2018-04-03T21:37:03Z

Yes, let me add that if we are happy with the API.

moljac added 6 commits March 12, 2018 21:29

email API initial commit

31dd70e

UWP OK

Email Sample and Device Tests

d0a4404

Merge remote-tracking branch 'origin/master' into master_based_email

2951115

API according to comments

475095f

Merge remote-tracking branch 'origin/master' into master_based_email

c76d63e

# Conflicts: # Samples/Caboodle.Samples/ViewModel/HomeViewModel.cs

Android Composing finished

18cfc29

moljac added the DO-NOT-MERGE Don't merge it.... don't do it! label Mar 27, 2018

mattleibow commented Mar 27, 2018

View reviewed changes

moljac added the awaiting-review This PR needs to have a set of eyes on it label Mar 28, 2018

moljac requested a review from Redth March 28, 2018 08:13

mattleibow added this to New in Triage Mar 28, 2018

mattleibow removed this from New in Triage Mar 28, 2018

mattleibow added this to Backlog in v0.5.0-preview Mar 28, 2018

mattleibow moved this from Backlog to In Progress in v0.5.0-preview Mar 28, 2018

Merge branch 'master' into master_based_email

155f685

mattleibow added 3 commits March 29, 2018 00:23

Update the Email API

1a042f3

Update the tests - removed the emil tests as this will leave the curr…

15ad2f2

…ent app

Update the tests - removed the emil tests as this will leave the curr…

ed4c421

…ent app

Add base for email attachments

f30efd3

mattleibow added 2 commits March 29, 2018 02:26

Removing attachments as this is not as simple as one might expect...

1f39e3c

Merge branch 'master' into master_based_email

a5136c2

mattleibow requested review from jamesmontemagno and moljac March 29, 2018 00:36

Removing usless email tests

d0b2cd8

jamesmontemagno suggested changes Mar 30, 2018

View reviewed changes

Return fast

7b9e295

Merge branch 'master' into master_based_email

b916357

jamesmontemagno previously approved these changes Apr 2, 2018

View reviewed changes

Update Email.uwp.cs

29d7195

mattleibow dismissed jamesmontemagno’s stale review via 29d7195 April 2, 2018 17:01

Merge branch 'master' into master_based_email

0e506e0

mattleibow removed the DO-NOT-MERGE Don't merge it.... don't do it! label Apr 2, 2018

Merge branch 'master' into master_based_email

703a02e

mattleibow added 7 commits April 3, 2018 23:58

Added the empty email docs

6a35251

Warnings instead of messages for missing docs as this is serious

92ea843

This should be private

0311f1b

Updated existing docs to match their APIs

835b9b3

Added the email docs

6b3355f

Updated the samples description

ab009f2

Fix test email address

a5dd478

Redth approved these changes Apr 4, 2018

View reviewed changes

Merge branch 'master' into master_based_email

9240317

jamesmontemagno approved these changes Apr 4, 2018

View reviewed changes

Redth removed the awaiting-review This PR needs to have a set of eyes on it label Apr 4, 2018

Merge branch 'master' into master_based_email

7a37e8c

Redth merged commit 4a110c7 into master Apr 4, 2018

Redth deleted the master_based_email branch April 4, 2018 19:53

Redth moved this from In Progress to Done in v0.5.0-preview Apr 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-27: Master based email #105

GH-27: Master based email #105

mattleibow commented Mar 22, 2018 •

edited

mattleibow Mar 27, 2018

mattleibow Mar 27, 2018

mattleibow Mar 27, 2018

mattleibow Mar 27, 2018

mattleibow Mar 27, 2018

mattleibow Mar 27, 2018

mattleibow Mar 27, 2018

mattleibow Mar 27, 2018

mattleibow Mar 27, 2018

mattleibow Mar 27, 2018

jamesmontemagno commented Mar 28, 2018

Redth commented Mar 28, 2018

mattleibow commented Mar 28, 2018

mattleibow commented Mar 28, 2018

jamesmontemagno Mar 30, 2018

jamesmontemagno Mar 30, 2018

mattleibow Apr 2, 2018

jamesmontemagno commented Apr 3, 2018

mattleibow commented Apr 3, 2018 •

edited

GH-27: Master based email #105

GH-27: Master based email #105

Conversation

mattleibow commented Mar 22, 2018 • edited

Description of Change

Bugs Fixed

API Changes

Behavioral Changes

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesmontemagno commented Mar 28, 2018

Redth commented Mar 28, 2018

mattleibow commented Mar 28, 2018

mattleibow commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesmontemagno commented Apr 3, 2018

mattleibow commented Apr 3, 2018 • edited

mattleibow commented Mar 22, 2018 •

edited

mattleibow commented Apr 3, 2018 •

edited