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

Add mumble registration related commands #591

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

s-maurice
Copy link

@s-maurice s-maurice commented May 27, 2024

  • Adds user-register, get-registered-user, and user-rename-registered commands. Registration deletion is handled by sending a rename command with an undefined name. I think filtering get-registered-user has to pull down the whole user list. Maybe it's better to just have a get-registered-list function instead or something.

  • Adds userId as synced property. This stores the registration id.
    Instead of being a normal syncProperty, a variant syncPropertyWithMapper is used.
    This is a hack I'm using for now, because the murmur server will return -1 (4294967295) on successful registration deletion. -1 values are mapped into undefined before syncing. I also ended up having to @ts-ignore to make this work. There should be a better way to do this.

  • For userRenameRegistered I'm not sure which packet to wait for. Currently I wait for a UserState packet but that might not work if the renamed user is offline.

  • Add register, deregister, renameRegistered methods on user. deregister and renameRegistered are registration dependent, so the user doesn't even need to be online. It might be better to include them somewhere else.

Don't merge yet, as tests are missing. Will fix after any changes. The linter is also broken on my machine.

s-maurice added 3 commits May 27, 2024 17:42
add userId as synced property, requires syncPropertyWithMapper as workaround.
add register, deregister, renameRegistered methods on user.
…istered user by their name, and userRenameRegistered to rename by user id.
src/commands/get-registered-user.ts Outdated Show resolved Hide resolved
src/commands/get-registered-user.ts Outdated Show resolved Hide resolved
src/commands/user-rename-registered.ts Outdated Show resolved Hide resolved
src/errors/no-registered-user.error.ts Outdated Show resolved Hide resolved
src/user.ts Outdated Show resolved Hide resolved
src/user.ts Outdated Show resolved Hide resolved
src/user.ts Outdated Show resolved Hide resolved
src/user.ts Outdated Show resolved Hide resolved
@garrappachc
Copy link
Member

Nice work! These are some useful commands :)

e2e/registers-user.e2e-spec.ts Outdated Show resolved Hide resolved
s-maurice added 5 commits May 30, 2024 00:55
Removed magic number MinusOneButUnsigned
Converted NoRegisteredUserError -> UserNotRegisteredError
Moved getRegisteredUserWithName into client
Removed syncPropertyWithMapper hack -> Mapping conversion is handled by getter/setter in User
@s-maurice
Copy link
Author

s-maurice commented May 29, 2024

What kind of tests am I missing? I'm not sure what I can test in the .spec files other than the getter/setter for userId.

Other than that I think it's complete.

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@s-maurice s-maurice requested a review from garrappachc May 30, 2024 13:17
@s-maurice s-maurice marked this pull request as ready for review June 5, 2024 22:13
@garrappachc
Copy link
Member

Can you rebase to master or resolve merge conflicts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants