MessagesAction Update to also handle non-actionable messages without failing #168
Conversation
|
I would like to discuss this further before merging, since there seem to be quite a few UI differences between messages on iOS and Android and I would rather discuss them before throwing out existing functionality here. Examples:
|
| @@ -14,7 +14,7 @@ class MessageActionMapper @Inject constructor() { | |||
| fun map(action: String?): Result<MessagesAction> { | |||
| return runCatching { | |||
There was a problem hiding this comment.
there is another error being thrown in the else case. I would propose changing the return type of this method to MessageAction? instead of result and update to runCatching { // exisiting logic }.getOrNull(). No need for NoAction imo as we can represent it with the null case
There was a problem hiding this comment.
Changed it accordingly
| @@ -15,7 +15,6 @@ data class Message( | |||
| val description: String? = null, | |||
| val action: String?, | |||
| val isDismissible: Boolean = true, | |||
| val isLoading: Boolean = false, | |||
There was a problem hiding this comment.
There is some logic in HomeViewModel that shows a loading indicator while a message is being processed, e.g. health summary pdf generation. I think we still need this
There was a problem hiding this comment.
I found this out now, yes - but I believe this should actually be in some wrapper type or something, since it has nothing to do with the model itself. Like this, I would assume that the server sends us this information, which is not the case.
There was a problem hiding this comment.
Totally agree, in general there should be distinct types for domain and ui layer, e.g. Message and MessageUiModel and currently the message is serving for both layers
| @@ -30,6 +30,7 @@ class MessagesHandler @Inject constructor( | |||
| val actionResult = messagesActionMapper.map(action = message.action) | |||
| var failure = actionResult.exceptionOrNull() | |||
| when (val messagesAction = actionResult.getOrNull()) { | |||
| is MessagesAction.NoAction -> Unit | |||
There was a problem hiding this comment.
it should not be needed as it is covered in the else case (applies also in case you change the response to nullable instead of result)
Question: Should we complete the message below if we could not map any action for it?
There was a problem hiding this comment.
I'm not sure actually, would need to check iOS
There was a problem hiding this comment.
I checked: iOS only calls dismissMessage, if an action has been performed and the message has isDismissible set to true.
|
@pauljohanneskraft Thank you for looking into this and thanks for the first feedback @eldcn! Would be great to reduce the disparities between the different platforms.
I think it could be fine to never collapse a message. We might still want to add a truncation mark after e.g. 10 lines to ensure that a long text or a bug doesn't break the UI?
I like the way iOS is currently handling this. An X for any dismissible actions and a action button that is bound to the message type & let's you perform a message.
Agree, we can remove that.
Good point. @eldcn & @pauljohanneskraft let me know if you need any additional context; happy to also sync on this on Tuesday if there are more open questions. |
…when a new message comes in
| data object HealthSummaryAction : MessageAction | ||
| } | ||
|
|
||
| data class Video(val sectionId: String, val videoId: String) |
There was a problem hiding this comment.
If this is used in context of message action only, I would be more in favor of simply declaring the properties in video action type instead of introducing this new type
There was a problem hiding this comment.
sounds good to me honestly
| val bluetooth: BluetoothUiState = BluetoothUiState.Idle(), | ||
| val measurementDialog: MeasurementDialogUiState = MeasurementDialogUiState(), | ||
| ) | ||
|
|
||
| data class MessageUiModel( | ||
| val message: Message, |
There was a problem hiding this comment.
Wrapping the domain message in a ui model still violates why we discussed. Ideally the ui model should remain domain agnostic and only contain properties relevant for UI. It should be created in a UI mapper, e.g.
data class MessageUiModel(
val id: String,
val title: String,
val description: String?,
val dueDate: String?,
val isExpanded: Boolean = false,
val isLoading: Boolean = false,
val icon: Int,
)
fun map(message: Message): MessageUiModel {
return MessageUiModel(
id = message.id,
title = message.title,
description = message.description,
dueDate = message.dueDate?.format(DateTimeFormatter.ofPattern("MMM dd, yyyy hh:mm a")),
isExpanded = false,
isLoading = false,
icon = when (message.action) {
is MessageAction.MedicationsAction -> R.drawable.ic_medication
is MessageAction.MeasurementsAction -> R.drawable.ic_vital_signs
is MessageAction.QuestionnaireAction -> R.drawable.ic_assignment
is MessageAction.VideoAction -> R.drawable.ic_visibility
is MessageAction.UnknownAction -> R.drawable.ic_assignment
is MessageAction.HealthSummaryAction -> R.drawable.ic_vital_signs
},
)
}There was a problem hiding this comment.
fine with this change as well, will do
There was a problem hiding this comment.
mhmmm, okay, so I tried this, but usually when we interact with lower layers (e.g. when dismissing a message or handling an action based on a message), we would usually want the message actually... How would you usually solve this? Mapping back seems to not really solve any issues, since either data is lost or we have an implicit coupling (i.e. we would need to copy the other object property-by-property anyways)
After seeing this, I'm actually more in favor of wrapping the model type in the UI-Model, but open to discussions. @eldcn
There was a problem hiding this comment.
The lower layer does not need all the message props, but only the id, action and whether it is dismissable, similarly as you adapted the handler api to process firebase notifications. I see here two ways:
- Store additionally
messageActionandisDismissableinMessageUiModelsince those are additionally needed for handling the message or - Cache the loaded messages list in a private property in the view model on every update before mapping. When having to handle a message due to
Action.MessageItemClicked, you can query the cached list for the message id and hand over to the handler
|
@pauljohanneskraft Double-check that this resolves #157 & #159 |
MessagesAction Update to also handle non-actionable messages without failing
♻️ Current situation & Problem
Previously, the app would handle a non-existing action as a failure, whereas it can simply be ignored and a message should then either be dismissible or will be dismissed automatically based on other user input.
⚙️ Release Notes
Add a bullet point list summary of the feature and possible migration guides if this is a breaking change so this section can be added to the release notes.
Include code snippets that provide examples of the feature implemented or links to the documentation if it appends or changes the public interface.
📚 Documentation
Please ensure that you properly document any additions in conformance to Spezi Documentation Guide.
You can use this section to describe your solution, but we encourage contributors to document your reasoning and changes using in-line documentation.
✅ Testing
Please ensure that the PR meets the testing requirements set by CodeCov and that new functionality is appropriately tested.
This section describes important information about the tests and why some elements might not be testable.
📝 Code of Conduct & Contributing Guidelines
By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: