Add message field to quote model #14
Conversation
| The quoted message | ||
|
|
||
| This should always be provided if a message ID is specified, unless the | ||
| app cannot find the message in its database. |
There was a problem hiding this comment.
Suggest to add:
This MUST NOT be a message containing a quote!
There was a problem hiding this comment.
As long as quotes aren't resolved recursively, there should be little harm in doing so. I think it's better if Threema Web is written in a robust way that can handle this, versus having to trust that the app doesn't have any validation bug.
There was a problem hiding this comment.
I disagree. It is not allowed to have a quote within a quote. Furthermore, a MUST does not make validation redundant. On the contrary, it makes the reader aware of it.
There was a problem hiding this comment.
I also think the recursive resolving is a very easy mistake to make.
|
Also, have you checked for side effects? This model is being used throughout a couple of other messages and I'm not sure it makes a lot of sense in all of them. |
What type of side effects would you expect? All new fields are optional, so the protocol change itself will not break anything, unless it's explicitly handled. You're right though that creating a quote and retrieving a quote could use separate types ( |
|
@lgrahl I added some additional notes regarding validation. |
The message needs to be embedded, since the webclient might not have received that message yet.