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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct the dir parameter of MSC3715 #2745

Conversation

dhenneke
Copy link
Contributor

@dhenneke dhenneke commented Oct 10, 2022

The implementation of MSC3715 was stabilized in synapse and we might want to use the new parameter. Note that the old name direction was always wrong and never worked so I don't think we need the fallback to the prefixed version. This change would also need a PR at the react-sdk but there only seems to be a single usage. If you accept this change I can do a second PR to update it. I assume merging would need to be coordinated to not break the build(?).

An alternative could be to keep the name direction in the interface and convert it to dir when adding it to the query params. That wouldn't require a potential breaking change and a change in a downstream project. Let me know if you would prefer that solution.

I added some primitive tests that only focus on the HTTP calls and not on the actual behavior of the relations function.

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

馃悰 Bug Fixes

@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Oct 10, 2022
Signed-off-by: Dominik Henneke <dominik.henneke@nordeck.net>
@dhenneke dhenneke force-pushed the nic/feat/fix-relation-direction-parameter branch from 79f3e2b to a75e5b7 Compare October 10, 2022 14:13
@dhenneke dhenneke marked this pull request as ready for review October 10, 2022 14:22
@dhenneke dhenneke requested a review from a team as a code owner October 10, 2022 14:22
@SimonBrandner SimonBrandner added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Oct 11, 2022
Copy link
Contributor

@SimonBrandner SimonBrandner left a comment

Choose a reason for hiding this comment

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

LGTM - thank you!

@SimonBrandner
Copy link
Contributor

SonarCloud seems to require a bit more tests for the MatrixClient class - if you could please add those, it would be awesome!

Signed-off-by: Dominik Henneke <dominik.henneke@nordeck.net>
auto-merge was automatically disabled October 12, 2022 07:09

Head branch was pushed to by a user without write access

@dhenneke
Copy link
Contributor Author

I added a test and hope SonarCloud is satisfied with that.

I also created the PR on the react-sdk: matrix-org/matrix-react-sdk#9391

@SimonBrandner SimonBrandner merged commit 913660c into matrix-org:develop Oct 12, 2022
@dhenneke dhenneke deleted the nic/feat/fix-relation-direction-parameter branch October 13, 2022 07:24
su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this pull request Oct 29, 2022
* Changes the `uploadContent` API, kills off `request` and `browser-request` in favour of `fetch`, removed callback support on a lot of the methods, adds a lot of tests. ([\matrix-org#2719](matrix-org#2719)). Fixes matrix-org#2415 and matrix-org#801.
* Remove deprecated `m.room.aliases` references ([\matrix-org#2759](matrix-org#2759)). Fixes element-hq/element-web#12680.
* Remove node-specific crypto bits, use Node 16's WebCrypto ([\matrix-org#2762](matrix-org#2762)). Fixes matrix-org#2760.
* Export types for MatrixEvent and Room emitted events, and make event handler map types stricter ([\matrix-org#2750](matrix-org#2750)). Contributed by @stas-demydiuk.
* Use even more stable calls to `/room_keys` ([\matrix-org#2746](matrix-org#2746)).
* Upgrade to Olm 3.2.13 which has been repackaged to support Node 18 ([\matrix-org#2744](matrix-org#2744)).
* Fix `power_level_content_override` type ([\matrix-org#2741](matrix-org#2741)).
* Add custom notification handling for MSC3401 call events  ([\matrix-org#2720](matrix-org#2720)).
* Add support for unread thread notifications ([\matrix-org#2726](matrix-org#2726)).
* Load Thread List with server-side assistance (MSC3856) ([\matrix-org#2602](matrix-org#2602)).
* Use stable calls to `/room_keys` ([\matrix-org#2729](matrix-org#2729)). Fixes element-hq/element-web#22839.
* Fix POST data not being passed for registerWithIdentityServer ([\matrix-org#2769](matrix-org#2769)). Fixes matrix-org/element-web-rageshakes#16206.
* Fix IdentityPrefix.V2 containing spurious `/api` ([\matrix-org#2761](matrix-org#2761)). Fixes element-hq/element-web#23505.
* Always send back an httpStatus property if one is known ([\matrix-org#2753](matrix-org#2753)).
* Check for AbortError, not any generic connection error, to avoid tightlooping ([\matrix-org#2752](matrix-org#2752)).
* Correct the dir parameter of MSC3715 ([\matrix-org#2745](matrix-org#2745)). Contributed by @dhenneke.
* Fix sync init when thread unread notif is not supported ([\matrix-org#2739](matrix-org#2739)). Fixes element-hq/element-web#23435.
* Use the correct sender key when checking shared secret ([\matrix-org#2730](matrix-org#2730)). Fixes element-hq/element-web#23374.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants