Discord Bot #417

Argn0 · 2022-02-25T16:44:31Z

All roles that have previously been set manually will be removed. Users who are still subscribers must assign them again themselves.

closes #63

howardchung · 2022-02-25T16:47:57Z

server/cleanup.ts

+
+    try {
+      // get all known users who have been assigned a sub role
+      const result = await postgres?.query('SELECT * FROM discord');


I'm fine with this being one-way (e.g. we don't remove roles for former subscribers).

Argn0 · 2022-02-25T16:48:56Z

This must be set for the bot to work

howardchung · 2022-02-25T16:49:03Z

server/server.ts

+      return res.status(400).json({ error: 'invalid user token' });
+    }
+    const customer = await getCustomerByEmail(decoded.email as string);
+    const isSubscriber = Boolean(


Do we have a helper function for this logic?

howardchung · 2022-02-25T16:50:05Z

server/server.ts

+    const isSubscriber = Boolean(
+      customer?.subscriptions?.data?.find((sub) => sub?.status === 'active')
+    );
+    if (!isSubscriber) {


TBH I think it would be okay to let all users link a Discord, but then have the background process do the sub checking/role assigning

howardchung · 2022-02-25T16:52:02Z

server/server.ts

+          true
+        );
+        if (user) {
+          await postgres?.query('DELETE FROM discord WHERE "customerId" = $1', [


we could just store email + discord id + sub state? And we could update the sub state in the same code that already syncs subs to postgres

But do we have to save anything if we never remove the sub roles like you suggested above?

I assume users might want to delete/edit their discord linked account

howardchung · 2022-02-25T16:52:59Z

server/server.ts

+    }
+    return res.status(200).json({});
+  })
+  .get('/discordRole', async (req, res) => {


could we return this in the metadata call maybe?

howardchung · 2022-02-25T16:54:41Z

src/components/Modal/ProfileModal.tsx

+    });
+  };
+
+  setDiscordUsername(value: string) {


rather than making the user enter a name and risk errors (or entering someone else's name), could we integrate with Discord OAuth to automatically fetch the user's info?

Argn0 · 2022-02-26T17:31:11Z

In the generated authentication URL the response_type=code must be changed to response_type=token

Argn0 · 2022-02-26T19:05:45Z

We don't do any security checks and right now someone could theoretically link someone else's Discord account but I don't believe this is something to be concerned about because no damage could be caused.

howardchung · 2022-02-26T23:47:02Z

server/server.ts

+  try {
+    // users should only have one account max. with a sub role per email,
+    // set email to null to tell background process to clean it up.
+    await postgres?.query('UPDATE discord SET email = null WHERE email = $1', [


wonder if it's better to have one links table that can support linking different types of accounts in the future by adding more columns?

Do you mean a table which has a column for each service's user ID? I think with a table like that we would not have to be matching the email address anymore.

we would still be matching the email to find the row, but that row could support multiple services.

server/server.ts

howardchung · 2022-02-27T00:02:33Z

server/syncSubs.ts

+    const members = await guild?.members.fetch();
+
+    // from discord API get username and discriminator of all users on discord server without a sub role
+    const membersWithoutRole = members


What if we just joined the subscriber and discord links table and applied the role to all those users? Since the discord has thousands of members, might be more efficient? As mentioned before I don't think cleanup is essential and we can add a separate process for that later if needed.

Yes, we could do that. But we will have to add a hasRole column to the discord table so we don't keep trying to assign a role to users who already have one over and over again.

this may be moot if we just do the role add on link

but to answer the question, can we check-then-assign for each user?

Yes, we can check role status for each user with an individual discord API call, if this is what you're asking.

src/components/Discord/Discord.tsx

howardchung · 2022-02-27T00:05:51Z

src/components/Modal/ProfileModal.tsx

+
+  deleteDiscord = async () => {
+    const token = await this.props.user.getIdToken();
+    await window.fetch(serverPath + '/discord/auth', {


seems confusing to have the same POST call do delete. Maybe make a separate DELETE verb call to the same endpoint?

src/components/Modal/ProfileModal.tsx

Argn0 · 2022-02-27T02:45:57Z

The important settings are

SERVERMEMBERS INTENT toggled on in bot settings
In the generated authentication URL the response_type=code must be changed to response_type=token
Bot Permissions: Manage roles
OAuth2 URL scope : identify

howardchung · 2022-02-27T16:45:29Z

server/server.ts

+    return res.sendStatus(500);
+  }
+  try {
+    const result = await postgres?.query(


i think you can delete and return the deleted content?

I don't think it returns the deleted rows. The rows array is always empty. Are you sure it does? Maybe I'm missing a query method argument?

you need to add RETURNING to your query

howardchung · 2022-02-27T16:46:12Z

server/server.ts

@@ -278,6 +384,19 @@ app.delete('/deleteAccount', async (req, res) => {
  }
  if (postgres) {
    await postgres?.query('DELETE FROM room WHERE owner = $1', [decoded.uid]);
+    const result = await postgres?.query(


we can just delete here and not change role?

howardchung · 2022-02-27T16:47:42Z

server/server.ts

+      [decoded.email]
+    );
+    if (result.rowCount) {
+      await discordBot.assignRole(


This is the link action? we don't need to remove any roles?

if you pass the argument undo=true to assignRole it removes the role. Maybe this is a bit confusing?

Yeah i'm asking why we need to remove the role here

Oh I see. Ok we will leave out all role removing stuff.

To answer the question: We remove the role here because we want to limit the linked discord accounts per email to max 1 . if we dont remove the role here someone could theoretically assign a sub role to an unlimited number of accounts.

Would that be fine?

But we defer this cleanup to later, right?

howardchung · 2022-02-27T16:48:24Z

sql/schema.sql

@@ -25,6 +25,13 @@ CREATE TABLE subscriber(
  PRIMARY KEY(uid)
 );

+CREATE TABLE discord(


any more thoughts on making this a generic link table that can support linking other accounts?

Sounds finde to me. Do you have any naming suggestion for the table?

howardchung · 2022-02-27T16:49:21Z

src/index.tsx

@@ -109,11 +116,16 @@ class WatchParty extends React.Component {
              );
            }}
          />
+          <Route path="/discord/auth" exact>


Can we make this a modal on the app rather than its own route? or is a new path required for the discord auth?

The user will redirected to the URL you choose when generating the OAuth2 URL. In our case the user will be redirected to watchparty.me/discord/auth . So I think we need this new path.

Argn0 · 2022-02-27T21:49:11Z

Alright, I believe I've addressed everything.

Argn0 · 2022-03-01T23:43:40Z

I tried doing this in a modal instead of a new window but Discord is blocking iframes and I don't know if there is another method.

howardchung · 2022-03-02T01:21:31Z

src/components/Modal/ProfileModal.tsx

+                onClick={this.authDiscord}
+              >
+                <Icon name="discord" />
+                Link Discord Account


let's say "Link Discord Account/Get Subscriber Role"?

That looked a little awkward because it's too long. I've added a popup instead.

src/components/Modal/ProfileModal.tsx

howardchung · 2022-05-25T06:39:15Z

server/server.ts

@@ -57,6 +59,23 @@ if (config.DATABASE_URL) {
  postgres.connect();
 }

+let discordBot: DiscordBot;


This should probably run in a separate service

Actually, that might make the endpoint logic more complicated. But we may need to scale to multiple instances of the server soon, and I'm not sure if that will work if multiple processes are logging into the same Discord bot

What do you think would be the best way to do the communication between the server processes and the discord process?

Maybe we could just use a redis queue?

But that might not be necessary because as far as I can tell it is possible to access the same bot through multiple different client instances using the same token.

My preferred solution is usually to set up an HTTP server on the bot and have the web instances call it. But if you think it might be OK to scale out, then we can try that first

I tried it out by starting the dev server multiple times. Which I guess comes close to having multiple instances of the same server.

Argn0 added 2 commits February 25, 2022 17:37

Discord Bot

88f88c9

Discord Bot

8581e11

howardchung reviewed Feb 25, 2022

View reviewed changes

Argn0 added 3 commits February 26, 2022 18:24

review

6e7bcd3

config

f9da636

remove comment

dc06ab2

Argn0 added 4 commits February 26, 2022 18:34

Merge branch 'master' into 2

3b03fb8

Update ProfileModal.tsx

f14da5d

handle one discord acc linked with multiple email

440d77c

remove condition

ef9cd5d

howardchung reviewed Feb 26, 2022

View reviewed changes

server/server.ts Outdated Show resolved Hide resolved

howardchung reviewed Feb 27, 2022

View reviewed changes

src/components/Discord/Discord.tsx Show resolved Hide resolved

howardchung reviewed Feb 27, 2022

View reviewed changes

src/components/Modal/ProfileModal.tsx Show resolved Hide resolved

Argn0 added 3 commits February 27, 2022 03:26

review2

3fbca67

review 2

42ddc9b

fix delete account

768de32

Update Discord.tsx

f0f7693

howardchung reviewed Feb 27, 2022

View reviewed changes

review

db88a77

Argn0 added 3 commits February 27, 2022 22:53

schema

e32bc94

subscriber modal

4683671

remove if else

1b6bb9c

howardchung reviewed Mar 2, 2022

View reviewed changes

src/components/Modal/ProfileModal.tsx Show resolved Hide resolved

Argn0 added 4 commits March 2, 2022 03:06

review

0333f46

review

2601976

only show when discord_auth_url is set

9ce92ed

Update config.ts

4ed2752

howardchung reviewed May 25, 2022

View reviewed changes

Merge branch 'master' into 2

240785d

howardchung mentioned this pull request Feb 5, 2023

[wip] discord admin bot #690

Merged

Discord Bot #417

Are you sure you want to change the base?

Discord Bot #417

Conversation

Argn0 commented Feb 25, 2022 • edited Loading

Choose a reason for hiding this comment

Argn0 commented Feb 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Argn0 commented Feb 26, 2022

Argn0 commented Feb 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Argn0 commented Feb 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Argn0 commented Feb 27, 2022

Argn0 commented Mar 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Argn0 commented Feb 25, 2022 •

edited

Loading

Argn0 commented Feb 26, 2022 •

edited

Loading