Skip to content

Conversation

@lennart-keller
Copy link
Contributor

@lennart-keller lennart-keller commented Jul 25, 2023

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) database calls.
  • I followed the coding and design guidelines.
  • I documented the Java code using JavaDoc style.

Motivation and Context

Following #6964 we would like to decrease the payload of the messaging endpoints even further to increase performance. This PR reduces the size of the conversation Object attached to posts.

In a follow-up PR, the conversation object will be set to null completely.

Description

For GET .../messages, the .hideDetails() method from #6964 is applied to the conversation of the returned posts, including the answers. Also for the endpoints to retrieve the channel for an exercise/lecture this method is invoked.

Steps for Testing

Prerequisites:

  • 1 student
  • 1 course with messaging enabled
  1. Open the developer tools inspecting the REST calls
  2. Visit the messages tab and click on a channel to load messages (write some if necessary)
  3. Confirm, that the conversation objects attached to messages in the response contains only minimal information, i.e. no exercise, no lecture, no exam, no course, no conversation participants
  4. The same should apply to the post object attached to answers of a message (write some answers if necessary)
  5. Confirm that you can create/edit/delete/react to messages and also create/edit/delete/react to answers
  6. Repeat step 5 for the lecture and exercise page for a lecture/exercise with an attached channel and look at the /channel responses as well
  7. Repeat step 2-4 for group chats and direct messages

Review Progress

Performance Review

  • I confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance
  • I confirm that the server changes (in particular related to database calls) are implemented with a very good performance

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Server

Class/File Line Coverage Confirmation (assert/expect)
BaseExercise.java 98%
Exercise.java 91%
FileUploadExercise.java 100%
FileUploadSubmission.java 100%
ProgrammingExercise.java 92%
ProgrammingSubmission.java 100%
TextExercise.java 100%
TextSubmission.java 98%
ModelingExercise.java 93%
ModelingSubmission.java 92%
DragAndDropQuestion.java 84%
DragAndDropQuestionStatistic.java 98%
DragAndDropSubmittedAnswer.java 91%
MultipleChoiceQuestion.java 88%
MultipleChoiceQuestionStatistic.java 99%
MultipleChoiceSubmittedAnswer.java 93%
QuizExercise.java 90%
QuizQuestion.java 96%
QuizSubmission.java 100%
ShortAnswerQuestion.java 87%
ShortAnswerQuestionStatistic.java 99%
ShortAnswerSubmittedAnswer.java 87%
ConversationMessageResource.java 97%

@lennart-keller lennart-keller self-assigned this Jul 25, 2023
@lennart-keller lennart-keller requested a review from a team July 25, 2023 20:17
@github-actions github-actions bot added the server Pull requests that update Java code. (Added Automatically!) label Jul 25, 2023
@krusche krusche added this to the 6.3.8 milestone Jul 25, 2023
krusche
krusche previously approved these changes Jul 26, 2023
TimOrtel
TimOrtel previously approved these changes Jul 26, 2023
Copy link
Contributor

@TimOrtel TimOrtel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Very nice improvement.

@github-actions

This comment was marked as outdated.

@github-actions github-actions bot added the deployment-error Added by deployment workflows if an error occured label Jul 26, 2023
@Strohgelaender Strohgelaender added deploy:artemis-test5 and removed deployment-error Added by deployment workflows if an error occured labels Jul 26, 2023
@Strohgelaender Strohgelaender temporarily deployed to artemis-test5.artemis.cit.tum.de July 26, 2023 09:27 — with GitHub Actions Inactive
tobias-lippert
tobias-lippert previously approved these changes Jul 26, 2023
Copy link
Contributor

@tobias-lippert tobias-lippert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on ts5. Sending messages is still possible and the payload seems to be almost minimal.
I only noticed that the response for /conversations contains the type property twice for each conversation and I don't know if this is caused by this PR and can lead to problems.

@lennart-keller
Copy link
Contributor Author

Tested on ts5. Sending messages is still possible and the payload seems to be almost minimal.
I only noticed that the response for /conversations contains the type property twice for each conversation and I don't know if this is caused by this PR and can lead to problems.

The duplicate type property is not introduced by this PR

@lennart-keller lennart-keller requested a review from TimOrtel July 26, 2023 11:09
Copy link
Contributor

@Strohgelaender Strohgelaender left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code

@tobias-lippert tobias-lippert temporarily deployed to artemis-test5.artemis.cit.tum.de July 26, 2023 11:39 — with GitHub Actions Inactive
Copy link
Contributor

@tobias-lippert tobias-lippert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on ts5. Still works after removing more content from the response body.

Copy link
Contributor

@RY997 RY997 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on ts5, the number of REST calls looks good for me

Copy link
Contributor

@laadvo laadvo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested on ts1, the response payload is now a lot smaller. Writing messages, replies and reacting to them still works

@krusche krusche merged commit 579bdf6 into develop Jul 26, 2023
@krusche krusche deleted the chore/minimize-messages-response branch July 26, 2023 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority:high ready to merge server Pull requests that update Java code. (Added Automatically!) tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants