Update help channel embed when channel is occupied #117

smichel17 · 2021-03-19T17:56:37Z

Context in commits. The code is mostly done but completely untested, because I haven't got the bot set up yet (need to request a Discord bot api key, etc). I'll probably get to that soon, but I also wanted to open a dialog here in case there's any changes you'd like me to make (and to make sure you're interested in merging, before spending too much time on this).

@gencha

https://discord.com/channels/508357248330760243/508357683506708514/821851432235433984 ^As @gencha typescript-community#69 points out, once a channel has been claimed, there's still a big 'ol embed just a little ways up that describes the channel as available -- which might contribute to help-channel sniping. This adds a new embed which marks the channel as occupied. It does not use it anywhere yet; that'll come soon.

Pull updateStatusEmbed out into a helper function while we're at it, used for changing the status from dormant to available, too. Occupied => dormant doesn't need to update the status since it always adds a new message. Maybe it doesn't need to when someone deletes their question, though..

It used to check against the available embed contents, but there's a new embed in town! We also need to check against the title now, because the occupied embed body changes depending on who claimed the channel.

src/modules/helpchan.ts

robertt · 2021-03-19T18:49:30Z

Looks good! Can I see a screenshot of the updated embed?

src/modules/helpchan.ts

smichel17 · 2021-03-19T19:29:43Z

Can I see a screenshot of the updated embed?

As soon as I get it running 😄

Along those lines — this is my first time doing Discord bot development, so I've not read over all features of discord.js. If there's something that would make sense for me to use and I'm not, it's probably because I haven't read that part of the docs yet and don't know about it, not an intentional choice :)

src/modules/helpchan.ts

When changing the channel status, the bot will update a previous embed if it's in the bottom 5 messages, rather than sending a new message. However, we were detecting *any* embed from the bot, rather than only status messages, so it would also replace playground links, for example. This detects status embeds by comparing their titles. In order to do that, it adds a title to the dormant embed. This isn't *necessary*; the following code would work instead: const isStatusEmbed = (embed: MessageEmbed) => ( embed.title === this.AVAILABLE_EMBED.title || embed.title === this.OCCUPIED_EMBED_BASE.title || embed.description === this.DORMANT_EMBED.description ); However, this way is both cleaner code-wise, and I think it's nice for all the status embeds to have the same format.

smichel17 · 2021-03-19T22:58:18Z

Got the bot up and running, but there's some permission errors and it's not creating channels for me like I believe it should. Not sure if I've misconfigured it. Hopefully I can figure that out soon or someone else who's gotten the bot to work in the past can try running this branch. In any case, it's working enough for me to get a screenshot of the embed:

Takeaways:

Mentions work!
- Not in the footer — I'll use their display name there instead
- "To @person" looks awkward — I'll drop the "To ".

@person

- "To @person" looks awkward; dropped the "To " - Mentions don't work in the footer, just use the username instead

smichel17 · 2021-03-20T13:37:25Z

The embed looks good to me now.

Fixed a few configuration problems but I still haven't gotten the bot to connect to postgres. It hangs at the await here ("Connected to DB" is never logged). Or it crashes with UnhandledPromiseRejectionWarning: Error: The server does not support SSL connections, but I think that's just if I give it an invalid url for postgres.

community-bot/src/db.ts

Lines 8 to 26 in a4c9050

    
           export async function getDB() { 
        
           	if (db) return db; 
        
           	db = await createConnection({ 
        
           		type: 'postgres', 
        
           		url: dbUrl, 
        
           		synchronize: true, 
        
           		logging: false, 
        
           		entities: [RepUser, RepGive, HelpUser], 
        
           		ssl: true, 
        
           		extra: { 
        
           			ssl: { 
        
           				rejectUnauthorized: false, 
        
           			}, 
        
           		}, 
        
           	}); 
        
           	console.log('Connected to DB'); 
        
           	return db; 
        
           }

Thus, I can't test everything yet, although the bits that I have been able to test seem to be working as intended.

robertt · 2021-03-20T13:39:12Z

I know the cause - sorry. The database provider we use made SSL required and I did a quick hotfix so we could get the help channels back online. If you remove these lines it'll work fine:

 		ssl: true, 
 		extra: { 
 			ssl: { 
 				rejectUnauthorized: false, 
 			}, 
 		},

do you think you could put it behind a process.env.NODE_ENV === 'production'?

robertt · 2021-03-20T13:41:20Z

Also, I really like these:

I think we can remove the tsplay.dev, since we offer our own playground shortener.

smichel17 · 2021-03-20T14:54:43Z

ok, I found the reason I was having trouble: for some reason, the very first character of the db url is dropped. process.env.DATABASE_URL is correct, so it seems like a typeOrm bug. I worked around it with

DATABASE_URL=" tscbot:tscbot@localhost:5432/tscbot"

smichel17 · 2021-03-20T15:34:47Z

The database provider we use made SSL required and I did a quick hotfix so we could get the help channels back online

This worked — should I add it to this PR or send another?

robertt · 2021-03-20T16:04:20Z

This worked — should I add it to this PR or send another?

Feel free to put it in this PR

src/db.ts

When running the bot locally, for development, SSL is an unnecessary extra setup step that makes it harder to get started.

robertt · 2021-03-20T16:29:05Z

Just tested locally, works well. Could you change the bullet points to the ones you came up with? After that we can do a full review and get it merged 👍

tjjfvi · 2021-03-20T17:06:01Z

IMO the tsplay.dev link is still useful because the bot won't shorten links over 2000 chars (discord message limit). It might be worthwhile for the bot to download message.txt files, check if they're playground links, and shorten them with tsplay.dev automatically.

Turns out the array of messages is sorted with the most recent message first. By reversing the array, the bot would sometimes edit a higher-up status embed rather than the most recent one.

smichel17 · 2021-03-20T23:08:18Z

How's this for an updated message?

const occupiedMessage = (asker: User) => `
**This channel is claimed by ${asker.toString()}.**

It is dedicated to answering their questions only.

**${asker.toString()} These things make your questions easier to answer:**
• Describe what you want to happen, and what happens instead.
• Copy-paste a short snippet (5-15 lines) that shows the problem.
• Start code blocks with \`\\\`\`ts for syntax highlighting.
• Try to reproduce your problem in the [TypeScript Playground](https://www.typescriptlang.org/play).

Usually someone will try to answer and help solve the issue within a few hours. If not, you may ping the @Helper role and ask what would make your question more likely to get an answer.

For more tips, check out StackOverflow's guide on **[asking good questions](https://stackoverflow.com/help/how-to-ask)**.
`;

Updates the text to reflect things we commonly tell people with regard to asking good questions.

src/modules/helpchan.ts

The message cache does not have a stable order (I suspect it is simply the order in which the messages were added to the cache). Thus, when searching for the latest status message, sometimes a message other than the latest was picked. In practice, it seems like this only ever happened when the bot was offline for a number of hours, which is why it hasn't come up in production. But in development, it could be reproduced by: - Leave the bot offline for a while (24h?) - Start the bot - Claim a channel - The channel that is moved up from being dormant has the wrong message edited, so the message at the bottom still says "dormant". I suspect this is what the `.reverse()`, which was removed in commit 09ad491, was intended to solve (but it caused bugs during normal operation). This should be a full fix. I am not sure of the performance implications here. I think it should not be too bad, because we are filtering to only messages from the bot, before sorting.

smichel17 · 2021-03-22T19:39:15Z

I think this is ready to be reviewed / merged. I updated the embed to be like this:

I think 5c87f89 will have a minor performance impact, but I'm not sure how long the bot keeps messages in the cache. In the future, I'm interested in pinning both the asked question and the bot's status message — this would also make the code for figuring out which message to update simpler. But I'll send that in a separate PR (if you're interested).

robertt · 2021-03-22T19:40:30Z

Yeah, it should be fine, the performance impact will be minimal.

tjjfvi · 2021-03-22T19:47:38Z

src/modules/helpchan.ts

@@ -39,6 +42,20 @@ This channel will be dedicated to answering your question only. Others will try
 For more tips, check out StackOverflow's guide on **[asking good questions](https://stackoverflow.com/help/how-to-ask)**.
 `;

+const occupiedMessage = (asker: User) => `
+**This channel is claimed by ${asker.toString()}.**
+It is dedicated to answering their questions only.


A little late on this, but maybe

Suggested change

It is dedicated to answering their questions only.

It is dedicated to answering their questions only; see <#${askHelpChannelId}> to claim your own.

Added just "More info: <#${askHelpChannelId}>", so it will fit on one line. Two lines distracts a little from the "This channel is claimed by" message on the line above, in my opinion (fixable by adding a newline, but then the message starts to become longer than I think it should be).

ckiee

Looks pretty good! Just one small suggestion that can possibly clean it up a bit.

src/modules/helpchan.ts

smichel17 · 2021-03-22T20:03:28Z

I'm busy for a few hours, will likely get to this tonight or tomorrow morning

Based on code review & feedback: - Drop explicit `.toString()` in the template. - Add a link to the how-to-get-help channel. - Be explicit that code blocks > screenshots. - Be explicit that it may be impossible to reproduce in the playground.

It was setting the person who wrote "!claim", rather than the person who the channel was being claimed for.

robertt · 2021-03-23T18:23:47Z

This looks done to me. @smichel17 @ronthecookie ready to merge?

ckiee

LGTM

smichel17 · 2021-03-23T20:10:40Z

Ready on my end. I have a handful of things I still want to do (e.g. didn't add back the channel topic change) but there's no reason they have to be in this PR.

robertt · 2021-03-23T20:20:03Z

🚀

smichel17 added 4 commits March 18, 2021 20:41

Update help status message when channel is claimed

3ff4e46

Update checkEmptyOngoing to reflect occupied embed

4d51ec2

It used to check against the available embed contents, but there's a new embed in town! We also need to check against the title now, because the occupied embed body changes depending on who claimed the channel.

tjjfvi reviewed Mar 19, 2021

View reviewed changes

src/modules/helpchan.ts Outdated Show resolved Hide resolved

tjjfvi reviewed Mar 19, 2021

View reviewed changes

src/modules/helpchan.ts Show resolved Hide resolved

tjjfvi reviewed Mar 19, 2021

View reviewed changes

src/modules/helpchan.ts Outdated Show resolved Hide resolved

Properly generate mentions in occupied help embed

4816e50

smichel17 commented Mar 19, 2021

View reviewed changes

src/modules/helpchan.ts Outdated Show resolved Hide resolved

Clean up occupied embed

15bbbf0

- "To @person" looks awkward; dropped the "To " - Mentions don't work in the footer, just use the username instead

robertt reviewed Mar 20, 2021

View reviewed changes

src/db.ts Outdated Show resolved Hide resolved

robertt approved these changes Mar 20, 2021

View reviewed changes

Only require SSL DB connection in production

16239be

When running the bot locally, for development, SSL is an unnecessary extra setup step that makes it harder to get started.

smichel17 force-pushed the update-embed branch from 3e88073 to 16239be Compare March 20, 2021 16:24

Bugfix: edit the most recent embed, not oldest

09ad491

Turns out the array of messages is sorted with the most recent message first. By reversing the array, the bot would sometimes edit a higher-up status embed rather than the most recent one.

Update Occupied Help Channel status message embed

9c71469

Updates the text to reflect things we commonly tell people with regard to asking good questions.

smichel17 changed the title ~~WIP: Update help channel embed when channel is occupied~~ Update help channel embed when channel is occupied Mar 22, 2021

tjjfvi reviewed Mar 22, 2021

View reviewed changes

src/modules/helpchan.ts Outdated Show resolved Hide resolved

smichel17 force-pushed the update-embed branch from 5c87f89 to 7a617bf Compare March 22, 2021 19:36

robertt requested a review from ckiee March 22, 2021 19:40

tjjfvi reviewed Mar 22, 2021

View reviewed changes

ckiee requested changes Mar 22, 2021

View reviewed changes

src/modules/helpchan.ts Outdated Show resolved Hide resolved

smichel17 added 2 commits March 23, 2021 13:53

Update help channel occupied status message

b8baf21

Based on code review & feedback: - Drop explicit `.toString()` in the template. - Add a link to the how-to-get-help channel. - Be explicit that code blocks > screenshots. - Be explicit that it may be impossible to reproduce in the playground.

Fix: set !claim command sets embed asker correctly

c18e1f7

It was setting the person who wrote "!claim", rather than the person who the channel was being claimed for.

smichel17 force-pushed the update-embed branch from 903186a to c18e1f7 Compare March 23, 2021 18:14

ckiee approved these changes Mar 23, 2021

View reviewed changes

Merge branch 'master' into update-embed

995ce85

robertt merged commit f828a7f into typescript-community:master Mar 23, 2021

smichel17 mentioned this pull request Mar 24, 2021

Fix: Don't crash if a message has no author #125

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update help channel embed when channel is occupied #117

Update help channel embed when channel is occupied #117

smichel17 commented Mar 19, 2021

robertt commented Mar 19, 2021

smichel17 commented Mar 19, 2021

smichel17 commented Mar 19, 2021

smichel17 commented Mar 20, 2021 •

edited

Loading

robertt commented Mar 20, 2021 •

edited

Loading

robertt commented Mar 20, 2021 •

edited

Loading

smichel17 commented Mar 20, 2021

smichel17 commented Mar 20, 2021

robertt commented Mar 20, 2021

robertt commented Mar 20, 2021

tjjfvi commented Mar 20, 2021

smichel17 commented Mar 20, 2021

smichel17 commented Mar 22, 2021 •

edited

Loading

robertt commented Mar 22, 2021

tjjfvi Mar 22, 2021

smichel17 Mar 23, 2021 •

edited

Loading

ckiee left a comment

smichel17 commented Mar 22, 2021

robertt commented Mar 23, 2021

ckiee left a comment

smichel17 commented Mar 23, 2021

robertt commented Mar 23, 2021

	It is dedicated to answering their questions only.
	It is dedicated to answering their questions only; see <#${askHelpChannelId}> to claim your own.

Update help channel embed when channel is occupied #117

Update help channel embed when channel is occupied #117

Conversation

smichel17 commented Mar 19, 2021

robertt commented Mar 19, 2021

smichel17 commented Mar 19, 2021

smichel17 commented Mar 19, 2021

smichel17 commented Mar 20, 2021 • edited Loading

robertt commented Mar 20, 2021 • edited Loading

robertt commented Mar 20, 2021 • edited Loading

smichel17 commented Mar 20, 2021

smichel17 commented Mar 20, 2021

robertt commented Mar 20, 2021

robertt commented Mar 20, 2021

tjjfvi commented Mar 20, 2021

smichel17 commented Mar 20, 2021

smichel17 commented Mar 22, 2021 • edited Loading

robertt commented Mar 22, 2021

tjjfvi Mar 22, 2021

Choose a reason for hiding this comment

smichel17 Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

ckiee left a comment

Choose a reason for hiding this comment

smichel17 commented Mar 22, 2021

robertt commented Mar 23, 2021

ckiee left a comment

Choose a reason for hiding this comment

smichel17 commented Mar 23, 2021

robertt commented Mar 23, 2021

smichel17 commented Mar 20, 2021 •

edited

Loading

robertt commented Mar 20, 2021 •

edited

Loading

robertt commented Mar 20, 2021 •

edited

Loading

smichel17 commented Mar 22, 2021 •

edited

Loading

smichel17 Mar 23, 2021 •

edited

Loading