fix: write own device info when creating and adding projects #297
Conversation
achou11
force-pushed
the
289/project-device-info-init
branch
from
98255f1 to
2fd8cb5
Compare
achou11
force-pushed
the
289/project-device-info-init
branch
from
2fd8cb5 to
669bda5
Compare
There was a problem hiding this comment.
I think this should be done the "other way around", e.g. it is the responsibility of the MapeoManager class when it creates or adds a project to call project.setDeviceInfo().
As currently written, if we release a future version that allows non-device-owners to change their device info, and a device is running this older version, we will get an loop where the device would keep automatically changing it back.
achou11
changed the title
src/mapeo-manager.js
Outdated
| if (deviceInfo.name) { | ||
| await project.$setOwnDeviceInfo({ name: deviceInfo.name }) | ||
| } |
There was a problem hiding this comment.
wondering if this is desired behavior or not. keeping this means that one should call manager.setDeviceInfo() before adding or creating a project. Otherwise, attempting to use the $member namespace causes issues.
src/mapeo-manager.js
Outdated
| if (deviceInfo.name) { | ||
| const project = await this.getProject(projectPublicId) | ||
| await project.$setOwnDeviceInfo({ name: deviceInfo.name }) | ||
| } |
There was a problem hiding this comment.
same concern as previously stated regarding needing to call manager.setDeviceInfo() first
tests/member-api.js
Outdated
There was a problem hiding this comment.
found that there was a lot of overlap between the tests here and in various other places (both e2e + unit). It was getting harder to update this because of the trickiness with mocking the various contructor parameters, and felt like i was re-implementing a bad version of MapeoManager + MapeoProject. Decided that moving this to e2e to specifically test the $member namespace made more sense (see test-e2e/members.js)
test-e2e/members.js
Outdated
| // TODO: Fails. Needs MemberApi.invite() to be updated to write device info record of invited peer | ||
| test('getting others after invite', { todo: true }, async (t) => { |
There was a problem hiding this comment.
I think this test fails to run due to some additional work needed in the MemberApi implementation, namely saving the saving the invited member's device info when they accept.
There was a problem hiding this comment.
- DeviceInfo and CoreOwnership records need to be written by the invited peer
- The invitor needs to write the Role record
- They need to sync after invite so each gets these records
There was a problem hiding this comment.
I think actually the member namespace needs to be able to handle a device not having a name, and also no code ownership record existing for a device. It's possible that after an invite, the invite or writes the role record (so the device appears as a member) but sync does not complete, so no device info or code ownership record is received from the invited device. The api then needs to have name as optional, and we need to have a fallback in the UX - maybe just show the device ID (we haven't planned out device IDs yet - there was a discussion somewhere about it, I think base-32 of the truncated device key, maybe hashed too. That can be a separate issue.
|
a note that this is waiting on #326 in order to update the MemberApi implementation to handle what is described in #297 (review) |
* main: feat: NamespaceSyncState improvements (#339) chore: update better-sqlite3 to 8.7.0 (#337) chore: update @mapeo/crypto to latest (#336) fix: write own device info when creating and adding projects (#297) Feat: add DataStore.writeRaw method (#334) feat: Add NamespaceSyncState (#313) chore: run CI on PRs against any branch (#330) feat: add capabilities.getAll() (#326)
Closes #289
exposes a
$setOwnDeviceInfo()method onMapeoProject, which is used by theMapeoManagerwhen callingcreateProject(),acceptProject(), andsetDeviceInfo()