Feat: add fields to invites #393

tomasciccola · 2023-11-29T15:35:43Z

should close #275

1. Add roleName and roleDescription to Invite protobuf (rpc.proto) 2. add fields to memberApi.invite 3. send those fields as part of 'invite-received' on invite-api callback

tomasciccola · 2023-11-29T15:39:57Z

the e2e tests that are failing are all related to calling manager.addProject which expects an Invite. I don't know from the context what the roleName (the missing field) should be in any case, or if maybe the signature for that function should Omit the roleName from Invite.
For now I'll go through every failing test and see if I can sort of guess the context?

* Omit roleName from mapeoManager.addProject (solves a bunch of tests, but may need to regress this) * capitalize roleName in member-api * fix local-peers tests

tomasciccola · 2023-11-29T16:15:41Z

test-e2e/members.js are failing with a timeout. It seems there's a bunch of promises not being resolved (see test-e2e/utils.js:93)
The reason seems to be the error check added here.
One solution would be to add a project name on the e2e test itself (haven't found a method to do that besides passing the name to localPeers.invite() which is different from memberApi.invite() ...)
Another question is, in ProjectSettings schema, name is optional. Should we keep it like that?

achou11 · 2023-11-29T18:04:51Z

test-e2e/members.js are failing with a timeout. It seems there's a bunch of promises not being resolved (see test-e2e/utils.js:93) The reason seems to be the error check added here. One solution would be to add a project name on the e2e test itself (haven't found a method to do that besides passing the name to localPeers.invite() which is different from memberApi.invite() ...)

The fix for the test would be to update this line in the test so that the project that's created has a name e.g. await invitor.createProject({ name: 'mapeo' }):

https://github.com/digidem/mapeo-core-next/blob/d1d398d09ed861c6ed2e2ad08eb00ff5d5cc6c36/test-e2e/members.js#L129

Probably also makes sense to have a test that makes sure that invites don't work if the project doesn't have a name, if that doesn't exist already.

Another question is, in ProjectSettings schema, name is optional. Should we keep it like that?

i think it should be kept that way. i believe we explicitly decided to let the consumer decide the fallback name for projects that don't have a name

tomasciccola · 2023-11-29T18:16:57Z

The fix for the test would be to update this line in the test so that the project that's created has a name e.g. await invitor.createProject({ name: 'mapeo' }):

heh, I missed that method signature 😄

achou11

raised a couple of concerns but seems like the foundational pieces are there! there's a related issue around translations that isn't entirely clear to me just yet, which would be good to get a resolution on

src/member-api.js

src/utils.js

src/local-peers.js

src/generated/rpc.js

achou11 · 2023-11-29T18:21:33Z

src/member-api.js

+   * @param {import('./generated/rpc.js').Invite_RoleName} opts.roleName
+   * @param {string} [opts.roleDescription]


Not sure the params should change here. To me, the roleId should still be used here, and the name and description sent with the invite are derived based on the capabilities associated with the roleId.

The other (tangential) uncertainty i have is how this all works with translations. if the name and description are built into the capability definition, how does internationalization work when you want to surface those things are part of the UI? (easier for name than description in that case)

thoughts @gmaclennan ?

I think what you say makes sense, but syncing this morning with gregor we arrived at that change maybe there's some reason I'm forgetting??

hm i see. if this was discussed already then feel free to disregard my initial comment! i admit the advantage of providing the description is that there's more freedom from the consumer to describe the role/capability, as opposed to us hardcoding it in the capability definition. still unsure about the role name as a param but hard to tell without diving deeper myself

Yes good points @achou11, it was late last night when discussing and I hadn't fully wrapped my head around this - I was wondering where the frontend would get the roleId from, but having a fixed role name doesn't allow the frontend to internationalize what is sent in the invite.

We need to export the DEFAULT_CAPABILITIES from @mapeo/core so the front end can read those - ideally they are accessible from the mapeo client without needing to do an async call to the backend - maybe an opportunity to use package.exports @achou11 :)

We keep roleId as the required param here, and both role name and role description optional. Role name can default to the name on the capability record. The frontend can pass a translated role name (the invitor would decide on the translation, rather than the invitee - because in the future capabilities and capability names might not be available on a device before it joins a project)

Need to add a check for a valid role ID and throw if invalid.

If we keep roleId, shouldn't we remove roleName from params since it can be grabbed from the roleId. I dunno about roleDescription since DEFAULT_CAPABILITIES doesn't have a roleDescription (maybe the idea is to add it to that record?)

If we keep roleId, shouldn't we remove roleName from params since it can be grabbed from the roleId.

it's still useful for roleName to be an optional param (not a required param) because otherwise we wouldn't be able to easily provide a translated role name for the invited device to see when they receive the invite. Gregor's suggestion is to have the optional name param and if that isn't provided use the name associated with the roleId. For example, an application should be able to do members.invite({ roleName: "Miembro", ... }) if they want the invited device to see the translated name of the role. Otherwise the default is the english name we're using in the role id definition

We keep roleId as the required param here, and both role name and role description optional.

The only risk with this is inviting someone with misleading information, which isn't a huge issue for us I guess. For example, one could do this:

members.invite({ roleId: MEMBER_ROLE_ID, roleName: "Coordinator", roleDescription: "Definitely a coordinator ;)" })

This avoid the tests hanging since now we need a project name in order to send an invite

achou11 · 2023-11-29T18:26:35Z

is adding the invitor name and id also going to be part of this PR? doesn't seem like it's doing so at the moment, but I think those were a later addition to that issue's checklist.

if not, let's make sure to update the PR title and description to be precise about which fields are being added by these changes

* regenerate rpc.js due to formatting issues

tomasciccola · 2023-11-29T18:42:34Z

is adding the invitor name and id also going to be part of this PR? doesn't seem like it's doing so at the moment, but I think those were a later addition to that issue's checklist.

if not, let's make sure to update the PR title and description to be precise about which fields are being added by these changes

I must double check with @gmaclennan but I think what we talked about was that sending the invitorId (peerId) is redundant since its already sended with the invite?
Regarding invitorName, I still need to add it. I think we resolved to being the deviceName by calling mapeoProject.getOwnDeviceName.
If I'm correct I'll update the issue and remove Invitor Name from the checklist, does that sound right??

achou11 · 2023-11-29T18:44:34Z

is adding the invitor name and id also going to be part of this PR? doesn't seem like it's doing so at the moment, but I think those were a later addition to that issue's checklist.
if not, let's make sure to update the PR title and description to be precise about which fields are being added by these changes

I must double check with @gmaclennan but I think what we talked about was that sending the invitorId (peerId) is redundant since its already sended with the invite? Regarding invitorName, I still need to add it. I think we resolved to being the deviceName by calling mapeoProject.getOwnDeviceName. If I'm correct I'll update the issue and remove Invitor Name from the checklist, does that sound right??

yeah i think that makes sense 👍

gmaclennan

a couple of cleanup suggestions, but some requested changes around role names and passing the role ID

proto/rpc.proto

src/mapeo-manager.js

src/member-api.js

gmaclennan · 2023-11-30T03:04:48Z

src/member-api.js

+   * @param {import('./generated/rpc.js').Invite_RoleName} opts.roleName
+   * @param {string} [opts.roleDescription]


Yes good points @achou11, it was late last night when discussing and I hadn't fully wrapped my head around this - I was wondering where the frontend would get the roleId from, but having a fixed role name doesn't allow the frontend to internationalize what is sent in the invite.

We need to export the DEFAULT_CAPABILITIES from @mapeo/core so the front end can read those - ideally they are accessible from the mapeo client without needing to do an async call to the backend - maybe an opportunity to use package.exports @achou11 :)

We keep roleId as the required param here, and both role name and role description optional. Role name can default to the name on the capability record. The frontend can pass a translated role name (the invitor would decide on the translation, rather than the invitee - because in the future capabilities and capability names might not be available on a device before it joins a project)

Need to add a check for a valid role ID and throw if invalid.

gmaclennan · 2023-11-30T03:08:18Z

If I'm correct I'll update the issue and remove Invitor Name from the checklist, does that sound right??

Invitor ID can be removed from the checklist, but invitor name is still needed, as you say, from mapeoProject.getOwnDeviceName

* revert Invite.roleName to be a string instead of an enum * add `roleId` again as a param to memberApi.invite. derive roleName as `DEFAULT_CAPABILITIES[roleId]` * on MapeoManager, instead of `Omit` `roleName`, `Pick` desired fields from `Invite` * fix tests do to api changes in memberApi.invite

src/invite-api.js

tomasciccola · 2023-11-30T19:45:36Z

test-e2e/sync.js is failing with code: 'SQLITE_READONLY_DBMOVED'. It happens only on linux and I can repro locally

achou11 · 2023-11-30T19:57:29Z

test-e2e/sync.js is failing with code: 'SQLITE_READONLY_DBMOVED'. It happens only on linux and I can repro locally

seems to happen on windows too, based on another run on the main branch: https://github.com/digidem/mapeo-core-next/actions/runs/7019047664/job/19095674265#step:6:2657

achou11 · 2023-11-30T20:00:36Z

@tomasciccola mind trying this from #396 to fix the tests?

https://github.com/digidem/mapeo-core-next/pull/396/files#diff-05ee223fd47283dc5d5dcee7b4b6de4bc57af24b09892a83627080bf64d7603c

EDIT: Doesn't look like it fully fixes the issue but basically seems like a known issue

gmaclennan · 2023-11-30T22:15:49Z

The failing test here is because the tear down function is deleting the temporary storage files, but SQLite is still trying to write to the database. I’m ok with ignoring for now until we merge the close() PR.

…ddFieldsToInvites

achou11

Overall changes are looking good! still noted a couple of items that should be addressed in this PR and ideally have tests added for them.

src/member-api.js

test-e2e/utils.js

src/member-api.js

src/capabilities.js

… not passed * check MEMBER and COORDINATOR ROLE_ID as only valid roleIds * delete `roleIdFromName` from `src/capabilities.js`

Tomás Ciccola added 3 commits November 29, 2023 12:03

add fields to invites

1fdde13

1. Add roleName and roleDescription to Invite protobuf (rpc.proto) 2. add fields to memberApi.invite 3. send those fields as part of 'invite-received' on invite-api callback

make roleName an Enum of COORDINATOR, MEMBER, BLOCKED

6f3fa02

start fixing e2e tests

97adb86

tomasciccola requested review from gmaclennan and achou11 November 29, 2023 15:35

keep fixing tests

d1d398d

* Omit roleName from mapeoManager.addProject (solves a bunch of tests, but may need to regress this) * capitalize roleName in member-api * fix local-peers tests

achou11 requested changes Nov 29, 2023

View reviewed changes

add project name to members and sync e2e tests

1d24c7d

This avoid the tests hanging since now we need a project name in order to send an invite

* add roleIdFromName to simplify memberApi.invite

ca6a475

* regenerate rpc.js due to formatting issues

gmaclennan requested changes Nov 30, 2023

View reviewed changes

Tomás Ciccola added 2 commits November 30, 2023 14:46

add invitorName to invite

4418d00

tomasciccola commented Nov 30, 2023

View reviewed changes

src/invite-api.js Outdated Show resolved Hide resolved

Tomás Ciccola added 2 commits November 30, 2023 15:20

union Invite with {invitorName:string}

5648271

add invitorName to Invite protobuf and fix tests

77d3863

Merge branch 'main' of github.com:digidem/mapeo-core-next into feat/a…

e2a993a

…ddFieldsToInvites

achou11 requested changes Dec 5, 2023

View reviewed changes

src/member-api.js Outdated Show resolved Hide resolved

src/member-api.js Outdated Show resolved Hide resolved

test-e2e/utils.js Outdated Show resolved Hide resolved

src/member-api.js Outdated Show resolved Hide resolved

src/capabilities.js Outdated Show resolved Hide resolved

* Make roleName optional in memberApi.invite, derive from roleId if…

f3cb7f0

… not passed * check MEMBER and COORDINATOR ROLE_ID as only valid roleIds * delete `roleIdFromName` from `src/capabilities.js`

tomasciccola requested a review from achou11 December 11, 2023 20:16

achou11 approved these changes Dec 11, 2023

View reviewed changes

achou11 mentioned this pull request Dec 12, 2023

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.4 #412

Merged

Merge branch 'main' into feat/addFieldsToInvites

4900007

tomasciccola merged commit 6eac910 into main Dec 13, 2023
7 checks passed

tomasciccola deleted the feat/addFieldsToInvites branch December 13, 2023 16:41

optic-release-automation bot mentioned this pull request Mar 14, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.5 #519

Merged

This was referenced Apr 11, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.6 #558

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.7 #573

Merged

optic-release-automation bot mentioned this pull request Apr 30, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.8 #592

Merged

optic-release-automation bot mentioned this pull request May 9, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.9 #632

Merged

This was referenced Jun 20, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.10 #709

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.11 #711

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.12 #714

Merged

This was referenced Jul 25, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.13 #726

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.14 #740

Merged

optic-release-automation bot mentioned this pull request Aug 13, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.15 #748

Merged

This was referenced Aug 21, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.16 #758

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.17 #787

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: add fields to invites #393

Feat: add fields to invites #393

tomasciccola commented Nov 29, 2023

tomasciccola commented Nov 29, 2023

tomasciccola commented Nov 29, 2023 •

edited

Loading

achou11 commented Nov 29, 2023 •

edited

Loading

tomasciccola commented Nov 29, 2023

achou11 left a comment

achou11 Nov 29, 2023

tomasciccola Nov 29, 2023

achou11 Nov 29, 2023 •

edited

Loading

gmaclennan Nov 30, 2023

tomasciccola Nov 30, 2023

achou11 Nov 30, 2023 •

edited

Loading

achou11 Nov 30, 2023 •

edited

Loading

achou11 commented Nov 29, 2023 •

edited

Loading

tomasciccola commented Nov 29, 2023

achou11 commented Nov 29, 2023

gmaclennan left a comment

gmaclennan Nov 30, 2023

gmaclennan commented Nov 30, 2023

tomasciccola commented Nov 30, 2023

achou11 commented Nov 30, 2023

achou11 commented Nov 30, 2023 •

edited

Loading

gmaclennan commented Nov 30, 2023

achou11 left a comment

		* @param {import('./generated/rpc.js').Invite_RoleName} opts.roleName
		* @param {string} [opts.roleDescription]

Feat: add fields to invites #393

Feat: add fields to invites #393

Conversation

tomasciccola commented Nov 29, 2023

tomasciccola commented Nov 29, 2023

tomasciccola commented Nov 29, 2023 • edited Loading

achou11 commented Nov 29, 2023 • edited Loading

tomasciccola commented Nov 29, 2023

achou11 left a comment

Choose a reason for hiding this comment

achou11 Nov 29, 2023

Choose a reason for hiding this comment

tomasciccola Nov 29, 2023

Choose a reason for hiding this comment

achou11 Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

gmaclennan Nov 30, 2023

Choose a reason for hiding this comment

tomasciccola Nov 30, 2023

Choose a reason for hiding this comment

achou11 Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

achou11 Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

achou11 commented Nov 29, 2023 • edited Loading

tomasciccola commented Nov 29, 2023

achou11 commented Nov 29, 2023

gmaclennan left a comment

Choose a reason for hiding this comment

gmaclennan Nov 30, 2023

Choose a reason for hiding this comment

gmaclennan commented Nov 30, 2023

tomasciccola commented Nov 30, 2023

achou11 commented Nov 30, 2023

achou11 commented Nov 30, 2023 • edited Loading

gmaclennan commented Nov 30, 2023

achou11 left a comment

Choose a reason for hiding this comment

tomasciccola commented Nov 29, 2023 •

edited

Loading

achou11 commented Nov 29, 2023 •

edited

Loading

achou11 Nov 29, 2023 •

edited

Loading

achou11 Nov 30, 2023 •

edited

Loading

achou11 Nov 30, 2023 •

edited

Loading

achou11 commented Nov 29, 2023 •

edited

Loading

achou11 commented Nov 30, 2023 •

edited

Loading