[keyserver] Create an endpoint that can be used to create or update a public link
ClosedPublic
Actions

Authored by tomek on May 19 2023, 9:56 AM.

Details

Reviewers

bartek
kamil
inka

Commits

rCOMMe605f2b33a26: [keyserver] Create an endpoint that can be used to create or update a public…

Summary

If a community doesn't have a public invite link - it will be created. If it has one, it will be updated.

Depends on D7762

Test Plan

Create a new community, create a link for it using manage public link screen (later in the stack) - a link should be created. Update it, using the same screen - it should be updated.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

tomek created this revision.May 19 2023, 9:56 AM

Herald added subscribers: atul, ashoat. · View Herald TranscriptMay 19 2023, 9:56 AM

tomek added a child revision: D7879: [lib] Add client side boilerplate and reducer for an endpoint that allows creating or updating public links.May 19 2023, 10:00 AM

Harbormaster completed remote builds in B19514: Diff 26681.May 19 2023, 10:14 AM

tomek requested review of this revision.May 19 2023, 10:14 AM

kamil requested changes to this revision.May 22 2023, 2:36 AM

kamil added inline comments.

keyserver/src/creators/invite-link-creator.js
23–25 ↗	(On Diff #26681)	Can this be done via validatior? (I think `tcomb` supports regular expressions in built-in validators)
68 ↗	(On Diff #26681)	`invalid_parameters` will always be the case here? I feel like it's more `affectedRows = 0` case only.
86 ↗	(On Diff #26681)	How about `expiration_time`, `limit_of_uses` columns? this will probably work that way but asking as those do not have `DEFAULT` clause
91 ↗	(On Diff #26681)	I am a bit confused about naming, you are using phrase `public` and still setting `primary = 1`. How is the "public invite link" related to the "primary link", are those the same or different (and if different is the logic here correct)?

This revision now requires changes to proceed.May 22 2023, 2:36 AM

Use tID type

tomek planned changes to this revision.May 22 2023, 3:47 AM

Harbormaster completed remote builds in B19573: Diff 26754.May 22 2023, 4:04 AM

tomek added inline comments.May 23 2023, 5:39 AM

keyserver/src/creators/invite-link-creator.js
23–25 ↗	(On Diff #26681)	Makes sense!
68 ↗	(On Diff #26681)	In case of an exception we can't talk about affected rows. We can check what is inside the exception and based on that conclude that it is because a unique constraint was violated - we can do that as a part of https://linear.app/comm/issue/ENG-3916/better-error-handling.
86 ↗	(On Diff #26681)	They are nullable, so don't need the default value.
91 ↗	(On Diff #26681)	Initially, during the design syncs we were using mostly term `primary`. And the implementation started with that in mind. In the meantime, the copy on UI contains term `public`. So indeed, these are the same. And `public` is more recent. So agree, it should be clarified by replacing `primary` with `public`. But that would require significant amount of work - created https://linear.app/comm/issue/ENG-3921/rename-all-the-usages-of-primary-to-public-links to track that.

tomek added inline comments.May 23 2023, 5:51 AM

keyserver/src/creators/invite-link-creator.js
23–25 ↗	(On Diff #26681)	I've implemented it and it would require some changes in terms of error handling. Currently, for invalid text, the error message is `data isn't of type T` - which doesn't make too much sense. So updating this to be a validator should happen as a part of https://linear.app/comm/issue/ENG-3916/better-error-handling.