[feature] Automatically open threads for messages in specific channels #164

RogueArt · 2022-04-20T23:27:49Z

Purpose of pull request

Related Issue: resolves #157

Automatically opens a new thread for career questions
Automatically opens a new thread for valid resume review submissions
- Checks if a resume review submission has an image or PDF, otherwise deletes it

Pull request checklist

Branch and PR are named correctly
Updated relevant documentation
Tested with a tester bot
Wrote unit tests for changes

NOTE: Please do not merge this before merging into these two PRs

Upgrade Discord.js to v13: [feature] Upgrade Discord.js to v13 #161
Small PR to fix linting not working on Windows: [bug] Fix Windows linting errors #163

Screenshots

Career Questions:

Resume Review:

Testing instructions

Run unit tests using yarn test
Set up locally and designate two channels, one as a career question channel and one as a resume review channel to test manually

payne911

I love the demo GIF! I like your style.

payne911 · 2022-04-21T01:32:30Z

src/bots/monitor/index.js

+      // Verify a resume is attached to message
+      if (message.attachments.size === 0) {
+        author.send(defaultStrings.noResume);
+        message.delete();
+        return;
+      }
+
+      // Verify that all attachments are in valid format for a resume
+      if (!message.attachments.every((file) => attachmentIsAllowed(file))) {
+        author.send(defaultStrings.invalidFormat);
+        message.delete();
+        return;
+      }


The DM should include the text the user posted and which got deleted. This way, if they had written a massive wall-of-text, it's not lost.

Great idea, I'll add that to the next revision

Zacheriah

Looks great! I added some feedback on some lines I thought you could expand your implementation.

Zacheriah · 2022-04-20T23:35:41Z

src/bots/monitor/index.js

+  // Run routine based on id
+  switch (message.channelId) {
+    // Career questions channel
+    case get('CAREER_QUESTIONS_CHANNEL_ID'):


Consider adding a case for when a person replies to another message at the top-level instead of in a thread. (as a reply instead of as a standalone message). Also consider returning the message they sent in the dm you send them.

That's a really smart idea, will do both. Thanks!

Zacheriah · 2022-04-20T23:38:07Z

src/bots/monitor/index.js

+
+      // Verify that all attachments are in valid format for a resume
+      if (!message.attachments.every((file) => attachmentIsAllowed(file))) {
+        author.send(defaultStrings.invalidFormat);


Consider sending the author of the invalid message a copy of their message when you delete it so they can repost it easily with the correct format.

That would save a lot of frustration. Will add that in the next revision, thanks for mentioning!

RogueArt · 2022-04-21T20:40:38Z

Made a new revision with the following changes:

Replies to a top-level comment/thread will be auto-deleted
Deleted replies will have their message contents AND attachments sent to the user as a direct message
- Added code to handle the edge case where the content length is >2000 characters

Here's a gif demonstrating the new features:

payne911

Side-note: I would rename the PR

[feature] Automatically open threads for messages in specific channels

payne911 · 2022-04-22T15:35:44Z

.env.template

+CAREER_QUESTIONS_CHANNEL_ID=0987654321
+RESUME_REVIEW_CHANNEL_ID=0987654321
+SHARE_YOUR_CONTENT_CHANNEL_ID=0987654321


As discussed on Discord, I believe the auto-thread feature should also be applied to #share-your-content and #hiring

That's a lot more specific. Renamed the PR to that.

Will add both channels to the code in the next revision.

Because it's fairly dynamic I think it would be better to have a database entry of like a 1 col table where you can add/remove channels by command. Never know if we plan to add or remove channels from this practice

crabbycrabby · 2022-04-23T06:22:36Z

.husky/pre-commit

@@ -1,4 +1,5 @@
 #!/bin/sh
 . "$(dirname $0)/_/husky.sh"

-yarn format && yarn lint


This is a breaking change, as previously yarn format failing would cause pre-commit to fail, and it does not anymore. Probably want set -x on verification scripts like this.

That's a good catch. I tested yarn format && yarn lint on Windows PowerShell just now and it seems to be working now while it wasn't earlier. I've added it #163, will need to drop the old commit from this pull request once #161 and #163 are merged.

…annels

RogueArt · 2022-04-24T01:00:59Z

Made a new revision adding support for hiring and share your question channels along with some unit tests. Verified that the current functionality for career questions and resume review still works:

payne911

The yarn comment from @crabbycrabby is beyond me, so I can't speak for that.

But the rest looks good to me. (Although my revision is very much on the surface since I am mostly a Java dev.)

I'm submitting an Approval so that my previous "Request changes" doesn't block this PR.

nikgil · 2022-04-24T15:29:27Z

Can you try pull and merge with development branch?

nikgil

Overall fine imo. Just needs the mod bypass stuff. Also the dynamic suggestion is more something for a future diff if the time comes to it. Also I merged your last diff in so this may present conflicts weith dev branch.

nikgil · 2022-04-24T15:31:16Z

.env.template

+CAREER_QUESTIONS_CHANNEL_ID=0987654321
+RESUME_REVIEW_CHANNEL_ID=0987654321
+SHARE_YOUR_CONTENT_CHANNEL_ID=0987654321


Because it's fairly dynamic I think it would be better to have a database entry of like a 1 col table where you can add/remove channels by command. Never know if we plan to add or remove channels from this practice

nikgil · 2022-04-24T15:33:32Z

src/bots/monitor/index.js

+        if (isMessageReply(message)) {
+          author.send(defaultStrings.invalidReply(channelName));
+          deleteAndReturnMessage(message);
+          return;
+        }


Need bypass for mods.

nikgil · 2022-04-24T15:36:43Z

src/bots/monitor/index.js

+      break;
+
+    // Resume review channel
+    case getChannelIdForName(CHANNEL_NAMES.RESUME_REVIEW):


I get the reasoning behind all this, but referring to my more dynamic suggestion I think this is a single breakaway case that can be dealt with like this,

RogueArt added 10 commits April 20, 2022 13:52

Upgrade Discord.js to v13 and bump up Jest version

0f246bd

Fix broken test after upgrade

b508b64

Bump up Node version everywhere

e86d988

Fix linting error

db60fcc

Fix ESLint and allow pre-commit hook to run on Windows

582f515

Support new channel IDs

73d1cc6

Update ESLint to prevent yarn lint / yarn format conflict

870eb81

Automatically open threads for career questions and resume review

d6d6de2

Add unit tests for valid attachment

9bb13f2

Add unit tests for valid attachment

bf308b8

payne911 suggested changes Apr 21, 2022

View reviewed changes

Zacheriah suggested changes Apr 21, 2022

View reviewed changes

Resend message to user, delete any top-level replies

ab47504

payne911 suggested changes Apr 22, 2022

View reviewed changes

RogueArt changed the title ~~[feature] Automatically open threads on comments~~ [feature] Automatically open threads for messages in specific channels Apr 22, 2022

crabbycrabby reviewed Apr 23, 2022

View reviewed changes

RogueArt added 3 commits April 23, 2022 17:57

Updated environment to add hiring and share-your-content channels

4cac0f5

Add thread automatically opening for hiring and share-your-content ch…

1463d8b

…annels

Add unit tests after supporting new channels

62044e5

payne911 approved these changes Apr 24, 2022

View reviewed changes

nikgil reviewed Apr 24, 2022

View reviewed changes

[feature] Automatically open threads for messages in specific channels #164

Are you sure you want to change the base?

[feature] Automatically open threads for messages in specific channels #164

Uh oh!

Conversation

RogueArt commented Apr 20, 2022

Purpose of pull request

Pull request checklist

Screenshots

Testing instructions

Uh oh!

payne911 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zacheriah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RogueArt commented Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

payne911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RogueArt commented Apr 24, 2022

Uh oh!

payne911 left a comment

Choose a reason for hiding this comment

Uh oh!

nikgil commented Apr 24, 2022

Uh oh!

nikgil left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

payne911 left a comment •

edited

Loading

RogueArt commented Apr 21, 2022 •

edited

Loading

nikgil left a comment •

edited

Loading