Submission for April Pull Request#16
Conversation
… deleted, completed tests
Lab3 to Main
Run tests without building again.
Run tests without building again.
Included connection strings in environment of integration tests to remove them from app_settings.
Merge Lab3 to Main
Merge Comment Fixes to Main
Add ServiceUnavailableException
Adjustments for functional tests
Accomodate functional test updates
Add ExceptionMiddleware to AprilPRSubmission
| } | ||
| catch (MessageExistsException e) | ||
| { | ||
| _logger.LogError(e, "Error adding message: {ErrorMessage}", e.Message); |
There was a problem hiding this comment.
no need to log an error here (an error means something went wrong in your service that developers should know about), the conflict will show up in the logs
| Task<SendMessageResponse> AddMessage(string conversationId, bool isFirstMessage, SendMessageRequest request); | ||
| Task<SendMessageResponse> AddFirstMessage(string conversationId, SendMessageRequest request); |
There was a problem hiding this comment.
Why do we need both here? An API should be minimal
|
|
||
| public interface IUserConversationService | ||
| { | ||
| Task<StartConversationResult> CreateConversation(StartConversationRequest request); |
There was a problem hiding this comment.
Try to be consistent with naming
| Text = request.Text | ||
| }; | ||
|
|
||
| await _messageStore.AddMessage(conversationId, message); |
There was a problem hiding this comment.
What if the message already exists because of a partial failure in the previous attempt? The modified time would not be updated
|
|
||
| private void AuthorizeSender(string conversationId, string senderUsername) | ||
| { | ||
| string[] usernames = ConversationIdUtilities.SplitConversationId(conversationId); |
There was a problem hiding this comment.
The fact that the split is happening here and the concatenation elsewhere is problematic. This class should not depend on implementation details happening elsewhere that can change, otherwise the coupling is too tight. Also from readability perspective, I can't tell at the moment where the concatenation is happening.
|
|
||
| string username1 = request.Participants.ElementAt(0); | ||
| string username2 = request.Participants.ElementAt(1); | ||
| string conversationId = ConversationIdUtilities.GenerateConversationId(username1, username2); |
There was a problem hiding this comment.
here we go, so the concatenation is happening here and the split elsewhere.. that's a problem. It should only be known to one class
| Task AddMessage(string conversationId, Message message); | ||
| Task<Message?> GetMessage(string conversationId, string messageId); | ||
| Task<GetMessagesResult> GetMessages(string conversationId, GetMessagesParameters parameters); | ||
| Task<bool> ConversationPartitionExists(string conversationId); |
There was a problem hiding this comment.
the concept of partition is a cosmos implementation detail, it should not be part of the interface
|
|
||
| public interface IUserConversationStore | ||
| { | ||
| Task UpsertUserConversation(UserConversation userConversation); |
There was a problem hiding this comment.
This ideally shouldn't be an upsert, it should be an Add that throws if the conversation already exists
|
|
||
| public class ConversationIdUtilities | ||
| { | ||
| private static readonly char Seperator = '_'; |
There was a problem hiding this comment.
it's good that you moved this to a utility but this should be only used by one class in the application, otherwise classes would depend on each other implementations not just their interfaces.
nehmebilal
left a comment
nehmebilal
left a comment
There was a problem hiding this comment.
Looks great overall, good work! You have room for improvement with responsibilities especially around the concatenation of username which is a concept that several classes are depending on. Ideally, this should be buried in the conversations store or the conversation service and no one else should need to know about (not even the tests).
3 participants
No description provided.