Submission for March Pull Request by nadimakk · Pull Request #9 · nadimakk/ChatService

nadimakk · 2023-04-01T19:56:17Z

No description provided.

… deleted, completed tests

…ection

Lab3 to Main

Run tests without building again.

Included connection strings in environment of integration tests to remove them from app_settings.

Merge Lab3 to Main

nehmebilal · 2023-04-04T08:56:07Z

+        using (_logger.BeginScope(new Dictionary<string, object>
+               {
+                   {"ConversationId", conversationId},
+                   {"SenderUsername", request.SenderUsername}


nehmebilal · 2023-04-04T08:56:59Z

+        {
+            try
+            {
+                SendMessageResponse response = await _messageService.AddMessage(conversationId, false, request);


it's not clear what the false represents here, use a named parameter to make it obvious.

nehmebilal · 2023-04-04T08:58:55Z

+            try
+            {
+                var result = await _imageService.DownloadImage(imageId);
+                _logger.LogInformation("Downloaded image with id {id}.", imageId);


remove, only log info for things added / updated, not needed for get requests

nehmebilal · 2023-04-04T08:59:11Z

+            try
+            {
+                var profile = await _profileService.GetProfile(username);
+                _logger.LogInformation("Profile of {Username} fetched.", username);


nehmebilal · 2023-04-04T08:59:33Z

+    [HttpPost]
+    public async Task<ActionResult<Profile>> PostProfile(Profile profile)
+    {
+        using (_logger.BeginScope("{Profile}", profile))


nehmebilal · 2023-04-04T09:01:05Z

+  "iisSettings": {
+    "windowsAuthentication": false,
+    "anonymousAuthentication": true,
+    "iisExpress": {
+      "applicationUrl": "http://localhost:61550",
+      "sslPort": 44359
+    }
+  },


can be deleted

nehmebilal · 2023-04-04T09:01:12Z

+    "IIS Express": {
+      "commandName": "IISExpress",
+      "launchBrowser": true,
+      "launchUrl": "swagger",
+      "environmentVariables": {
+        "ASPNETCORE_ENVIRONMENT": "Development"
+      }
+    }


can be deleted

nehmebilal · 2023-04-04T09:02:27Z

+public interface IImageService
+{
+    Task<UploadImageServiceResult> UploadImage(Image image);
+    Task<FileContentResult> DownloadImage(string imageId);


FileContentResult is the controller business, the service layer should not be aware that it is being called from a controller.

nehmebilal · 2023-04-04T09:09:39Z

+
+    public async Task<SendMessageResponse> AddFirstMessage(string conversationId, SendMessageRequest request)
+    {
+        return await AddMessage(conversationId, true, request);


use a named parameter to make it easy to read what the true represents

nehmebilal · 2023-04-04T09:10:11Z

+        ValidateConversationId(conversationId);
+        ValidateLimit(limit);
+        ValidateLastSeenConversationTime(lastSeenConversationTime);


Good job on using private methods to improve readability

nehmebilal · 2023-04-04T09:11:30Z

+        var result = await _messageStore.GetMessages(
+            conversationId, limit, orderBy, continuationToken, lastSeenConversationTime);
+
+        return new GetMessagesServiceResult


GetMessagesResult. If you have another one in the controller with the same name, the controller object should be called GetMessagesResponse

nehmebilal · 2023-04-04T09:14:52Z

+        await _imageService.DeleteImage(profile.ProfilePictureId);
+        await _profileStore.DeleteProfile(username);


Do it in parallel:

await Task.WhenAll( _imageService.DeleteImage(profile.ProfilePictureId), _profileStore.DeleteProfile(username) );

nehmebilal · 2023-04-04T09:16:22Z

+
+public class UserConversationService : IUserConversationService
+{
+


remove blank line

nehmebilal · 2023-04-04T09:20:11Z

+        if (!await _profileService.ProfileExists(username1))
+        {
+            throw new ProfileNotFoundException($"A profile with the username {username1} was not found.");
+        }
+
+        if (!await _profileService.ProfileExists(username2))
+        {
+            throw new ProfileNotFoundException($"A profile with the username {username2} was not found.");
+        }


await Task.WhenAll(request.Participants.Select(username => ThrowIfParticipantNotFound(username)))

and this can be done in a private function:

await EnsureThatParticipantsExist(request.Participants);

nehmebilal · 2023-04-04T13:04:29Z

+    {
+        await AddMultipleUserConversations(_userConversation1, _userConversation2, _userConversation3);
+
+        var response = await _userConversationStore.GetUserConversations(_userConversation.Username, 1, OrderBy.ASC, null, 1);


please always use named parameters when it's not clear what the parameter is. In this case, it's hard to guess what 1, null and 1 are without looking at the implementation

nehmebilal · 2023-04-04T13:04:51Z

+        await AddMultipleUserConversations(_userConversation1, _userConversation2, _userConversation3);
+
+        var response = await _userConversationStore.GetUserConversations(_userConversation.Username, 1, OrderBy.ASC, null, 1);
+        Assert.Equal(1, response.UserConversations.Count);


I believe you can use Assert.Single in this case

nehmebilal · 2023-04-04T13:06:37Z

+        response = await _userConversationStore.GetUserConversations(_userConversation.Username, 3, OrderBy.ASC, null, 1);
+        Assert.Equal(3, response.UserConversations.Count);
+
+        await DeleteMultipleUserConversations(_userConversation1, _userConversation2, _userConversation3);


this should be done in the cleanup not here because cleanup is executed even if the test fails halfway through

nehmebilal · 2023-04-04T13:07:59Z

+        if(_userConversation1.LastModifiedTime > lastSeenConversationTime) { userConversationsExpected.Add(_userConversation1); }
+        if(_userConversation2.LastModifiedTime > lastSeenConversationTime) { userConversationsExpected.Add(_userConversation2);}
+        if(_userConversation3.LastModifiedTime > lastSeenConversationTime) { userConversationsExpected.Add(_userConversation3);}
+        if(_userConversation.LastModifiedTime > lastSeenConversationTime) { userConversationsExpected.Add(_userConversation);}


if you had a list of conversation you can do this in a for loop instead of repeating 4 times the same code

nehmebilal · 2023-04-04T13:09:55Z

+
+    public async Task DisposeAsync()
+    {
+        await _userConversationStore.DeleteUserConversation(_userConversation.Username, _userConversation.ConversationId);


this cleanup can be used to delete all conversations, if you put them in a list

nehmebilal · 2023-04-04T13:14:04Z

+        UserConversation userConversation1 = new UserConversation
+        {
+            Username = username1,
+            ConversationId = conversationId,
+            LastModifiedTime = unixTimeNow
+        };
+        await _userConversationStore.CreateUserConversation(userConversation1);
+
+        UserConversation userConversation2 = new UserConversation
+        {
+            Username = username2,
+            ConversationId = conversationId,
+            LastModifiedTime = unixTimeNow
+        };
+        await _userConversationStore.CreateUserConversation(userConversation2);


await Task.WhenAll( CreateUserConversation(username1, conversationId, unixTimeNow), CreateUserConversation(username2, conversationId, unixTimeNow), )

Faster and easier on the eye :)

nehmebilal · 2023-04-04T13:20:05Z

+
+        foreach (UserConversation userConversation in userConversations)
+        {
+            string[] usernames = userConversation.ConversationId.Split('_');


The "_" separator is spread in multiple places. If we decide to change it, we have to change multiple places in the code, which is not ideal. This separator should live in one place and be hidden from the rest of the application.

nehmebilal · 2023-04-04T13:27:52Z

+        UserConversation userConversation1 = new UserConversation
+        {
+            Username = username1,
+            ConversationId = conversationId,
+            LastModifiedTime = unixTimeNow
+        };


There is a conceptual issue here. The UserConversation object doesn't say who is the other user involved in the conversation but the storage layer is not aware that the conversationId is a concatenation of usernames. I think you have two choices here:
1- The storage layer would own the knowledge of the separator being used and hide it from the rest of the application. You would pass in both usernames and ask the storage layer to create a conversation. The storage layer would create the conversationId using the concatenation and store the user conversation. The getUserConversations would return UserConversation list where each UserConversation has both participants. That way, the storage layer completely encapsulates the separator logic.
2- The service layer owns the separator logic. In this case, we should still pass both usernames and have them stored in the database because the storage layer doesn't know that the conversation id is a concatenations of usernames.

You current solution is a mix of the two approaches above, which is problematic because the logic is distributed in multiple places and the storage layer depends on the service layer indirectly (and vise versa).

nehmebilal · 2023-04-04T13:29:45Z

+                recipientUsername = usernames[0];
+            }
+
+            Profile recipientProfile = await _profileService.GetProfile(recipientUsername);


every time you have multiple await calls, you should consider doing them in parallel if possible

nehmebilal · 2023-04-04T13:40:55Z

+                partitionKey: new PartitionKey(username),
+                new ItemRequestOptions
+                {
+                    ConsistencyLevel = ConsistencyLevel.Session


move this to a private field instead of repeating every time, to make it easier to change the consistency level if needed (instead of having to change in multiple places).

nehmebilal · 2023-04-04T13:41:22Z

+        try
+        {
+            IQueryable<UserConversationEntity> query = Container
+                .GetItemLinqQueryable<UserConversationEntity>(false, continuationToken, options)


named parameter to show what the false is

nehmebilal · 2023-04-04T13:44:48Z

+{
+    Task AddMessage(string conversationId, Message message);
+    Task<Message> GetMessage(string conversationId, string messageId);
+    Task<(List<Message> Messages, string NextContinuationToken)> GetMessages(


This is what mine looks like:

Task<GetMessagesResult> GetMessages(string conversationId, GetMessagesParameters parameters);

this works too:

Task<GetMessagesResult> GetMessages(GetMessagesParameters parameters);

Your signature is a bit too verbose.

nehmebilal · 2023-04-04T13:47:40Z

+public interface IProfileStore
+{
+    Task AddProfile(Profile profile);
+    Task<Profile?> GetProfile(string username);


I noticed that in other store classes your throwing an exception if the entity doesn't exist. You may want to choose one approach instead of mixing. Both are fine but nullable is a bit cleaner in this case.

nehmebilal · 2023-04-04T13:48:15Z

+{
+    Task CreateUserConversation(UserConversation userConversation);
+    Task<UserConversation> GetUserConversation(string username, string conversationId);
+    Task<(List<UserConversation> UserConversations, string NextContinuationToken)> GetUserConversations(


Similar to messages, this signature can be simplified

nehmebilal · 2023-04-04T13:48:52Z

@@ -0,0 +1,14 @@
+namespace ChatService.Web.Utilities;
+
+public class ConversationIdUtilities


This class should be also responsible of splitting and should be the only class aware of the "_" separator

nehmebilal

Alright enough comments for this one :)
Looks pretty good overall, good job! Please make sure to address the comments and apply them to other similar cases in the code. I will review the changes in your next PR.

nadimakk and others added 30 commits February 18, 2023 17:00

implement profiles APIs + tests

9166587

start of images implementation

6867305

Implement ImageController & BlobImageStore

106d166

Implement ImageControllerTests & Perform Minor Changes

7a90a8a

Create ImageDto & Modify ImageController and BlobImageStore accordingly

a68c064

add checks for validity of profile pic, add delete image when profile…

f456588

… deleted, completed tests

Change ImageDto to Image

2a85615

Modify CosmosProfileStoreTests Constructor to handle dependencies inj…

d82da04

…ection

use response from DownloadToAsync to avoid calling Blob storage

569d4b3

add tests for ImageExists from BlobStorage

62ebf48

Create ProfileService with Tests

dfe4f8e

Edit relevant classes and tests to incorporate ProfileService

b37371f

Create custom exceptions

2d9b7b4

Create prbuild.yml

7cf6206

Create azure_deploy.yml

17405d3

Update prbuild.yml

4454fe3

Merge pull request #6 from nadimakk/Lab3

60e144f

Lab3 to Main

Edit workflow azure_deploy.yml

2945732

Run tests without building again.

Edit workflow prbuild.yml

58c53ca

Run tests without building again.

Update workflow azure_deploy.yml

c4b52e9

Included connection strings in environment of integration tests to remove them from app_settings.

Remove connection strings from appsettings

335e335

Update prbuild.yml

a7decb1

rename Blob connection string name

270dea7

fix BlobStorage name in Program.cs

6623f7c

Merge pull request #8 from nadimakk/Lab3

5bd108e

Merge Lab3 to Main

structure and begin storage layer

dd5f28c

Modify database entities & update ComosConversationStore

c0b3947

complete conversation store and tests

d6152f4

Implement CosmosMessageStore

233bf11

begin conversation service

5c1623c

nehmebilal reviewed Apr 4, 2023

View reviewed changes

nehmebilal approved these changes Apr 4, 2023

View reviewed changes

		await _imageService.DeleteImage(profile.ProfilePictureId);
		await _profileStore.DeleteProfile(username);


		public class UserConversationService : IUserConversationService
		{

		@@ -0,0 +1,14 @@
		namespace ChatService.Web.Utilities;

		public class ConversationIdUtilities

Conversation

nadimakk commented Apr 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nehmebilal Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nehmebilal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nehmebilal Apr 4, 2023 •

edited

Loading