cadence/out-of-your-element
cadence/out-of-your-element
3
7
Fork 3
Code Issues 11 Pull requests 2 Releases 22 Activity

feat/add-unlink-space-button #63

Open
Elliu wants to merge 6 commits from Elliu/out-of-your-element:feat/add-unlink-space-button into main
pull from: Elliu/out-of-your-element:feat/add-unlink-space-button
merge into: cadence:main
Conversation 2 Commits 6 Files changed 4 +268 -40
Elliu commented 2025-11-01 13:40:53 +00:00
Contributor
Copy link

Add an unlink space button in the /guild page

This unlinks every rooms in the linked space, then self power down the bridge user, and leave the space

Add an unlink space button in the /guild page This unlinks every rooms in the linked space, then self power down the bridge user, and leave the space
Elliu added 6 commits 2025-11-01 13:40:53 +00:00 
tests, to delete after rebase main when fix-remote-join is merged)
Elliu reviewed 2025-11-01 13:43:24 +00:00
src/web/routes/link.js Outdated
@ -213,0 +271,4 @@
// but when selected we get the "Please add the bot to your server using the buttons on the home page." page
//
// So either keep as-is, or delete from guild_active, but also leave the discord guild? Not sure if we want that or not
// db.prepare("DELETE FROM guild_active WHERE guild_id=?").run(guild_id)
Elliu commented 2025-11-01 13:43:04 +00:00
Author
Contributor
Copy link

Hi, there is this issue here, I'm not sure what would be the best thing to do with this situation, please let me know if you have any ideas of what should be best

Hi, there is this issue here, I'm not sure what would be the best thing to do with this situation, please let me know if you have any ideas of what should be best
cadence commented 2025-11-02 09:58:02 +00:00
Owner
Copy link

Nice find, thanks for pointing it out. For further reading, the purpose and values of guild_active are explained here: https://gitdab.com/cadence/out-of-your-element/src/branch/main/docs/self-service-room-creation-rules.md

The message "Please add the bot to your server using the buttons on the home page" is deliberately designed to appear in this situation. However, the user experience here could be better.

There's a couple of possible paths forward here:

  • leave the Discord guild as part of this operation
  • change the view that appears in that case (guild_access_denied.pug line 31) to present the easy-mode and self-service buttons directly here, and have the user go through those if they want to do setup for the guild again
  • set guild_active autocreate = 0, forcing it into self-service mode, so it knows it's ready to bridge but nothing gets autmatically re-created (I don't like this one)

Because per the explainer doc, it's bad to leave around the a guild_active entry if you don't want the guild to be bridged. If Easy Mode was previously selected, it would cause channels to bridge themselves again just from anybody sending a message in the server. That would be pretty weird if somebody just clicked unbridge and it starts bridging again seemingly by itself! So we can't just do nothing here.

I'll let you pick which path forward you'd like to go with. I don't have any preference between the first two.

Nice find, thanks for pointing it out. For further reading, the purpose and values of guild_active are explained here: https://gitdab.com/cadence/out-of-your-element/src/branch/main/docs/self-service-room-creation-rules.md The message "Please add the bot to your server using the buttons on the home page" is deliberately designed to appear in this situation. However, the user experience here could be better. There's a couple of possible paths forward here: - leave the Discord guild as part of this operation - change the view that appears in that case (guild_access_denied.pug line 31) to present the easy-mode and self-service buttons directly here, and have the user go through those if they want to do setup for the guild again - set guild_active autocreate = 0, forcing it into self-service mode, so it knows it's ready to bridge but nothing gets autmatically re-created (I don't like this one) Because per the explainer doc, it's bad to leave around the a guild_active entry if you don't want the guild to be bridged. If Easy Mode was previously selected, it would cause channels to bridge themselves again just from anybody sending a message in the server. That would be pretty weird if somebody just clicked unbridge and it starts bridging again seemingly by itself! So we can't just do nothing here. I'll let you pick which path forward you'd like to go with. I don't have any preference between the first two.
cadence commented 2025-11-02 10:00:30 +00:00
Owner
Copy link

I note that you've coded it to always leave the Matrix space after this operation. I don't think it would be out of character for it to leave the Discord server as well.

I note that you've coded it to always leave the Matrix space after this operation. I don't think it would be out of character for it to leave the Discord server as well.
cadence force-pushed feat/add-unlink-space-button from 10d14bbdaa to 16309f26b3 2025-11-02 07:51:44 +00:00 Compare
cadence added 1 commit 2025-11-02 09:31:23 +00:00
cadence reviewed 2025-11-02 09:43:23 +00:00
cadence left a comment
Owner
Copy link

Just an initial eyeball. I haven't executed this code yet.

Just an initial eyeball. I haven't executed this code yet.
src/web/routes/link.js
@ -119,4 +175,2 @@
// Check guild ID or nonce
const guildID = parsedBody.guild_id
if (!managed.has(guildID)) throw createError({status: 403, message: "Forbidden", data: "Can't edit a guild you don't have Manage Server permissions in"})
cadence commented 2025-11-02 09:43:07 +00:00
Owner
Copy link

You deleted the thing that checks that the user has Manage Server permissions and you didn't put it back.

Not sure how this wasn't caught by tests. Maybe I need to add more tests.

I think I preferred how this looked before, where the checks were done inline, rather than how it looks now, where you've extracted each check to a function. Sure, there was some repetition of code before, but I think it was easier to read because you can just read down the file and see everything, rather than having to jump around looking in to each function.

If you keep the checked as extracted functions, I would like to see more descriptive function names + descriptions of what happens in the /** */ comments on the function so they can be hovered to see their intention. The current names are not so good:

  • validateUserHaveRightsOnGuild - what kind of validation outcome + what kind of user + what kind of rights? try ensureDiscordUserHasManageServer
  • validateGuildAccess - what kind of validation outcome + what kind of access + whose access? try coming up with a better name
You deleted the thing that checks that the user has Manage Server permissions and you didn't put it back. Not sure how this wasn't caught by tests. Maybe I need to add more tests. I think I preferred how this looked before, where the checks were done inline, rather than how it looks now, where you've extracted each check to a function. Sure, there was some repetition of code before, but I think it was easier to read because you can just read down the file and see everything, rather than having to jump around looking in to each function. If you keep the checked as extracted functions, I would like to see more descriptive function names + descriptions of what happens in the `/** */` comments on the function so they can be hovered to see their intention. The current names are not so good: - `validateUserHaveRightsOnGuild` - what kind of validation outcome + what kind of user + what kind of rights? try `ensureDiscordUserHasManageServer` - `validateGuildAccess` - what kind of validation outcome + what kind of access + whose access? try coming up with a better name
src/web/routes/link.test.js
@ -678,2 +680,4 @@
}))
t.equal(error.data, "Channel ID 665310973967597573 is not currently bridged")
db.prepare("INSERT INTO channel_room (channel_id, room_id, name, nick, thread_parent, custom_avatar, last_bridged_pin_timestamp, speedbump_id, speedbump_checked, speedbump_webhook_id, guild_id, custom_topic) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)").run(row.channel_id, row.room_id, row.name, row.nick, row.thread_parent, row.custom_avatar, row.last_bridged_pin_timestamp, row.speedbump_id, row.speedbump_checked, row.speedbump_webhook_id, row.guild_id, row.custom_topic)
cadence commented 2025-11-02 09:32:24 +00:00
Owner
Copy link

What is the purpose of this database operation?

What is the purpose of this database operation?
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u feat/add-unlink-space-button:Elliu-feat/add-unlink-space-button
git checkout Elliu-feat/add-unlink-space-button

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git checkout main
git merge --no-ff Elliu-feat/add-unlink-space-button
git checkout Elliu-feat/add-unlink-space-button
git rebase main
git checkout main
git merge --ff-only Elliu-feat/add-unlink-space-button
git checkout Elliu-feat/add-unlink-space-button
git rebase main
git checkout main
git merge --no-ff Elliu-feat/add-unlink-space-button
git checkout main
git merge --squash Elliu-feat/add-unlink-space-button
git checkout main
git merge --ff-only Elliu-feat/add-unlink-space-button
git checkout main
git merge Elliu-feat/add-unlink-space-button
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
blocking

critical issue that blocks release of milestone

  
web
No labels
blocking
web
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: cadence/out-of-your-element#63
No description provided.
Delete branch "Elliu/out-of-your-element:feat/add-unlink-space-button"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?