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

feat: MemberInfo - expose when a project member joined the project #745

Merged
merged 4 commits into from
Aug 16, 2024

Conversation

tomasciccola
Copy link
Contributor

should close #744

@@ -25,8 +25,7 @@ test('getting yourself after creating project', async (t) => {
const deviceInfo = manager.getDeviceInfo()
const project = await manager.getProject(await manager.createProject())

const me = await project.$member.getById(project.deviceId)

const { joinedAt: _, ...me } = await project.$member.getById(project.deviceId)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we test the value of joinedAt somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, was thinking about this, see -> #745 (comment)

@@ -30,6 +30,7 @@ import { ROLES, isRoleIdForNewInvite } from './roles.js'
* @prop {import('./roles.js').Role} role
* @prop {import('@mapeo/schema').DeviceInfo['name']} [name]
* @prop {import('@mapeo/schema').DeviceInfo['deviceType']} [deviceType]
* @prop {import('@mapeo/schema').DeviceInfo['createdAt']} [joinedAt]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way we can make this required? It seems like it should always be defined, at least theoretically. Might require a larger change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, was going through that. The easiest way I'm thinking of is turning the result (here) into a SetOptional<MemberInfo,'joinedAt'> when declared. Is there a better type wizardry for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mhm, I think is basically the same making it an optional field. I always struggle with ts when declaring an object of a type where some members are added some lines later. Is there a way to signal this to the type system?? (basically, 'I know joinedAt is missing, just look some lines later!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case I think the issue is that calling this.#coreOwnership.getCoreId and this.#dataTypes.deviceInfo.getByDocId can throw. So if that happens then joinedAt would be undefined (as with the other optionals of MemberInfo)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I guess if everything else can be undefined then this is okay too.

@@ -25,8 +25,7 @@ test('getting yourself after creating project', async (t) => {
const deviceInfo = manager.getDeviceInfo()
const project = await manager.getProject(await manager.createProject())

const me = await project.$member.getById(project.deviceId)

const { joinedAt: _, ...me } = await project.$member.getById(project.deviceId)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

all tests fail 'cause they don't expect a joinedAt wondering options:

  1. remove joinedAt from the comparison and leave it at that
  2. remove joinedAt from the comparison but assert that the field should be there (despite not caring about matching values)
  3. another way of getting the expected value for joinedAt to add to the expected object

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the 2nd option. Maybe we could also check that the timestamp is near the current timestamp?

@EvanHahn
Copy link
Contributor

Looks like there's a test failure, too.

test-e2e/members.js Outdated Show resolved Hide resolved
Co-authored-by: Evan Hahn <[email protected]>
@tomasciccola tomasciccola merged commit b243235 into main Aug 16, 2024
7 checks passed
@tomasciccola tomasciccola deleted the feat/exposeCreatedAtToMemberApi branch August 16, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Surface when a project member joined a project
2 participants