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 command to elevate a user (or the bot) as room administrator #219

Merged
merged 17 commits into from
Mar 7, 2022

Conversation

maranda
Copy link
Contributor

@maranda maranda commented Feb 9, 2022

This is a rebase of #126. I added tests yet as of now only the first self test works the second never does, fwiw I passed hours checking the correctness of the code (tried dozens and dozens of variants) and that there's not an error on my part.
The problem always happen when getFirstReaction() gets called on the second test, the message gets sent correctly in the management room, yet Mjolnir doesn't seem to pick it up.. So the problem looks to be into the mx-tester's boilerplate at this rate.

Copy link
Contributor

@Yoric Yoric left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR!

I have a few nitpicks, mostly on the test.

src/commands/MakeRoomAdminCommand.ts Outdated Show resolved Hide resolved
src/commands/MakeRoomAdminCommand.ts Outdated Show resolved Hide resolved
test/integration/commands/makedminCommandTest.ts Outdated Show resolved Hide resolved
test/integration/commands/makedminCommandTest.ts Outdated Show resolved Hide resolved
test/integration/commands/makedminCommandTest.ts Outdated Show resolved Hide resolved
@Yoric Yoric self-requested a review February 10, 2022 16:38
Copy link
Contributor

@Yoric Yoric left a comment

Choose a reason for hiding this comment

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

We're getting quite close!

config/default.yaml Outdated Show resolved Hide resolved
config/default.yaml Outdated Show resolved Hide resolved
src/commands/CommandHandler.ts Outdated Show resolved Hide resolved
config/harness.yaml Show resolved Hide resolved
config/default.yaml Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
test/integration/commands/makedminCommandTest.ts Outdated Show resolved Hide resolved
test/integration/commands/makedminCommandTest.ts Outdated Show resolved Hide resolved
test/integration/commands/makedminCommandTest.ts Outdated Show resolved Hide resolved
test/integration/commands/makedminCommandTest.ts Outdated Show resolved Hide resolved
- Textual description/error messages changes
- Make the command disabled by default
@Yoric
Copy link
Contributor

Yoric commented Feb 23, 2022

@maranda When you're ready for another review, don't forget to ping me :)

@maranda
Copy link
Contributor Author

maranda commented Feb 23, 2022

@Yoric ready :)

@Yoric Yoric self-requested a review February 23, 2022 09:03
Copy link
Contributor

@Yoric Yoric left a 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, thanks a lot!

Ready to merge when you are.

@maranda
Copy link
Contributor Author

maranda commented Feb 24, 2022

@Yoric 👌

@Yoric Yoric merged commit 97df4d5 into matrix-org:main Mar 7, 2022
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.

2 participants