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
| @@ -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.
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: