Fix various edge cases in the unlocalized body calculation #1924
Conversation
Otherwise we were skipping the emoji tests without crypto set up and similar, which was not intentional.
It is not clear why we ever would want to return the formatted_body when we ask for the body, but it seems to not be used anywhere and there are no tests covering that functionality. However it leads to suprising results, where the plaintextBody can be tricked into returning html without applying conversions. So we just get rid of that functionality.
Covers a few edge cases that still fail. Changes to the unlocalizedBody function shouldn't cause behavioural changes apart from fixing a few edge cases.
We used to randomly return an empty string when the formatted body was empty, even though we never return an empty string usually. Similarly we used to return the original formatted body in an edit, when the new event has no formatted body.
Fixes some noise around logs when I was trying to enable branch coverage.
| ); | ||
| testUnlocalizedBody( | ||
| // not sure we actually want m.room.message here and not an empty string | ||
| expectation: 'm.room.message', |
There was a problem hiding this comment.
I would say empty string for this
There was a problem hiding this comment.
Well, that is historical behaviour. At least some events don't have a body, how do you localize them? All of them just as empty?
There was a problem hiding this comment.
localization could just use a the event type if the body is empty, inserting the type when the body is empty is very confusing imo
There was a problem hiding this comment.
We can change that, but that would be something @krille-chan signs off on, imo.
| editHtml: false, | ||
| ); | ||
| testUnlocalizedBody( | ||
| expectation: 'm.room.message', |
| testUnlocalizedBody( | ||
| expectation: 'm.room.message', | ||
| plaintextBody: false, | ||
| body: 5, |
There was a problem hiding this comment.
I would expect a '5' here, I thought this was already the case?
There was a problem hiding this comment.
Why? A number is not a string, so it falls back to using the same logic as if the body was missing?
There was a problem hiding this comment.
Yes but we could be more flexible here? I don't see why not just show the int content if we can
There was a problem hiding this comment.
Because that case should never happen, we want a string and it could be arbitrarily broken. The number here is just a placeholder, but in general someone could put a whole json document in there.
| expectation: 'body', | ||
| plaintextBody: true, | ||
| body: 'body', | ||
| formattedBody: 5, |
There was a problem hiding this comment.
again would expect a '5' here
| formattedBody: '<b>formatted body</b>', | ||
| html: true, | ||
| isEdit: true, | ||
| editBody: null, |
There was a problem hiding this comment.
same as above, would expect editBody to be shown
| expectation: '**formatted body**', | ||
| plaintextBody: true, | ||
| body: 'body', | ||
| formattedBody: '<b>formatted body</b>', |
There was a problem hiding this comment.
same here, would expect editBody
There was a problem hiding this comment.
Edit body is null, which is an invalid value, so it falls back to using the event formatted body. So you expect an empty string here? Even though the newContent is missing?
There was a problem hiding this comment.
So you expect an empty string here?
yep, falling back to the old event sounds bad
There was a problem hiding this comment.
It's not the old event, it is the fallback body in the new event, it just usually has a weird * in front of it or so.
| body: 'body', | ||
| formattedBody: '<b>formatted body</b>', | ||
| html: true, | ||
| isEdit: true, |
| html: true, | ||
| isEdit: true, | ||
| editBody: 'edit body', | ||
| editFormattedBody: '<b>edit formatted body</b>', |
There was a problem hiding this comment.
hm shouldn't this return the edited formatted body?
There was a problem hiding this comment.
No, because it has plaintextBody set to false.
| editHtml: true, | ||
| ); | ||
| testUnlocalizedBody( | ||
| expectation: 'edit body', |
See the additional test cases. There are still some weird edge cases, but those are documented in the tests now.