Feature: Show forwarded messages
#448
Conversation
julian-wls
added
the
database changes
julian-wls
added
the
api calls
...formatics/www1/artemis/native_app/feature/metis/conversation/service/network/MetisService.kt
Show resolved
Hide resolved
| Box( | ||
| modifier = Modifier | ||
| .clip(shape = CircleShape) | ||
| .fillMaxHeight() |
There was a problem hiding this comment.
I think we use exactly the same border also for LinkPreviews, right? If so, we could create a composable for this. It would also improve the readability :D
...rmatics/www1/artemis/native_app/feature/metis/conversation/ui/post/ForwardedMessageColumn.kt
Show resolved
Hide resolved
...rmatics/www1/artemis/native_app/feature/metis/conversation/ui/post/ForwardedMessageColumn.kt
Show resolved
Hide resolved
| PostItemMainContent( | ||
| modifier = Modifier, | ||
| modifier = Modifier.wrapContentHeight(unbounded = true), |
There was a problem hiding this comment.
Out of curiosity, can you explain why wrapContentHeight is needed here?
| val id = | ||
| cachedForwardedMessages.find { it.destinationPostId == chatListItem.post.serverPostId }?.sourceId | ||
| forwardedSourcePosts = forwardedSourcePosts + (cachedStandaloneSourcePosts[id] ?: emptyList()) | ||
| forwardedSourcePosts = forwardedSourcePosts + (cachedAnswerSourcePosts[id] ?: emptyList()) |
There was a problem hiding this comment.
I think we could make the list mutable and make use of the forwardedSourcePosts += syntax here
| * and stored in cachedStandardsSourcePosts and cachedAnswerSourcePosts. | ||
| * Common usage: 1. Extract forwarded messages from a list of posts in [extractForwardedMessages]. | ||
| * 2. Load the forwarded messages for the extracted ids and fetch their source posts in [loadForwardedMessages]. | ||
| * 3. Match the previously loaded source posts to the destination posts in [resolveForwardedMessages]. |
There was a problem hiding this comment.
We could improve the readability of this class by ordering the method definitions in the ordered they are supposed to be called :D
| @@ -117,6 +117,7 @@ internal fun ConversationChatListScreen( | |||
|
|
|||
| LaunchedEffect(courseId, conversationId) { | |||
| chatListState.scrollToItem(0) | |||
| viewModel.onCloseThread = null | |||
There was a problem hiding this comment.
Is this one of the bugfixes mentioned in the PR description? At least from some testing, it looks like it solves #438, so I assigned it accordingly. In case that's wrong, feel free to undo it :D
...informatics/www1/artemis/native_app/feature/metis/conversation/ConversationMessagesUITest.kt
Show resolved
Hide resolved
|
|
||
| interface ForwardedMessage { | ||
| val forwardedPosts: List<IBasePost?> | ||
| val courseId: Long |
There was a problem hiding this comment.
The courseId feels kinda off here, is it only used for creating the inAppLink? If so, I'd suggest to create a new CommunicationDeepLinks.ToConversationCourseAgnostic, similar to eg ToLectureCourseAgnostic.
Also, would it then make sense to remove the interface ForwardedMessage and its implementations, and instead move forwardedPosts to the PostItem and leave it empty for posts without forwarded messages? But maybe I missed something, please let me know :D
Problem Description
Forwarded messages are currently not rendered by the app, and that's why there are sometimes posts with no content in the chat.
Changes
This PR introduces the rendering of forwarded messages by making the required requests and adding logic to correctly match the forwarded posts to their destination posts.
The following changes have been made:
Note:
Steps for testing
Screenshots