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 10 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 6 Commits 10 Files changed 5 +247 -26
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.
Elliu commented 2025-11-04 15:41:09 +00:00
Author
Contributor
Copy link

Ok, I just pushed doing a full clean of guild_space, guild_active, invite, and leaving the discord server.

The clean of invite is not strictly necessary, but I you don't clean it, it might lead to buttons in the "select a space" from the self service mode later where the unbridged space is here but if you click it, you get an error because it cannot join it

Ok, I just pushed doing a full clean of `guild_space`, `guild_active`, `invite`, and leaving the discord server. The clean of `invite` is not strictly necessary, but I you don't clean it, it might lead to buttons in the "select a space" from the self service mode later where the unbridged space is here but if you click it, you get an error because it cannot join it
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 Outdated
@ -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
Elliu commented 2025-11-04 15:22:39 +00:00
Author
Contributor
Copy link

About the quoted code, I'm not sure why it doesn't appear on the quote, but I replaced it with a

	const guild = await validateGuildAccess(event, guildID)

so the error was still being detected I think

Anyway, I reverted it back to inline checks so it should be ok now

About the quoted code, I'm not sure why it doesn't appear on the quote, but I replaced it with a ```js const guild = await validateGuildAccess(event, guildID) ``` so the error was still being detected I think Anyway, I reverted it back to inline checks so it should be ok now
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?
Elliu commented 2025-11-04 15:34:26 +00:00
Author
Contributor
Copy link

The test deletes the row above, to test for checks.
But as I added more tests below related to unlinking, if this is kept deleted, then this cause unwanted errors when trying to run following tests because of inconsistent DB. So I restored it before moving to next test.

The test deletes the row above, to test for checks. But as I added more tests below related to unlinking, if this is kept deleted, then this cause unwanted errors when trying to run following tests because of inconsistent DB. So I restored it before moving to next test.
Elliu added 4 commits 2025-11-04 15:32:17 +00:00
Elliu commented 2025-11-04 15:42:25 +00:00
Author
Contributor
Copy link

I also pushed 0f2e575df2 to demote bridge user power level, tell me if you'd rather like to have it on a dedicated MR or not, it can be removed from this one

I also pushed 0f2e575df21bf940e4780c30d2701da989f62471 to demote bridge user power level, tell me if you'd rather like to have it on a dedicated MR or not, it can be removed from this one
Elliu commented 2025-11-04 15:45:23 +00:00
Author
Contributor
Copy link

Not for now but opening the discussion for another day:

As we are leaving rooms / spaces, we might be leaving rooms and spaces, leaving them with no admin (especially in easy mode where if nobody got invited with admin rights, the bridge is the only admin). Maybe run an initial check before doing anything in /api/unlink / /api/unlink-space and add an additional warning to the user?

Not for now but opening the discussion for another day: As we are leaving rooms / spaces, we might be leaving rooms and spaces, leaving them with no admin (especially in easy mode where if nobody got invited with admin rights, the bridge is the only admin). Maybe run an initial check before doing anything in `/api/unlink` / `/api/unlink-space` and add an additional warning to the user?
Elliu commented 2025-11-04 15:47:19 +00:00
Author
Contributor
Copy link

Also, I'm not sure about what's the deal with ccc10564f1
The issue is explained in the deleted comment from this commit

I added await on some db operations and that fixed the issue, but I only did the change for the DB operations that were in the path of this API call, I'm not sure if this is something that should be added to more than those calls or not

Also, I'm not sure about what's the deal with ccc10564f1e33ab277bc15f360b8c65f2d0ea867 The issue is explained in the deleted comment from this commit I added await on some db operations and that fixed the issue, but I only did the change for the DB operations that were in the path of this API call, I'm not sure if this is something that should be added to more than those calls or not
Elliu reviewed 2025-11-04 15:51:24 +00:00
src/web/routes/link.js
@ -235,0 +280,4 @@
await db.prepare("DELETE FROM guild_space WHERE guild_id=? AND space_id=?").run(guild_id, spaceID)
await db.prepare("DELETE FROM guild_active WHERE guild_id=?").run(guild_id)
await discord.snow.user.leaveGuild(guild_id)
Elliu commented 2025-11-04 15:51:24 +00:00
Author
Contributor
Copy link

Also, this line causes issue when running the test.
I couldn't add the snow member like createRoom or api in the test to add the function / member definition because it wasn't defined in the type, not really sure what's the best way to fix it

Also, this line causes issue when running the test. I couldn't add the `snow` member like `createRoom` or `api` in the test to add the function / member definition because it wasn't defined in the type, not really sure what's the best way to fix it
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?