Created UpdateItem and added necessary trait implementations #18

tobypilling · 2024-10-23T14:25:18Z

I put this together for mark read/unread - can you let me know if you would have approached it differently?

I tried to follow create_item and get_item but wasn't sure how much I should be adjusting common.rs vs keeping things local to update_item.rs

Can you maybe give some guidance?

src/types/common.rs

src/types/update_item.rs

src/types/common.rs

leftmostcat

Brendan has covered most of the things I would, but there are a few other things I'm bringing up here. I use Conventional Comments as a guideline for what's blocking and non-blocking; as such, todo comments will need to be addressed before approval, and I generally expect question comments to be answered as well.

As regards documentation, we should definitely remove the copy-pasted documentation before merging. I'll leave it up to you whether to rewrite it or leave it off for now, as a documentation pass is needed for several areas as it is.

src/types/update_item.rs

src/types/common.rs

src/types/update_item.rs

…w as Option<MessageDisposition>) and add Clone for all types

leftmostcat

I'm asking for quite a few changes here and a lot of them are about style and convention in documentation or ordering of files for consistency. Additionally, there are some slightly bigger changes that are needed because of decisions we made earlier which impact the way the Message struct is used. I will put up a PR into this branch to try to illustrate what I'm expecting rather than flooding this one with suggestions.

leftmostcat · 2024-10-28T18:52:17Z

src/types/common.rs

@@ -315,7 +327,7 @@ pub enum BaseItemId {
 /// The unique identifier of an item.
 ///
 /// See <https://learn.microsoft.com/en-us/exchange/client-developer/web-service-reference/itemid>
-#[derive(Clone, Debug, Deserialize, XmlSerialize, PartialEq)]
+#[derive(Clone, Default, Debug, Deserialize, XmlSerialize, PartialEq)]


todo: Default doesn't make sense here. See next comment.

src/types/common.rs

src/types/update_item.rs

leftmostcat · 2024-10-28T20:48:20Z

src/types/update_item.rs

+    AlwaysOverwrite,
+}
+
+/// Represents a change to an individual item, including the item ID and updates.


nitpick: This and the other struct comments throughout the file should follow the noun phrase convention.

leftmostcat · 2024-10-28T20:50:20Z

src/types/update_item.rs

+    pub message: Message,         // The new value for the specified field.
+}
+
+/// Struct representing updates to be applied to an item.


suggestion: "Struct representing" is implicit in the declaration already and doesn't really add anything here.

Suggested change

/// Struct representing updates to be applied to an item.

/// A list of changes to fields, with each element representing a single change.

leftmostcat · 2024-10-28T20:50:48Z

src/types/update_item.rs

+
+/// Struct representing updates to be applied to an item.
+///
+/// This struct is used to create an UpdateItem request.


nitpick: This doesn't really add anything not implied by the type hierarchy.

leftmostcat · 2024-10-28T20:52:32Z

src/types/update_item.rs

+/// See <https://learn.microsoft.com/en-us/exchange/client-developer/web-service-reference/updates-item>
+#[derive(Clone, Debug, XmlSerialize)]
+pub struct Updates {
+    pub set_item_field: SetItemField,


issue: A couple problems here. First is that this only allows for a single field to be changed per update and it should instead be a Vec. Second is that it doesn't allow for append/delete operations. That should be done by using an enum.

leftmostcat · 2024-10-28T20:53:30Z

src/types/update_item.rs

+    pub item_changes: Vec<ItemChange>,
+}
+
+/// Struct representing the field update operation.


suggestion: As below, "Struct representing" doesn't add much here, and the second paragraph could easily be folded into this one to give a more complete description, but as below, this needs additional revision.

leftmostcat · 2024-10-28T20:55:51Z

src/types/update_item.rs

+impl UpdateItem {
+    /// Adds another `ItemChange` to the `UpdateItem` request.
+    pub fn add_item_change(&mut self, item_change: ItemChange) {
+        self.item_changes.item_changes.push(item_change);
+    }
+}


issue: There's still not really cause to have this be a separate function. The field to be changed is public, so while this provides a slight shortcut to accessing that field, it also provides limited means of interacting with it. In particular, since we generally create these operation structs all at once, push() isn't very useful as a way of building a list of changes. Instead, we should generally be preferring using Rust iterators and collect().

leftmostcat · 2024-10-28T20:58:20Z

src/types/update_item.rs

+impl UpdateItem {
+    /// Adds another `ItemChange` to the `UpdateItem` request.
+    pub fn add_item_change(&mut self, item_change: ItemChange) {
+        self.item_changes.item_changes.push(item_change);
+    }
+}
+
+impl Operation for UpdateItem {
+    type Response = UpdateItemResponse;
+}
+
+impl EnvelopeBodyContents for UpdateItem {
+    fn name() -> &'static str {
+        "UpdateItem"
+    }
+}
+
+/// A response to an [`UpdateItem`] request.
+///
+/// See <https://learn.microsoft.com/en-us/exchange/client-developer/web-service-reference/updateitemresponse>
+#[derive(Clone, Debug, Deserialize)]
+#[serde(rename_all = "PascalCase")]
+pub struct UpdateItemResponse {
+    pub response_messages: ResponseMessages,
+}
+
+impl OperationResponse for UpdateItemResponse {}
+
+impl EnvelopeBodyContents for UpdateItemResponse {
+    fn name() -> &'static str {
+        "UpdateItemResponse"
+    }
+}
+
+#[derive(Clone, Debug, Deserialize)]
+#[serde(rename_all = "PascalCase")]
+pub struct ResponseMessages {
+    pub update_item_response_message: Vec<UpdateItemResponseMessage>,
+}
+
+#[derive(Clone, Debug, Deserialize)]
+#[serde(rename_all = "PascalCase")]
+pub struct UpdateItemResponseMessage {
+    /// The status of the corresponding request, i.e. whether it succeeded or
+    /// resulted in an error.
+    pub response_class: ResponseClass,
+
+    pub response_code: Option<ResponseCode>,
+
+    pub message_text: Option<String>,
+
+    pub items: Items,
+}


nitpick: I have generally kept the actual definition of the operations at the beginning of the file with operation-specific types declared below. I don't have solid reasoning for this, but it's consistent and puts the named part of the file up front.

Even nitpickier but reflected in that tendency is that, in Rust, I tend to prefer to declare a type and then declare additional types on which it depends below them, e.g.:

struct Foo { bar: Bar, } struct Bar {}

src/types/update_item.rs

babolivier

One minor documentation nit, otherwise lgtm :)

babolivier · 2024-10-31T18:38:44Z

src/types/common.rs

@@ -177,6 +177,18 @@ pub enum DistinguishedPropertySet {
    UnifiedMessaging,
 }

+/// The action an Exchange server will take upon creating a `Message` item.


Suggested change

/// The action an Exchange server will take upon creating a `Message` item.

/// The action an Exchange server will take upon creating or updating a `Message` item.

Co-authored-by: Sean Burke <[email protected]>

babolivier requested changes Oct 23, 2024

View reviewed changes

leftmostcat requested changes Oct 23, 2024

View reviewed changes

Created UpdateItem and added necessary trait implementations

188d3da

tobypilling force-pushed the tobypilling/update_item branch from 416442d to 188d3da Compare October 23, 2024 20:49

tobypilling added 7 commits October 28, 2024 11:49

Respond to feedback on UpdateItem, unduplicate MessageDisposition (no…

f784450

…w as Option<MessageDisposition>) and add Clone for all types

Also added default trait for ConflictResolution

468a45a

Adjusted comment to use a noun phrase for ConflictResolution

4277779

Fixed formatting issues

9cfb448

Test formatting

b1c4485

Fix formatting again

aaf24c1

Test broken formatting

475bd39

tobypilling force-pushed the tobypilling/update_item branch from cb30763 to 475bd39 Compare October 28, 2024 19:34

tobypilling added 3 commits October 28, 2024 15:35

Merge branch 'main' into tobypilling/update_item

8d77946

Test broken formatting

ccae29c

Fix broken formatting

423b52b

tobypilling requested review from leftmostcat and babolivier October 28, 2024 19:46

leftmostcat requested changes Oct 28, 2024

View reviewed changes

leftmostcat mentioned this pull request Oct 28, 2024

Update documentation and get rid of serialize-only Message type #23

Merged

Update documentation and get rid of serialize-only Message type (#23)

483be7c

tobypilling requested a review from leftmostcat October 29, 2024 17:42

leftmostcat requested changes Oct 29, 2024

View reviewed changes

src/types/update_item.rs Show resolved Hide resolved

Add reference to EWS updateitem docs

d3e2e90

leftmostcat approved these changes Oct 29, 2024

View reviewed changes

babolivier approved these changes Oct 31, 2024

View reviewed changes

tobypilling merged commit 3b1d4db into main Oct 31, 2024
2 checks passed

tobypilling deleted the tobypilling/update_item branch October 31, 2024 19:19

leftmostcat added a commit that referenced this pull request Oct 31, 2024

Create UpdateItem and add necessary trait implementations (#18)

cd5d38b

Co-authored-by: Sean Burke <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Created UpdateItem and added necessary trait implementations #18

Created UpdateItem and added necessary trait implementations #18

tobypilling commented Oct 23, 2024

leftmostcat left a comment

leftmostcat left a comment

leftmostcat Oct 28, 2024

leftmostcat Oct 28, 2024

leftmostcat Oct 28, 2024

leftmostcat Oct 28, 2024

leftmostcat Oct 28, 2024

leftmostcat Oct 28, 2024

leftmostcat Oct 28, 2024

leftmostcat Oct 28, 2024

babolivier left a comment

babolivier Oct 31, 2024

	/// Struct representing updates to be applied to an item.
	/// A list of changes to fields, with each element representing a single change.

	/// The action an Exchange server will take upon creating a `Message` item.
	/// The action an Exchange server will take upon creating or updating a `Message` item.

Created UpdateItem and added necessary trait implementations #18

Created UpdateItem and added necessary trait implementations #18

Conversation

tobypilling commented Oct 23, 2024

leftmostcat left a comment

Choose a reason for hiding this comment

leftmostcat left a 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

babolivier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment