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

Improve compliance with MSC3266 #4155

Merged
merged 15 commits into from
Apr 22, 2024

Conversation

AndrewFerr
Copy link
Member

@AndrewFerr AndrewFerr commented Apr 11, 2024

This fixes some non-compliance with MSC 3266, and exports the payload type.

Signed-off-by: Andrew Ferrazzutti andrewf@element.io

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

Also remove redundant room_type field which is inherited from elsewhere
Use the endpoint recommended by the MSC
@AndrewFerr AndrewFerr requested a review from a team as a code owner April 11, 2024 16:25
src/matrix.ts Outdated Show resolved Hide resolved
@t3chguy
Copy link
Member

t3chguy commented Apr 12, 2024

Please improve the PR title given it will be used in the changelog verbatim

Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Looks sane otherwise

@AndrewFerr AndrewFerr changed the title MSC 3266 compliance Improve compliance with MSC3266 Apr 12, 2024
@AndrewFerr
Copy link
Member Author

Please improve the PR title given it will be used in the changelog verbatim

Done -- let me know if the title still needs changing.

I am also willing to add an integration test for this.

@t3chguy
Copy link
Member

t3chguy commented Apr 12, 2024

Yeah looks like CI won't let it in without testing

@AndrewFerr
Copy link
Member Author

Good thing CI is strict, because this PR requires a Synapse change too: element-hq/synapse#17078

@t3chguy
Copy link
Member

t3chguy commented Apr 12, 2024

So we'll likely need fallback code then
for older Synapse support

@AndrewFerr
Copy link
Member Author

So we'll likely need fallback code then for older Synapse support

The Synapse PR has a fallback: it responds to both the old & new endpoint used by getRoomSummary.

Then again, getRoomSummary could also have a fallback: if it gets a 404 response for the new endpoint, it can retry at the old endpoint. I'll add that now.

@AndrewFerr
Copy link
Member Author

Hm, even with the integration test, the code analysis check is failing. What needs to be added to fix that?

Other than that, bd09442 adds the fallback, so once the check is fixed, this should PR be ready.

@t3chguy
Copy link
Member

t3chguy commented Apr 15, 2024

Hm, even with the integration test, the code analysis check is failing

It says you reached 33.3% coverage, out of the required 80%.

image

Things marked red are untested.

Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Looks sane

@AndrewFerr
Copy link
Member Author

Alright, code coverage for new code is now at 100%, so this should be done.

@AndrewFerr
Copy link
Member Author

Do I need to rebase this PR branch before it can be merged?

@toger5 toger5 added this pull request to the merge queue Apr 22, 2024
Merged via the queue into matrix-org:develop with commit e874468 Apr 22, 2024
25 checks passed
@AndrewFerr AndrewFerr deleted the msc-3266-compliance branch April 23, 2024 09:08
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.

3 participants