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

registration: add function to re-request email token #2357

Merged

Conversation

justjanne
Copy link
Contributor

@justjanne justjanne commented May 10, 2022

Type: Enhancement
Related: element-hq/element-web#21984
Related: matrix-org/matrix-react-sdk#8554


Here's what your changelog entry will look like:

✨ Features

  • registration: add function to re-request email token (#2357). Contributed by @justjanne.

@justjanne justjanne force-pushed the justjanne/feat/21984-registration-email-verification branch from 55445ed to 221776f Compare May 11, 2022 08:32
@justjanne justjanne force-pushed the justjanne/feat/21984-registration-email-verification branch from 221776f to 58a0a7e Compare May 11, 2022 10:07
@justjanne justjanne marked this pull request as ready for review May 11, 2022 10:07
@justjanne justjanne requested a review from a team as a code owner May 11, 2022 10:07
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, but as pointed out by Sonar, does not hit the required mark of 80% coverage on new code.

/**
* Requests a new email token and sets the email sid for the validation session
*/
public requestEmailToken = async () => {
Copy link
Member

Choose a reason for hiding this comment

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

out of interest is there any reason this is an arrow func?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to use this in the function, and while I personally would prefer to use a regular function and binding it in the constructor, across the codebase I’ve found arrow functions to be used more commonly to accomplish the same.

Copy link
Member

Choose a reason for hiding this comment

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

Sure but the function is always called with the correct this context already (L493) so would work in either way. Its not being copied to lose its bound context anywhere yet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it is? It’s a public function, so it can be called with any this context. And it is called like with a different context as onClick handler in matrix-org/matrix-react-sdk#8554.

Copy link
Member

Choose a reason for hiding this comment

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

The SDK as a whole makes the assumption that the caller will always call with the correct this context, we tend to use arrow funcs for when we want to use it as a local handler. Arrow functions here are not wrong by any means, was just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that’s an interesting assumption! Good to know, I’ve never seen a codebase that assumed this before.

Copy link
Member

Choose a reason for hiding this comment

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

It is very much a bad assumption to have, but one we make anyway. If you feel strongly about it we could make a rule that all public methods have to be arrow functions to remedy this

@justjanne justjanne requested a review from t3chguy May 11, 2022 11:15
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.

🥳 Thanks for being the first to brave the better tested new world order!

@justjanne
Copy link
Contributor Author

justjanne commented May 11, 2022

Sadly I couldn’t migrate the tests to typescript cause the other tests do some type trickery :(

@justjanne justjanne merged commit 923ff4b into develop May 11, 2022
@justjanne justjanne deleted the justjanne/feat/21984-registration-email-verification branch May 11, 2022 11:17
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.

2 participants