Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for HTML renderings of room topics #2272

Merged
merged 15 commits into from
May 16, 2022

Conversation

Johennes
Copy link
Contributor

@Johennes Johennes commented Apr 2, 2022

For matrix-org/matrix-react-sdk#8215
MSC: matrix-org/matrix-spec-proposals#3765

Relates to element-hq/element-web#5180

This introduces m.topic as an extension in m.room.topic to allow for both plain text and HTML rendering of room topics.

Eventually most of the logic should be pushed down into matrix-events-sdk (see matrix-org/matrix-events-sdk#5). However, in order not to complicate future architectural changes of this project, @turt2live advised to put the code here for now.


Here's what your changelog entry will look like:

✨ Features

  • Add support for HTML renderings of room topics (#2272). Contributed by @Johennes.

Based on extensible events as defined in [MSC1767]

Relates to: element-hq/element-web#5180
Signed-off-by: Johannes Marbach <johannesm@element.io>

[MSC1767]: matrix-org/matrix-spec-proposals#1767
@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2022

Codecov Report

Merging #2272 (dac3058) into develop (db58a66) will increase coverage by 0.00%.
The diff coverage is 64.70%.

❗ Current head dac3058 differs from pull request most recent head 98dc106. Consider uploading reports for the commit 98dc106 to get more accurate results

@@           Coverage Diff            @@
##           develop    #2272   +/-   ##
========================================
  Coverage    59.75%   59.75%           
========================================
  Files           91       92    +1     
  Lines        16450    16455    +5     
  Branches      3796     3799    +3     
========================================
+ Hits          9829     9833    +4     
- Misses        6621     6622    +1     
Impacted Files Coverage Δ
src/client.ts 38.04% <0.00%> (-0.10%) ⬇️
src/@types/topic.ts 100.00% <100.00%> (ø)
src/content-helpers.ts 83.33% <100.00%> (+3.33%) ⬆️
src/crypto/index.ts 64.32% <0.00%> (-0.11%) ⬇️

@Johennes Johennes marked this pull request as ready for review April 3, 2022 12:46
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this side lgtm

src/client.ts Outdated Show resolved Hide resolved
@Johennes
Copy link
Contributor Author

Could I get a pass with "only" 73% coverage? I think I've covered the meaningful bits with tests.

@t3chguy
Copy link
Member

t3chguy commented May 11, 2022

The gate being 80% is actually considered low, James wanted the gate for the js-sdk to be 100% given it is a reusable SDK. The bits in topic.ts are fully covered, content-helpers is lacking one line and client.ts is at 59.3%.

https://sonarcloud.io/code?id=matrix-js-sdk&pullRequest=2272&selected=matrix-js-sdk%3Asrc%2Fcontent-helpers.ts&line=217
setRoomTopic looks entirely untested? https://sonarcloud.io/code?id=matrix-js-sdk&pullRequest=2272&selected=matrix-js-sdk%3Asrc%2Fclient.ts&line=3573

If you cover either of the above you should hit the gate. Are there any parts here which are infeasible to cover for some reason?

@t3chguy
Copy link
Member

t3chguy commented May 11, 2022

Could you also please pull latest develop, some of the checks aren't running seemingly

@Johennes
Copy link
Contributor Author

Are there any parts here which are infeasible to cover for some reason?

I suppose not infeasible, no. The main untested chunk setRoomTopic just seemed like it might not be worth covering because it mainly just calls sendStateEvent. I see the rationale for the coverage threshold though so let me see what I can do to meet it.

Could you also please pull latest develop, some of the checks aren't running seemingly

Hm, I had merged in develop just last night. 🤔 I'll merge it again when working on the tests. Hopefully that fixes it.

@t3chguy
Copy link
Member

t3chguy commented May 12, 2022

The main untested chunk setRoomTopic just seemed like it might not be worth covering because it mainly just calls sendStateEvent.

The added backwards compatibility overload is certainly worth testing, those are super fragile.

@t3chguy
Copy link
Member

t3chguy commented May 12, 2022

(Adding the label re-ran the missing CI job)

@Johennes
Copy link
Contributor Author

Hm, something's wrong. I've added tests for setRoomTopic and it's now fully covered. However, SonarCloud still rejects the PR with

74.6% Coverage on New Code (is less than 80%)

When I click through, it lists a lot of uncovered lines in "new" code for files that I haven't actually touched, like 81 lines in room.ts.

@t3chguy any idea what's wrong here?

@turt2live
Copy link
Member

I wonder if it just doesn't understand merge commits...

@Johennes Johennes requested a review from t3chguy May 13, 2022 11:31
@t3chguy
Copy link
Member

t3chguy commented May 13, 2022

Not sure what happened here - #2366 was unaffected, showing 81.6% covg before & after the merge commit

@t3chguy
Copy link
Member

t3chguy commented May 16, 2022

image

Wonder if something has gone a bit strange on this branch given gh can't even rebase it

@t3chguy
Copy link
Member

t3chguy commented May 16, 2022

Skipping Sonar check due to weirdness, coverage is appropriate already

@t3chguy t3chguy merged commit f44510e into matrix-org:develop May 16, 2022
@Johennes
Copy link
Contributor Author

Thanks @t3chguy!

su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this pull request May 28, 2022
* Implement changes to MSC2285 (private read receipts) ([\matrix-org#2221](matrix-org#2221)).
* Add support for HTML renderings of room topics ([\matrix-org#2272](matrix-org#2272)).
* Add stopClient parameter to MatrixClient::logout ([\matrix-org#2367](matrix-org#2367)).
* registration: add function to re-request email token ([\matrix-org#2357](matrix-org#2357)).
* Remove hacky custom status feature ([\matrix-org#2350](matrix-org#2350)).
* Remove default push rule override for MSC1930 ([\matrix-org#2376](matrix-org#2376)). Fixes element-hq/element-web#15439.
* Tweak thread creation & event adding to fix bugs around relations ([\matrix-org#2369](matrix-org#2369)). Fixes element-hq/element-web#22162 and element-hq/element-web#22180.
* Prune both clear & wire content on redaction ([\matrix-org#2346](matrix-org#2346)). Fixes element-hq/element-web#21929.
* MSC3786: Add a default push rule to ignore `m.room.server_acl` events ([\matrix-org#2333](matrix-org#2333)). Fixes element-hq/element-web#20788.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants