ls1intum / Artemis

Artemis - Interactive Learning with Automated Feedback
https://docs.artemis.cit.tum.de
MIT License
487 stars 288 forks source link

`Communications`: Client does not properly keep track of total number of posts on page #8985

Closed PaRangger closed 2 months ago

PaRangger commented 3 months ago

Describe the bug

TLDR:

The "envelope" icon in an empty conversation does not dissapear on leaving messages until you reload the page.

Full Description:

The metis.service.ts has a local variable called cachedTotalNumberOfPots (I guess it is a typo and should be Posts instead of Pots), and the variable is used to keep track of the unpaginated number of posts of the currently selected conversation.

The variable is updated only within the getFilteredPosts function of the service, if a certain set of conditions is met. This if condition instructs the function to force reload all posts and then update the cachedTotalNumberOfPots variable. On the creation/deletion of a new post, or when the websocket reports that another user left a new post on the current conversation, this function does not force reload all posts, presumably for performance reasons. However, this means that the cachedTotalNumberOfPots does not get updated in those cases. Which could be fine, since the variable is the cached total number of posts.

However, the variable is reported on any change in the conversation (creation, deletion, etc of a post) as the totalNumberOfPosts observable. This leads to unintended behaviour, when implementing checks on the number of posts, because the actual total number of posts is different than what the variable says. An instance of a problem is, that the "envelope" icon in a conversation without messages does not disappear when a user sends the first message. This is due to the component checking for totalNumberOfPosts === 0. The cached totalNumberOfPosts is 0, however the totalNumberOfPosts should be 1. If you reload the page, the icon dissapears, because the variable is now set to the proper amount of posts. I can imagine further issues arising due to confusion within the meaning of variable.

To Reproduce

  1. Log into artemis
  2. Go to a course that has communications enabled.
  3. Go to the "Communication" tab.
  4. Go to a conversation without any messages.
  5. Write the first message.

Expected behavior

The totalNumberOfPosts observable should either deliver the real amount of posts or be renamed to cachedTotalNumberOfPosts. Either way, for the client to properly react to the amount of posts, the service needs to either keep track of the real amount via the backend, or, in case of the envelope, atleast keep track of the number of posts that are visible on the screen.

Screenshots

Bildschirmfoto 2024-07-07 um 14 32 25

Which version of Artemis are you seeing the problem on?

7.4.2

What browsers are you seeing the problem on?

Chrome, Safari

Additional context

No response

Relevant log output

No response

PaRangger commented 3 months ago

I have 2 ideas on how to resolve this issue:

  1. Determine and return the number of posts on all create/delete calls in the "X-Total-Count" header and safe the value into cachedTotalNumberOfPots. This however would probably increase the load on the server side, since it always has to query the amount of posts in these operations, in order to attach it to the response.
  2. Rename the observable for better clarity, and in the case of the "envelope" icon listen to the .length of the posts observable. Since that is the posts that are actually visible on screen.

Personally, I prefer option 2 since it is much less efford and does not introduce any extra load on the server. While option 1 seems to yield better possibilities for future developments, since the amount will always be up-to-date, thus improving information quality, I don't think it is worth the effort, since i cannot imagine a scenario where the correctness of the number will have a significant impact on the user. I will open a branch to implement option 2 for now in order to fix the issue. But I am open to other ideas. I'll also make sure to fix the typo 😄