Creation and modification of private distribution list is not supported by ews.
http://msdn.microsoft.com/en-us/library/aa580529%28v=EXCHG.80%29.aspx

Thanks!

Created attachment 243667 [details] [review]
Bug #658892 - Support contact list creation

Created attachment 243668 [details] [review]
Bug #658892 - Support contact list creation

I'm getting this warnings[0] for every first contact added in the Distribution List. With or without my patch.

[0]:
(evolution:13321): libeutil-CRITICAL **: e_contact_store_get_contact: assertion `ITER_IS_VALID (contact_store, iter)' failed

(evolution:13321): libeutil-CRITICAL **: e_contact_store_get_contact: assertion `ITER_IS_VALID (contact_store, iter)' failed

(evolution:13321): libebook-contacts-CRITICAL **: e_contact_get: assertion `contact && E_IS_CONTACT (contact)' failed

(evolution:13321): libebook-contacts-CRITICAL **: e_contact_get: assertion `contact && E_IS_CONTACT (contact)' failed

(evolution:13321): libebook-contacts-CRITICAL **: e_contact_get: assertion `contact && E_IS_CONTACT (contact)' failed

(In reply to comment #4)
> I'm getting this warnings[0] for every first contact added in the Distribution
> List. With or without my patch.

Right, it "sometimes" happens when autocompleting the contacts. I didn't notice any side-effects of the warnings on console, though. In any case, those are unrelated to your changes.

Review of attachment 243668 [details] [review]:

I like the patch size, I knew it's trivial, it was only waiting for a proper exchange version :) Below are issues I noticed.

A distribution-list can contain other distribution lists. The problem is that you show "D-List <(null)>" currently for it. It would be good to traverse into deepness when parsing the response. I would not rely on the server with proper dependency circle detection. Also note that we can have IDs of contacts in the Contact List, thus it would be good to populate them as well, if they are available.
  <t:Mailbox>
    <t:Name>D-List</t:Name>
    <t:RoutingType>MAPIPDL</t:RoutingType>
    <t:MailboxType>PrivateDL</t:MailboxType>
    <t:ItemId Id="AAMkAGFhNW...AAA=" ChangeKey="EgAAAA=="/>
  </t:Mailbox>
  <t:Mailbox>
    <t:Name>no-note@no.where</t:Name>
    <t:EmailAddress>no-note@no.where</t:EmailAddress>
    <t:RoutingType>SMTP</t:RoutingType>
    <t:MailboxType>Contact</t:MailboxType>
    <t:ItemId Id="AAMkAGFhNW...AAA=" ChangeKey="EQAAAA=="/>
  </t:Mailbox>


If I enter address in evolution with email only, or with name only, then I cannot save the contact list and I get an error instead:
> Cannot modify contacts: The request failed schema validation:
> The 'http://schemas.microsoft.com/exchange/services/2006/type
> s:EmailAddress' element is invalid - The value '' is invalid
> according to its datatype 'http://schemas.microsoft.com/exch
> ange/services/2006/types:NonEmptyStringType' - The actual
> length is less than the MinLength value.
thus if one is missing, then you should populate it with the other (basically the opposite from what I suggest right below, when name and email are either missing or the same).

::: src/addressbook/e-book-backend-ews.c
@@ +1240,2 @@
 static void
+get_mailbox_info (const gchar *info, gchar **name, gchar **email)

coding style: param per line

@@ +1249,3 @@
+	 * [1] means Contact Email
+	 */
+	tokens = g_strsplit_set (info, "<>", -1);

nonono, it can break easily, rather use Camel for address parsing, it's much safer. See [1] for reference (note of all error checking; the 'raw' is 'info' in your function.

From the past experience, either name or email can be NULL, thus count with it, same as if the both equal, then you might not need to set both, but only email.

[1] https://git.gnome.org/browse/evolution-mapi/tree/src/libexchangemapi/e-mapi-book-utils.c#n540

@@ +1467,3 @@
+		if (e_ews_connection_get_server_version (ebews->priv->cnc) < E_EWS_EXCHANGE_2010) {
+			g_object_unref (contact);
+			e_data_book_respond_create_contacts (book, opid, EDB_ERROR (NOT_SUPPORTED), NULL);

It'll be good to explain why it's not supported. Also beware of UNKNOWN server version.

It feels like a new function:
   gboolean e_ews_connect_covers_server_version (cnc, minimum_version);
or e_ews_connect_satisfies_server_version (...)
or any better name for a test whether minimum version is enough for current server version, would be good to have.

@@ +1806,3 @@
+		if (e_ews_connection_get_server_version (ebews->priv->cnc) < E_EWS_EXCHANGE_2010) {
+			g_object_unref (contact);
+			e_data_book_respond_create_contacts (book, opid, EDB_ERROR (NOT_SUPPORTED), NULL);

similar as above, tell them why it's not supported

::: src/server/e-ews-connection.c
@@ +4264,3 @@
+	 * We need to ensure that we are not creating a request with an invalid server version.
+	 */
+	version = minimum_required > cnc->priv->version ? minimum_required : cnc->priv->version;

It seems like these checks should/could be done automatically for each e_ews_message_new_with_header(), what do you think? I saw on an MSDN page that a newer Exchange server version can return different XMLs, but I guess/hope they mean only to return richer XML, rather than completely different XMLs.

What I would expect is to pass a 'cnc' into e_ews_message_new_with_header() as the first argument, and use the given version as a minimum_version, and update version accordingly only inside e_ews_message_new_with_header(). And if I would want to be paranoid, then even add a new boolean parameter 'force_minimum_version' which will make sure to not update version based on the received.

(In reply to comment #6)
> Review of attachment 243668 [details] [review]:
> 
> I like the patch size, I knew it's trivial, it was only waiting for a proper
> exchange version :) Below are issues I noticed.
> 
> A distribution-list can contain other distribution lists. The problem is that
> you show "D-List <(null)>" currently for it. It would be good to traverse into
> deepness when parsing the response. I would not rely on the server with proper
> dependency circle detection.

Ouch! I didn't realize about this case (and neither the first person who wrote this code). In a quick look, the server doesn't treat circular dependencies, as you imagined.
About traverse into deepness when parsing the response ... don't know how good it could be. If we could refer an existing DL easily into another DL would be the best case, IMHO. I need to do some tests to understand correctly what we could do.

> Also note that we can have IDs of contacts in the
> Contact List, thus it would be good to populate them as well, if they are
> available.
>   <t:Mailbox>
>     <t:Name>D-List</t:Name>
>     <t:RoutingType>MAPIPDL</t:RoutingType>
>     <t:MailboxType>PrivateDL</t:MailboxType>
>     <t:ItemId Id="AAMkAGFhNW...AAA=" ChangeKey="EgAAAA=="/>
>   </t:Mailbox>
>   <t:Mailbox>
>     <t:Name>no-note@no.where</t:Name>
>     <t:EmailAddress>no-note@no.where</t:EmailAddress>
>     <t:RoutingType>SMTP</t:RoutingType>
>     <t:MailboxType>Contact</t:MailboxType>
>     <t:ItemId Id="AAMkAGFhNW...AAA=" ChangeKey="EQAAAA=="/>
>   </t:Mailbox>

We are populating all information we can, but we are not saving it into the VCard, and IIUC, we don't should save it into the VCard.


> 
> @@ +1249,3 @@
> +     * [1] means Contact Email
> +     */
> +    tokens = g_strsplit_set (info, "<>", -1);
> 
> nonono, it can break easily, rather use Camel for address parsing, it's much
> safer. See [1] for reference (note of all error checking; the 'raw' is 'info'
> in your function.
> 
> From the past experience, either name or email can be NULL, thus count with it,
> same as if the both equal, then you might not need to set both, but only email.
> 
> [1]
> https://git.gnome.org/browse/evolution-mapi/tree/src/libexchangemapi/e-mapi-book-utils.c#n540

So, I suppose it is already being mounted in a wrong way.
Thank you for the catch!

 
> ::: src/server/e-ews-connection.c
> @@ +4264,3 @@
> +     * We need to ensure that we are not creating a request with an invalid
> server version.
> +     */
> +    version = minimum_required > cnc->priv->version ? minimum_required :
> cnc->priv->version;
> 
> It seems like these checks should/could be done automatically for each
> e_ews_message_new_with_header(), what do you think? I saw on an MSDN page that
> a newer Exchange server version can return different XMLs, but I guess/hope
> they mean only to return richer XML, rather than completely different XMLs.
> 
> What I would expect is to pass a 'cnc' into e_ews_message_new_with_header() as
> the first argument, and use the given version as a minimum_version, and update
> version accordingly only inside e_ews_message_new_with_header(). And if I would
> want to be paranoid, then even add a new boolean parameter
> 'force_minimum_version' which will make sure to not update version based on the
> received.

I did a kind-of this change in the #699847's patch. I also comment there that I disagree with your approach to pass the connection to the e_ews_message_new_with_header(). If you don't mind I would prefer pass server_version, minimum_version and force_minimum_version to this function.

(In reply to comment #7)
> About traverse into deepness when parsing the response ... don't know how good
> it could be. If we could refer an existing DL easily into another DL would be
> the best case, IMHO. I need to do some tests to understand correctly what we
> could do.

Evolution doesn't support "linkage" in contact lists, it's only the contact list editor, which can show contacts in a tree, when the list contains another contact list as a member. Try to create two plain contact lists in Personal book, then create third and include couple plain contacts together with these two new contact lists, and save it as a vCard. See its content for a way how the relation ship is saved in the vCard (again, it's used only for showing in the contact list and nothing else).

Thinking of it, if you save a contact list, and it contains other distribution list, then you might want to check the server-side content against the one to-be-saved, and if it's the same, then save to the server as a distribution list member, not with addresses expanded. But it's only for a case you want to be super-great. If I recall correctly, evolution-mapi doesn't do that and simply expands groups on save.

Created attachment 244124 [details] [review]
Bug #658892 - Support contact list creation

Created attachment 244126 [details] [review]
Bug #658892 - Support contact list creation

Review of attachment 244126 [details] [review]:

I found couple memory management issues, please fix them. Also commit the cleanup patch first, thus it'll be easier to test this (because this patch depends on the cleanup work).

::: src/addressbook/e-book-backend-ews.c
@@ +1236,3 @@
+	e_soap_message_start_element (msg, "Members", NULL, NULL);
+
+	attributes = e_vcard_get_attributes (E_VCARD (contact));

change this to use e_contact_get (contact, E_CONTACT_EMAIL), which returns newly allocated GList of newly allocated strings. You get safest results  (attribute names in vCard are case insensitive, and this is a direct way of getting all configured emails from EContact). Check evolution's code on the way it's used, you can get both "strings only", or vCard attributes.

@@ +1246,3 @@
+			continue;
+
+		addr = camel_internet_address_new ();

the 'addr' is not freed

@@ +1249,3 @@
+		values = e_vcard_attribute_get_values (l->data);
+
+		if (camel_address_decode (CAMEL_ADDRESS (addr), values->data) > 0) {

once you have strings you can check for non-NULL too (you know, let's be paranoid, do not trust to anyone).

@@ +2596,3 @@
+ebews_traverse_dl (EBookBackendEws *ebews,
+		   EContact **contact,
+		   GSList *items,

this doesn't work, you should have it "GSList **items"

@@ +2606,1 @@
+		if (g_slist_find_custom (items, mb->item_id->id, g_str_equal))

better to use GHashTable, because it's quicker in searching.

@@ +2609,1 @@
+		items = g_slist_prepend (items, mb->item_id->id);

once the freeing of 'members' is implemented (see below), you should insert a copy of the item's ID, then you'll need a free of items' members as well. You can, alternatively, store the 'member' into a hash tale and use already allocated ID as a key, which will save reallocations of memory.

@@ +2613,3 @@
+			EWS_PRIORITY_MEDIUM,
+			mb,
+			&members,

who is freeing the 'members'?

@@ +2663,3 @@
+	for (l = members; l != NULL; l = l->next) {
+		ebews_traverse_dl (ebews, &contact, items, l->data, error);
+		if (*error != NULL) {

do not trust the caller that he/she passed in a GError instance. Do it like in the cleanup work, let the ebews_traverse_dl() return TRUE/FALSE for 'success'.

@@ +2672,2 @@
+exit:
+	g_slist_free (items);

I'm pretty sure the 'items' is always NULL here. (see above)

Created attachment 244189 [details] [review]
Bug #658892 - Support contact list creation

Review of attachment 244189 [details] [review]:

I just realized that there can be also public distribution lists, it's when I added a distribution list from a GAL. This one has set a SMTP address, while I believe it's better to not expand the list in this case, thus the message is routed to proper recipients by the server (there used to be an option in evolution-exchange whether to expand lists or not).
  <t:Mailbox>
     <t:Name>Testers</t:Name>
     <t:EmailAddress>testers@no.where</t:EmailAddress>
     <t:RoutingType>SMTP</t:RoutingType>
     <t:MailboxType>PublicDL</t:MailboxType>
  </t:Mailbox>
You show the above contact as a regular address, which is fine with me (it's caused by its RoutingType and unknown MailboxType, rather than being done intentionally by this patch). :)

Anyway, I'm fine with the change as is, for now, just fix the two things (technically three) below, and commit to master. We can open follow-up bugs for anything found with this.

::: src/addressbook/e-book-backend-ews.c
@@ +1442,3 @@
+				EDB_ERROR_EX (
+					NOT_SUPPORTED,
+					_("This operation is only supported on EWS Server 2010 or later")),

'This operation' is too vague, it can come form anywhere, any time, thus it's better to give more detailed information, like "Cannot save contact list, it's supported with Exchange 2010 server or later." Similar message at e_book_backend_ews_modify_contacts().

@@ +2726,3 @@
+	contact = ebews_get_dl_info (ebews, id, d_name, members, error);
+	if (contact == NULL)
+		return;

'members' is not freed with this return. The way it is done now is not optimal, the ebews_fetch_items() should not give a responsibility to free 'members' to functions it calls, better if the ebews_fetch_items() itself frees 'members'. (at line 2835)

Pushed on master (3.9.2+):
https://git.gnome.org/browse/evolution-ews/commit/?id=5de4de5a162e0621e0547dc1fb208b55dd4df252