-
Notifications
You must be signed in to change notification settings - Fork 1
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: add assignRole()
method to MemberApi
#423
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just one bug to fix around fromIndex
.
For testing that a role is updated rather than duplicated (two documents with the same ID without links
), we need a way of reaching into the internals and reading the RoleDoc in the database.
If we do this.#dataType[kCreateWithDocId]
twice for the same DocId (deviceId) like we were doing before, then the indexer would interpret this as a fork, and use the createdAt to resolve with the latest (which would be what we expect). Even if the docs were created at the same time (e.g. in a test) then the indexer would resolve the "winner" using the versionId (which is the the core discovery key and the block index), which if both assignments are by the same device, then the "latest" (e.g. update) would also always resolve as the winner. The only way we would see the "create twice rather than update" bug would be if a different device did the "update", with exactly the same createdAt date, and with a versionId for the first that resolves as the "winner" over the versionId of the second.
TL;DR Testing this without reaching into the internals to check for forks would be really hard / almost impossible, so we should have a test that does something like project[kDataTypes].role.getByDocId()
and then checks that role.forks.length === 0
.
src/capabilities.js
Outdated
|
||
if (existingRoleDoc) { | ||
await this.#dataType.update(existingRoleDoc.versionId, { | ||
...valueOf(existingRoleDoc), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use the current fromIndex
- the role applies from the fromIndex
calculated above. Better not use the existingRoleDoc, just use the same value as when creating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed via 120eea5
src/capabilities.js
Outdated
.catch(() => null) | ||
|
||
if (existingRoleDoc) { | ||
await this.#dataType.update(existingRoleDoc.versionId, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if this should be a merge update? E.g.
const links = [existingRoleDoc.versionId, ...existingRoleDoc.forks]
await this.#dataType.update(links, //...
I think maybe yes for now, to keep the database clean, until we have some kind of UI to deal with forks in a role.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed via 120eea5
310725f
to
120eea5
Compare
@gmaclennan added some assertions for the role record in the added test via fa5afed. only checking on the peer that calls the assignRole() since i didn't think it was necessary to be exhaustive and check both peers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added one additional test for a forked role and checking that it is merged. Looks great, good work, let's merge.
Closes #413
Includes the following changes:
Potential TODOs:
ideally add another test in the role assigning e2e test for each peer updating roles locally and then syncing. Easier to do if we had a greater variety of roles since creators and coordinators are the only ones that can assign roles in practice
not really sure what the best way to test that we update the role doc instead of creating a new one as we don't really expose the raw role docs in any of the higher-level APIs. maybe we need unit tests for the Capabilities class and access that granular information in those?
assignRole()
multiple times for the same device id implicitly tests this, so maybe doesn't need a specifically named test? Can confirm that the e2e test i added catches this