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

Automatically create mapped channels if mappings[...].createRoom is set #1099

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

Half-Shot
Copy link
Contributor

@Half-Shot Half-Shot commented Aug 7, 2020

Fixes #1096

The idea behind this is that you can just specify a bunch of mappings in your config and have rooms be created on the fly for them.

There are a few problems though. The behavior of mappings is to normally only bridge things defined in the section. If you remove a mapping entry, the bridge should stop bridging it. However this is more difficult in this case as the roomId is created by the bridge and not stored in the config. We need to persist it in the db, but this means adding special behaviors. I've done this for postgres, but it's harder to do in NeDB.

Copy link
Contributor

@jaller94 jaller94 left a comment

Choose a reason for hiding this comment

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

Still reviewing.. but here are some comments that would be helpful to have answered.

let exclusionQuerys = [];
console.log(server.getNetworkId(), server.getAutoCreateMappings());
for (const mapping of server.getAutoCreateMappings()) {
exclusionQuerys.push(mapping.channel);
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though this comes from a config that only an admin can configure, I wish we would escape the channel names before putting them into an SQL statement.

createRoom: true,
};

await test.beforeEach(env);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this?
... Looks up env-bundle...

for (const [channel, data] of Object.entries(serverConfig.mappings)) {
if (data.createRoom) {
// We don't want to map this.
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Return, not continue??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a habit of mixing those up 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

.map and functions let you use your preferred return command. 😀

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return;
continue;


for (const [channel, opts] of Object.entries(serverConfig.mappings)) {
if (opts.createRoom) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

One opts.createRoom makes the entire setting of the server impossible?

});
const notChannels = server.getAutoCreateMappings().map((c) => c.channel);
entries = (await entries).filter((e) => e.remote?.get("domain") === server.domain &&
!notChannels.includes(e.remote?.get("channel") as string));
Copy link
Contributor

@jaller94 jaller94 Aug 13, 2020

Choose a reason for hiding this comment

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

remote? but a cast to string.
That seems like a cast where we're possibly lying to get TypeScript happy. Can this not be string or undefined?

});
const notChannels = server.getAutoCreateMappings().map((c) => c.channel);
entries = (await entries).filter((e) => e.remote?.get("domain") === server.domain &&
Copy link
Contributor

Choose a reason for hiding this comment

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

entries was already awaited two lines above.

for (const [channel, data] of Object.entries(serverConfig.mappings)) {
if (data.createRoom) {
// We don't want to map this.
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

.map and functions let you use your preferred return command. 😀

@Half-Shot Half-Shot requested a review from a team as a code owner June 9, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants