[TeamsBotApps] Add all message activity handlers and schema #282
Conversation
This reverts commit 6f243f9.
rido-min
left a comment
rido-min
left a comment
There was a problem hiding this comment.
this is looking great !! I have some suggestions/comments that might be worth it to review in person
| /// <summary> | ||
| /// Represents a message reaction activity. | ||
| /// </summary> | ||
| public const string MessageReaction = "messageReaction"; |
There was a problem hiding this comment.
is there any reason to have these types in Core? I think those should be moved to TeamsSchema
There was a problem hiding this comment.
Nope, I was just getting confused where to put it
| if (addText) | ||
| { | ||
| string? currentText = activity.Properties.TryGetValue("text", out object? value) ? value?.ToString() : null; | ||
| string? currentText = activity.Properties.TryGetValue("text", out var t) ? t?.ToString() : null; |
There was a problem hiding this comment.
if we need the text maybe we could use MessageActivity.. I dont think mentions make sense in other activity types
| MessageActivity activity = new("Hello World"); | ||
| Assert.Equal(ActivityType.Message, activity.Type); | ||
| Assert.Equal(TeamsActivityType.Message, activity.Type); | ||
| Assert.Equal("Hello World", activity.Text); |
There was a problem hiding this comment.
We might want to check TextFormat
There was a problem hiding this comment.
This test is for the constructor which doesn't set TextFormat, you think it should ? Other tests check it
| .Rebase(); | ||
| public static new TeamsActivity FromJsonString(string json) | ||
| { | ||
| using JsonDocument doc = JsonDocument.Parse(json); |
There was a problem hiding this comment.
this seems like a double deserialization, if we only need the type we could use a reader.
| /// </summary> | ||
| public static readonly string MessageReaction = "messageReaction"; | ||
|
|
||
| internal static readonly Dictionary<string, (Func<CoreActivity, TeamsActivity> FromActivity, Func<string, TeamsActivity> FromJson)> ActivityDeserializerMap = new() |
There was a problem hiding this comment.
this is very interesting, let's chat about it
| public static new TeamsActivity FromJsonString(string json) => | ||
| FromJsonString(json, TeamsActivityJsonContext.Default.TeamsActivity) | ||
| .Rebase(); | ||
| public static new TeamsActivity FromJsonString(string json) |
There was a problem hiding this comment.
we coud think of adding a generic here to return the concrete activities
There was a problem hiding this comment.
added overload for that
| /// <returns>A MessageReactionActivity instance.</returns> | ||
| public static new MessageReactionActivity FromJsonString(string json) | ||
| { | ||
| MessageReactionActivity activity = JsonSerializer.Deserialize( |
There was a problem hiding this comment.
can use FromJson from the base?
There was a problem hiding this comment.
yes, added generic overload/helper to base class and using that
MehakBindra
changed the title
No description provided.