Page MenuHomePhabricator

[lib] Join a community on an overridden keyserver
ClosedPublic

Authored by tomek on Feb 8 2024, 5:56 AM.
Tags
None
Referenced Files
F3523458: D10992.id36845.diff
Mon, Dec 23, 9:23 AM
F3522065: D10992.id37095.diff
Mon, Dec 23, 5:39 AM
F3522064: D10992.id36994.diff
Mon, Dec 23, 5:39 AM
F3522063: D10992.id36993.diff
Mon, Dec 23, 5:39 AM
F3522062: D10992.id36991.diff
Mon, Dec 23, 5:39 AM
F3522061: D10992.id36992.diff
Mon, Dec 23, 5:39 AM
F3522060: D10992.id36844.diff
Mon, Dec 23, 5:39 AM
F3522059: D10992.id36845.diff
Mon, Dec 23, 5:39 AM
Subscribers

Details

Summary

In order to join a keyserver we need to make sure that the user is authenticated. To do that, we need to make sure that the keyserver is present in the store and that the keyserver connection handler successfully performed the auth.

The solution is to replace a simple async action with creating a new promise, that is resolved or rejected after a timeout or after a successful or unsuccessful join operation.

Depends on D10991

Test Plan

Check on web that:

  1. An invalid link is displayed as invalid
  2. We are able to accept a link to a keyserver we're authenticated with
  3. A timeout is performed if the auth takes too long

I haven't tested if joining a community works when the handler performs successful auth - this requires first testing https://linear.app/comm/issue/ENG-6114/final-testing-task

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested review of this revision.Feb 8 2024, 6:27 AM
lib/hooks/invite-links.js
188 ↗(On Diff #36845)

Nit – I would invert the condition to reduce indentation

217 ↗(On Diff #36845)

Same here

lib/hooks/invite-links.js
180–181 ↗(On Diff #36845)

We probably should clear the state here

Clear the timeout on unmount

Clear the timeout on unmount

This code is quite complex in the case you were not able to test - when we connect to a different keyserver, and even mo so when we connect to a couple of keyservers. I can see that it would be hard to test now, but it would be great if you could create a linear task describing how this should be tested once it's possible, and set it as blocked by appropriate tasks.

We talked about this diff offline, and all my issues were addressed, so other than that I think this code is fine

This revision is now accepted and ready to land.Feb 12 2024, 6:49 AM
In D10992#318515, @inka wrote:

This code is quite complex in the case you were not able to test - when we connect to a different keyserver, and even mo so when we connect to a couple of keyservers. I can see that it would be hard to test now, but it would be great if you could create a linear task describing how this should be tested once it's possible, and set it as blocked by appropriate tasks.

Update the description of https://linear.app/comm/issue/ENG-6114/final-testing-task - we should test the links as a part of end-to-end connection handlers testing