Using Fedora 17, Evolution v3.4.3, evolution-ews v3.4.3-1.  I can connect to my company's Exchange server (email, contacts, calendar).  Everybody had a photo which Outlook displays just fine. Evolution doesn't display a single one.  No photos are shown in an email, no photos when looking at the GAL contact list directly.

Created attachment 241896 [details] [review]
Handle inlined attachments in  e_ews_connection_{create,get}_attachments

Created attachment 241897 [details] [review]
Bug #654950 - Contact doesn't fetch photo

Known issues:
- Update picture -- there is a comment in src/addressbook/e-book-backend-ews.c
                    Basically old file should be deleted
- Create a new contact -- need to find a way to set imagine in this case.
- Convert any format to jpg? -- jpg is the only supported file. We should, at least
                                use a filter in the filesector to only show supported
                                types
- Contact editor GUI sometimes freeze after set a contact picture, I cannot see
any response/request message to try to debug.

Review of attachment 241896 [details] [review]:

I didn't test the patch, but these are my immediate concerns on it:

::: src/server/e-ews-connection.c
@@ +5688,3 @@
+		case EWS_ATTACHMENT_INFO_TYPE_INLINED:
+		{
+			buffer = (gchar *) ews_attachment_info_get_inlined_data (info, &length);

this leads to error (*) [see below]

@@ +5693,3 @@
+		}
+		default:
+			return;

put here similar warning as in e_ews_attachment_info_free(), about unknown type

@@ +5708,1 @@
 	free (buffer);

(*) [from above] you free something what is not owned by the function

::: src/server/e-ews-item.c
@@ -33,3 @@
 #include <libsoup/soup-misc.h>
 #include "e-ews-item.h"
-#include "e-ews-connection.h"

nice catch :)

@@ +1569,1 @@
+		tmpfilename = (gchar *) content;

I see it was already there, but please fix the 'tmpfilename' definition, it's not required to be writable, then define it as const.

::: src/server/e-ews-item.h
@@ +115,3 @@
 } EwsAddress;
 
+struct inlined {

I'd say to not use 'inlined', but rather 'inline' (remove the ednign 'd') overall this patch in general. For this particular it'll be better to name it EEwsAttachmentInline, or some similar (with the fully qualified prefix EEwsAttachment).

@@ +117,3 @@
+struct inlined {
+	gchar *filename;
+	gchar *mime_type;

Can even the URI attachment have the both above strings? I suppose it can.

@@ +125,3 @@
+	EWS_ATTACHMENT_INFO_TYPE_INLINED,
+	EWS_ATTACHMENT_INFO_TYPE_URI
+} EwsAttachmentInfoType;

Please prefix with E, like
   E_EWS_ATTA....
and EEwsAtta....

@@ +130,3 @@
+	EwsAttachmentInfoType type;
+	union {
+		struct  inlined inlined;

you can define this structure inside here too, see how it's done here:
https://git.gnome.org/browse/evolution-data-server/tree/addressbook/libedata-book/e-data-book.c?h=gnome-3-6#n101

Review of attachment 241897 [details] [review]:

::: src/addressbook/e-book-backend-ews.c
@@ +379,3 @@
+	attachments_ids = e_ews_item_get_attachments_ids (item);
+	if (!attachments_ids)
+		return NULL;

You should PRIV_UNLOCK when you return early from the function (applies couple times below too).

@@ +399,3 @@
+	photo = e_contact_photo_new ();
+	photo->type = E_CONTACT_PHOTO_TYPE_INLINED;
+	e_contact_photo_set_inlined (photo, content, len);

ouch, now I see why you used 'inlined' in the previous patch.

@@ +414,3 @@
+	EContactPhoto *photo;
+
+	photo = ews_get_photo (ebews, item);

here should be used the cancellable, isn't it?

@@ +599,3 @@
+                 EContact *contact)
+{
+

todo :)

@@ +844,3 @@
+#if 0
+	/*
+	 * Still need to find a way to get the old photo's attachemnt id

what about inventing a new property on the EContact, saying X-EVOLUTION-EWS-PHOTO-ATTACH-ID on the EContact, or just re-get attachment list and do what is needed not based on the ews data, but based on the server data?

After all, the e_ews_connection_create_photo_attachment() should ideally read list of attachments and if there is any already added, then delete it, which effectively replaces the attachment.

@@ +868,3 @@
+		ews_attachment_info_set_filename (info, "ContactPicture.jpg");
+
+		files = g_slist_append (files, info);

who is freeing all the above allocated memory?

::: src/server/e-ews-connection.c
@@ +6000,1 @@
 			info = e_ews_item_dump_mime_content (item, async_data->directory);

Can the info-s be leaked, when you add them into ->items, but not free them with ews_attachment_info_free()? (it belongs more to the first patch, but I realized it only now)

@@ +6252,3 @@
+}
+
+

two empty lines

::: src/server/e-ews-item.c
@@ +1503,3 @@
 
+void
+e_ews_item_set_contact_photo (EEwsItem *item, GSList *contact_photo)

GSList on a new line

@@ +1585,3 @@
+		if (g_ascii_strcasecmp (param_name, "IsContactPhoto") == 0) {
+			value = e_soap_parameter_get_string_value (subparam);
+			is_contact_photo = !(g_ascii_strcasecmp (value, "true") == 0);

this is XML, you can g_strcmp0() here (which is safer for NULL too)

(In reply to comment #4)

> 
> +struct inlined {
> 
> I'd say to not use 'inlined', but rather 'inline' (remove the ednign 'd')
> overall this patch in general. For this particular it'll be better to name it
> EEwsAttachmentInline, or some similar (with the fully qualified prefix
> EEwsAttachment).

As discussed on IRC, we cannot use the "inline" word. :-)

> 
> @@ +117,3 @@
> +struct inlined {
> +    gchar *filename;
> +    gchar *mime_type;
> 
> Can even the URI attachment have the both above strings? I suppose it can.

It could. But does it makes sense?
When you are working with an inline content you need to know the filename/mime-type at the moment you will drop/save the content in somewhere. When you are working with a URI you already have the whole filename.

> 
> @@ +125,3 @@
> +    EWS_ATTACHMENT_INFO_TYPE_INLINED,
> +    EWS_ATTACHMENT_INFO_TYPE_URI
> +} EwsAttachmentInfoType;
> 
> Please prefix with E, like
>    E_EWS_ATTA....
> and EEwsAtta....

I'm a but confused about when I could use EwsSomething or EEwsSomething.

> 
> @@ +130,3 @@
> +    EwsAttachmentInfoType type;
> +    union {
> +        struct  inlined inlined;
> 
> you can define this structure inside here too, see how it's done here:
> https://git.gnome.org/browse/evolution-data-server/tree/addressbook/libedata-book/e-data-book.c?h=gnome-3-6#n101

Oh, thanks! :-)

(In reply to comment #5)

 
> @@ +844,3 @@
> +#if 0
> +    /*
> +     * Still need to find a way to get the old photo's attachemnt id
> 
> what about inventing a new property on the EContact, saying
> X-EVOLUTION-EWS-PHOTO-ATTACH-ID on the EContact, or just re-get attachment list
> and do what is needed not based on the ews data, but based on the server data?
> 
> After all, the e_ews_connection_create_photo_attachment() should ideally read
> list of attachments and if there is any already added, then delete it, which
> effectively replaces the attachment.

Oh. I agree with you, Milan.
The main problem is find a way to have the EwsItem to get this list from server.
It's not "for free". Let me see how I can workaround on it. 

> 
> @@ +868,3 @@
> +        ews_attachment_info_set_filename (info, "ContactPicture.jpg");
> +
> +        files = g_slist_append (files, info);
> 
> who is freeing all the above allocated memory?

Oops.

> 
> ::: src/server/e-ews-connection.c
> @@ +6000,1 @@
>              info = e_ews_item_dump_mime_content (item, async_data->directory);
> 
> Can the info-s be leaked, when you add them into ->items, but not free them
> with ews_attachment_info_free()? (it belongs more to the first patch, but I
> realized it only now)

Yes. The content is completely "leakable" (I'm pretty sure it is not a word).
Unlike what I did above, developer must care to free the content.

> 
> @@ +1585,3 @@
> +        if (g_ascii_strcasecmp (param_name, "IsContactPhoto") == 0) {
> +            value = e_soap_parameter_get_string_value (subparam);
> +            is_contact_photo = !(g_ascii_strcasecmp (value, "true") == 0);
> 
> this is XML, you can g_strcmp0() here (which is safer for NULL too)

Ouch! Why is g_ascii_strcasecmp used everywhere?

.
> - Convert any format to jpg? -- jpg is the only supported file. We should, at
> least
>                                 use a filter in the filesector to only show
> supported
>                                 types

It's done for free using GdkPixbuf. And I also add a patch in Evolution, so the image selector filechooser is only filtering by mime-type image/*

(In reply to comment #7)
> (In reply to comment #5)
> 
> 
> > @@ +844,3 @@
> > +#if 0
> > +    /*
> > +     * Still need to find a way to get the old photo's attachemnt id
> > 
> > what about inventing a new property on the EContact, saying
> > X-EVOLUTION-EWS-PHOTO-ATTACH-ID on the EContact, or just re-get attachment list
> > and do what is needed not based on the ews data, but based on the server data?
> > 
> > After all, the e_ews_connection_create_photo_attachment() should ideally read
> > list of attachments and if there is any already added, then delete it, which
> > effectively replaces the attachment.
> 
> Oh. I agree with you, Milan.
> The main problem is find a way to have the EwsItem to get this list from
> server.
> It's not "for free". Let me see how I can workaround on it. 

I found a (quick) solution for this based in EWS data.
As soon I update the patch we can discuss if it is or isn't a valid solution.

(In reply to comment #6)
> As discussed on IRC, we cannot use the "inline" word. :-)

True, I complete forgot of it being a keyword in C. I'm sorry for that.

> It could. But does it makes sense?
> When you are working with an inline content you need to know the
> filename/mime-type at the moment you will drop/save the content in somewhere.
> When you are working with a URI you already have the whole filename.

Hmm, OK, if you'll keep the original file name, then it's no problem.

> I'm a but confused about when I could use EwsSomething or EEwsSomething.

Right, the evo-ews sources are quite inconsistent in this. A "little" cleanup would be nice there.

(In reply to comment #7)
> Ouch! Why is g_ascii_strcasecmp used everywhere?

Good question. Either g_strcmp0() was not in GLib in time of the initial work on evo-ews, or the founders preferred g_ascii_strcasecmp. Who knows. I prefer g_strcmp0() for its NULL checking, and as long as we are checking XML data, we can be sure the values are case sensitive. Actually, definitely element and attribute names are case sensitive, whether also values of respective (builtin) types I'm not sure. The functionality safer is g_ascii_strcasecmp(), the "crash" safer is g_strcmp0(). Feel free to skip this point, I guess I should not review on Friday evening, I'm too much in a nit-pick/weekend-mode.

Created attachment 242239 [details] [review]
Handle inlined attachments in  e_ews_connection_{create,get}_attachments

Created attachment 242240 [details] [review]
Bug #654950 - Contact doesn't fetch photo

This operation is only supported for Exchange Server newer than 2010_SP2.
Ideally we might could populate which item can be changed or not
according with the server version, on the fly, but, for now, it's
implemented in a different way by evolution and some work will be needed
to be done to change it.

Created attachment 242243 [details] [review]
Bug #654950 - Contact doesn't fetch photo

This operation is only supported for Exchange Server newer than 2010_SP2.
Ideally we might could populate which item can be changed or not
according with the server version, on the fly, but, for now, it's
implemented in a different way by evolution and some work will be needed
to be done to change it.

Review of attachment 242239 [details] [review]:

I know the below comments are more about cleanup of the old code, but I hope you'll agree it'll make the code easier to read. I'll check actual functionality together with the second patch.

::: src/server/e-ews-connection.c
@@ +1586,3 @@
+const gchar *
+e_ews_attachment_info_get_inlined_data (EEwsAttachmentInfo *info,
+				      gsize *len)

align params with its above line param (couple times below as well)

@@ +5647,3 @@
+	switch (type) {
+		case E_EWS_ATTACHMENT_INFO_TYPE_URI:
+		{

this bracket should be one line above

@@ +5662,3 @@
+			if (stat (filepath, &st) == -1) {
+				g_warning ("Error while calling stat() on %s\n", filepath);
+				return;

I know this is not your change, you only changed indentation, but please fix the leak of 'filepath' on error.

@@ +5674,2 @@
+			buffer = malloc (length);
+			if (read (fd, buffer, length) != length) {

(I know it's not your code) the malloc can fail too. What about changing the whole old code to use g_file_get_contents(), which seems to be easier to use?

@@ +5682,2 @@
+			filename = strrchr (filepath, '/');
+			filename = filename ? g_strdup (++filename) : g_strdup (filepath);

I do not see this one freed

@@ +5708,3 @@
 
+	if (type == E_EWS_ATTACHMENT_INFO_TYPE_URI)
+		free (buffer);

a) g_free (buffer);
b) what about using two variables:
     const gchar *content;
     gchar *buffer = NULL;
   and that 'content' will be used as the 'buffer' is now, while
   the buffer will be set only when a URI attachment is used, thus
   you avoid a typecast of e_ews_attachment_info_get_inlined_data ()
   and you can always call g_free (buffer); because it'll be
   initialized to NULL?

@@ +6001,3 @@
 		}
+
+		if (info && attach_id) {

Does it leak here if the condition fails?

Review of attachment 242243 [details] [review]:

Please check the below, then I'll test the functionality.

::: src/addressbook/e-book-backend-ews.c
@@ +396,3 @@
+
+	id = e_ews_item_get_id (item);
+	g_hash_table_insert (ebews->priv->photos, g_strdup (id->id), (gchar *)contact_photo->data);

space in front of typecast (the same 3 lines below)

@@ +884,3 @@
+		GSList *list = NULL;
+
+		attachment_id = g_hash_table_lookup (ebews->priv->photos, id);

Hmm, does it make sense to keep all the attachment IDs for photos in memory? Consider a company with 10.000+ employees, each with a photo on his contact for better recognition. It means 10.000 * 2 * (len of uid) in memory for something quite rarely used, not counting overhead on memory for the GHashTable. I know the advantage is speed, but I'm afraid the trade-off is too high.

@@ +1203,3 @@
 
+		/*
+		 * The contact photo is basically and attachment with a special name.

s/and/an/

@@ +1442,2 @@
+			/* set item id */
+			item_id = e_ews_item_get_id ((EEwsItem *) items->data);

'(EEwsItem *) items->data' equals to 'item' assigned two lines above, doesn't it?

@@ +1452,3 @@
+
+			e_contact_set (modify_contact->old_contact, E_CONTACT_UID, id);
+			e_contact_set (modify_contact->old_contact, E_CONTACT_REV, change_key);

Hmm, does it make sense to modify the old_contact here? It's supposed to be just freed after the call is done, if I recall correctly.

@@ +3438,3 @@
 	priv = g_new0 (EBookBackendEwsPrivate, 1);
 	priv->ops = g_hash_table_new (NULL, NULL);
+	priv->photos = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free);

There is missing a destroy on this new member, but I'm afraid it will not work if you consider that the users can close evolution and evolution-addressbook-factory in the time. What I thought of during our IRC chat was to check available attachments in set_photo() and do what is necessary base don online fresh data, rather than on stale/old local cache data. It'll take more time, but it'll worth it.

::: src/server/e-ews-connection.c
@@ +958,3 @@
+		 * updated.
+		 */
+		if (error->code == 7)

Oh, this is not correct. What is the '7'? You should use g_error_matches() with defined domain and an enum value for the error, rather than some random integer, which can change in the future.

::: src/server/e-ews-item.c
@@ +288,3 @@
+	if (priv->contact_photo) {
+		g_slist_foreach (priv->contact_photo, (GFunc) g_free, NULL);
+		g_slist_free (priv->contact_photo);

g_slist_free_full() might make it easier :)

@@ +1581,3 @@
+		} else if (g_strcmp0 (param_name, "IsContactPhoto") == 0) {
+			value = e_soap_parameter_get_string_value (subparam);
+			is_contact_photo = !(g_strcmp0(value, "true") == 0);

this test seems odd to me

One more thing, about the '7' error code, I really thought that you'll not fire a server request with empty UpdateItem, it only shows a faulty client, which we do not want to be. My idea was to call e_soap_message_get_xml_doc() and then traverse the XML and check whether the UpdateItem has any children or not. If not, then skip the request completely, instead of passing nothing to the server and expecting an error. The other option, as you mentioned on IRC, is to change the callback prototype to return whether it wrote anything to the message or not.

(In reply to comment #15)
 
> @@ +884,3 @@
> +        GSList *list = NULL;
> +
> +        attachment_id = g_hash_table_lookup (ebews->priv->photos, id);
> 
> Hmm, does it make sense to keep all the attachment IDs for photos in memory?
> Consider a company with 10.000+ employees, each with a photo on his contact for
> better recognition. It means 10.000 * 2 * (len of uid) in memory for something
> quite rarely used, not counting overhead on memory for the GHashTable. I know
> the advantage is speed, but I'm afraid the trade-off is too high.
> 

Hmm. You're right.
 
> @@ +3438,3 @@
>      priv = g_new0 (EBookBackendEwsPrivate, 1);
>      priv->ops = g_hash_table_new (NULL, NULL);
> +    priv->photos = g_hash_table_new_full (g_str_hash, g_str_equal, g_free,
> g_free);
> 
> There is missing a destroy on this new member, but I'm afraid it will not work
> if you consider that the users can close evolution and
> evolution-addressbook-factory in the time. What I thought of during our IRC
> chat was to check available attachments in set_photo() and do what is necessary
> base don online fresh data, rather than on stale/old local cache data. It'll
> take more time, but it'll worth it.

I really had missed what you pointed above and now I can understand why it will worth it.

> 
> ::: src/server/e-ews-connection.c
> @@ +958,3 @@
> +         * updated.
> +         */
> +        if (error->code == 7)
> 
> Oh, this is not correct. What is the '7'? You should use g_error_matches() with
> defined domain and an enum value for the error, rather than some random
> integer, which can change in the future.

Yeah. This is completely wrong.
I didn't know how to handle this situation and, as you said, you were expecting that I wouldn't fire an invalid server request.

(In reply to comment #16)
> One more thing, about the '7' error code, I really thought that you'll not fire
> a server request with empty UpdateItem, it only shows a faulty client, which we
> do not want to be. My idea was to call e_soap_message_get_xml_doc() and then
> traverse the XML and check whether the UpdateItem has any children or not. If
> not, then skip the request completely, instead of passing nothing to the server
> and expecting an error. The other option, as you mentioned on IRC, is to change
> the callback prototype to return whether it wrote anything to the message or
> not.

Your idea is better than mine. At least changes will be "centralized" in one place.
Sorry about the confusion, I really didn't realize about the e_soap_message_get_xml_doc() before.

Created attachment 242480 [details] [review]
Handle inlined attachments in  e_ews_connection_{create,get}_attachments

Created attachment 242481 [details] [review]
Bug #654950 - Contact doesn't fetch photo

This operation is only supported for Exchange Server newer than 2010_SP2.
Ideally we might could populate which item can be changed or not
according with the server version, dinamically, but, for now, it is
implemented in a different way by evolution and some work will be
necesaary to change it.

Created attachment 242494 [details] [review]
Bug #654950 - Contact doesn't fetch photo

This operation is only supported for Exchange Server newer than 2010_SP2.
Ideally we might could populate which item can be changed or not
according with the server version, dinamically, but, for now, it is
implemented in a different way by evolution and some work will be
necesaary to change it.

Created attachment 242532 [details] [review]
Bug #654950 - Contact doesn't fetch photo

This operation is only supported for Exchange Server newer than 2010_SP2.
Ideally we might could populate which item can be changed or not
according with the server version, dinamically, but, for now, it is
implemented in a different way by evolution and some work will be
necesaary to change it.

Review of attachment 242480 [details] [review]:

Patch looks fine (still untested on my side, that I'll do with the second patch), just fix the three things below and commit.

::: src/server/e-ews-connection.c
@@ +5668,3 @@
+			g_file_get_contents (uri, &buffer, &length, &error);
+			if (error != NULL) {
+				g_free (filepath);

and g_free (filename) (or populate it after this g_file_get_contents()).

@@ +5675,2 @@
+			g_free (filepath);
+			break;

if you assign content = buffer, then you can avoid below check

@@ +5738,3 @@
 	for (l = files; l != NULL; l = g_slist_next (l))
+		if (!e_ews_connection_attach_file (msg, l->data)) {
+			g_simple_async_result_complete_in_idle (simple);

I thought that you'll provide a GError to e_ews_connection_attach_file(), which will propagate that here, which you assign to the 'simple', thus the caller knows the issue, because otherwise it looks like a properly finished request

::: src/server/e-ews-item.h
@@ +130,3 @@
+	EEwsAttachmentInfoType type;
+	union {
+		struct  inlined inlined;

I see you didn't define it 'inline' (not as the name, but as an action), but that's ok, this works too :)

Review of attachment 242532 [details] [review]:

The below is just for the read-review. From the functional review, I cannot change only email on an already filled contact, the element_has_child() returns FALSE, because xpath_eval() returns NULL.

::: src/addressbook/e-book-backend-ews.c
@@ +862,3 @@
+
+	if (error != NULL) {
+		g_clear_error (&error);

The g_clear_error() takes care of NULL/not NULL, thus you can just call it, and, if you do not use the error, neither for a debugging on a console, then you can just avoid it completely (a use for g_warning() or g_message(), might be better, though; or propagate it to the caller).

@@ +866,3 @@
+	}
+
+	g_slist_free_full (files, (GDestroyNotify) e_ews_attachment_info_free);

This should be done always, not only when there is no error, same as the 'id' free.

@@ +902,3 @@
+	if (error != NULL) {
+		g_clear_error (&error);
+		return;

free the contact_item_ids too

@@ +917,3 @@
+
+		g_slist_free_full (new_items, g_object_unref);
+		g_slist_free_full (contact_item_ids, g_free);

contact_item_ids, same as new_items, is not freed if there are no other attachments on the contact

::: src/server/e-ews-connection.c
@@ +4104,3 @@
+
+	if (result->type == XPATH_NODESET && xmlXPathNodeSetIsEmpty (result->nodesetval))
+	{

just a coding style, bracket on the same line as 'if'

@@ +4124,3 @@
+
+	doc = e_soap_message_get_xml_doc (message);
+	xpctx = xmlXPathNewContext (doc);

this one is not freed

@@ +4141,3 @@
+		return FALSE;
+
+	if (!xmlXPathNodeSetGetLength(result->nodesetval))

just a coding style, space after Length

@@ +4147,3 @@
+	node = nodeset->nodeTab[0];
+	if (!node->children)
+		goto exit;

is it necessary to go this far? I thought that the check for xmlXPathNodeSetGetLength() is enough, or alternatively  xmlXPathNodeSetIsEmpty() which is used in xpath_eval()

@@ +5783,3 @@
 	e_ews_message_write_string_parameter (msg, "Name", NULL, filename);
+	if (contact_photo)
+		e_ews_message_write_string_parameter(msg, "IsContactPhoto", NULL, "true");

just a coding style, space after '...parameter'

Created attachment 242565 [details] [review]
Handle inlined attachments in  e_ews_connection_{create,get}_attachments

Created attachment 242566 [details] [review]
Bug #654950 - Contact doesn't fetch photo

This operation is only supported for Exchange Server newer than 2010_SP2.
Ideally we might could populate which item can be changed or not
according with the server version, dinamically, but, for now, it is
implemented in a different way by evolution and some work will be
necesaary to change it.

Review of attachment 242565 [details] [review]:

Thanks. This one looks good. Feel free to commit it to master.

Review of attachment 242566 [details] [review]:

Final changes, please :)

::: src/addressbook/e-book-backend-ews.c
@@ +882,3 @@
+	}
+
+	g_slist_free_full (files, (GDestroyNotify) e_ews_attachment_info_free);

you forgot of this, from the previous review

@@ +939,3 @@
+			g_slist_free_full (new_items, g_object_unref);
+			g_propagate_error (error, local_error);
+			return;

add also g_slist_free_full (contact_item_ids, g_free);

@@ +1597,3 @@
+
+			if (error != NULL) {
+				g_warning ("Error while Modifying contact: %s", error->message);

if you propagate the error to the UI, then no need for the warning. When I spoke about warnings I thought of places where you get an error, but do not do anything but free it.

::: src/server/e-ews-connection.c
@@ +4136,3 @@
+			(xmlChar *) "http://schemas.microsoft.com/exchange/services/2006/messages");
+
+	full_path = g_strdup_printf ("/s:Envelope/s:Body/%s", path);

I would not be afraid of define full path in the caller, it's OK to know how the XML looks like inside. And as we spoke on IRC, add also the 't' namespace for 'types' and (see below...)

@@ +4217,3 @@
 		simple, async_data, (GDestroyNotify) async_data_free);
 
+	if (!element_has_child (msg, "m:UpdateItem/m:ItemChanges/ItemChange/Updates")) {

(...continue from above) check here for two paths, one like the above, the other with "...t:ItemChange/t:Updates", because this seems like a bug in libxml, because I'd expect an Exchange server to reject such XML with a validation error, but it doesn't do that, thus the libxml seems to forget of the "default" namespace when defined on a previous element.

Functional things:
a) remove an email doesn't remove it on the server (most likely caused by other
   things, the element_has_child works as expected, thus no need to change API)

b) if I attach a .txt file to a contact in OWA, then editing the contact
   removes this non-photo attachment and I lose data on the server (note
   there can be attached S/MIME certificates on contacts too).

(In reply to comment #29)
> a) remove an email doesn't remove it on the server (most likely caused by other
>    things, the element_has_child works as expected, thus no need to change API)

I filled bug #698971 for this, I can reproduce it without this patch as well.

Created attachment 242759 [details] [review]
Bug #654950 - Contact doesn't fetch photo

This operation is only supported for Exchange Server newer than 2010_SP2.
Ideally we might could populate which item can be changed or not
according with the server version, dinamically, but, for now, it is
implemented in a different way by evolution and some work will be
necesaary to change it.

Created attachment 242760 [details] [review]
Bug #654950 - Contact doesn't fetch photo

This operation is only supported for Exchange Server newer than 2010_SP2.
Ideally we might could populate which item can be changed or not
according with the server version, dinamically, but, for now, it is
implemented in a different way by evolution and some work will be
necesaary to change it.

Created attachment 242791 [details] [review]
Bug #654950 - Contact doesn't fetch photo

This operation is only supported for Exchange Server newer than 2010_SP2.
Ideally we might could populate which item can be changed or not
according with the server version, dinamically, but, for now, it is
implemented in a different way by evolution and some work will be
necesaary to change it.

Review of attachment 242791 [details] [review]:

The g_slist_free_full() and g_slist_free() accept NULL as its first argument, thus it's safe to use it directly, same as g_free() accepts NULL as its argument.

::: src/addressbook/e-book-backend-ews.c
@@ +394,3 @@
+			cancellable,
+			&local_error))
+		goto exit;

the convention is that the function returns FALSE when there is an error, either a programming error (for those g_return_val_if_fail() macros) or the server errors, thus you get FALSE and the local_error is populated with a GError together, but you do not propagate the error at the end of this function. Usually, and here it seems the same, you do not define local_error, but simply pass 'error' into the _sync() calls and check only whether it returned TRUE/FALSE. It's simple, and you get things for free.

@@ +905,3 @@
+			files,
+			cancellable,
+			&local_error);

similar here, just pass 'error' into the function.

I got double-free here:

+ Trace 231874

(In reply to comment #35)
> I got double-free here:

oops, not a double-free, but:
> *** glibc detected *** /usr/libexec/evolution-addressbook-factory: free():
> invalid pointer: 0x00007ff278003560 ***

There might be an EEwsItem, a GObject descendant, not a simple structure, inside the new_items GSList.

Created attachment 242805 [details] [review]
Bug #654950 - Contact doesn't fetch photo

This operation is only supported for Exchange Server newer than 2010_SP2.
Ideally we might could populate which item can be changed or not
according with the server version, dinamically, but, for now, it is
implemented in a different way by Evolution.

Review of attachment 242805 [details] [review]:

Looks good from functional point of view, my smoke tests seem to be OK. Please consider fixing the below (all *error, not only the first I wrote the comment at), and commit to master. Thanks.

::: src/addressbook/e-book-backend-ews.c
@@ +394,3 @@
+			error);
+
+	if (*error != NULL)

oops, I meant to write it like this:
   if (!e_ews_connection_get_photo_attachment_id_sync (...))
      goto exit;
In general, there is not guarantee that the 'error' will not be NULL, thus you can dereference NULL, which leads to crash (I mean in general, this code is fine, with this respect), that's why most of the functions return "success", thus you can easily check whether they worked or not.

@@ +903,3 @@
+compare_photos (EContactPhoto *old,
+		EContactPhoto *new)
+{

what about 'photos_equal', instead of 'compare_photos' (similar as g_str_equal() is)?

@@ +946,1 @@
+	g_print ("FIDENCIO -- PHOTOS ARE DIFFERENT\n");

debug print

@@ +968,3 @@
+
+		if (*error != NULL)
+			goto exit;

hmm, it's tricky here, the function doesn't return 'success'. I guess
  if (!deleted_attachments)
does the same job

@@ +2603,3 @@
 			FALSE, NULL, E_EWS_BODY_TYPE_TEXT, &new_items, NULL, NULL,
 			cancellable, error);
+	}

this change is not needed here, is it?

Pushed!
https://git.gnome.org/browse/evolution-ews/commit/?id=116808856258ead2df22dfef291ee2f40fec5b4f

https://git.gnome.org/browse/evolution-ews/commit/?id=477ea341525bb5882d22dc836b1d73e27f9a6b9f

Please see bug #660748 comment #4 for a typo in the e_ews_connection_attach_file() function.