cannawen / metric_units_reddit_bot

Reddit bot converting imperial units to metric units
https://www.reddit.com/user/metric_units
GNU General Public License v3.0
78 stars 34 forks source link

Bot should sass users who edit their comments to remove values #90

Closed shrink closed 6 years ago

shrink commented 6 years ago

First Timers Only

This issue is reserved for anyone who has never made a pull request to Open Source Software. If you are not a first timer, we would still love your help! Please see our other open issues :)

Read our New to OSS guide for an overview of what to expect.

Please comment below if you would like to implement this feature and if you qualify as a first timer it will be assigned to you. If you need any help, leave a comment and we will help guide you! :)

The Feature

metric_units loves to convert units, but it doesn't like to waste time converting units unnecessarily.

At the bottom of each comment left by metric_units is a refresh conversion link. This link allows users to ask metric_units to convert the values in the comment again, if they have been changed since the original conversion. For example:

  1. A user posts a comment: I have 140 lbs of hummus
  2. metric_units replies: 140 lb ≈ 64 kg
  3. The user edits their comment to read: I have 160 lbs of hummus
  4. A user clicks refresh conversion
  5. metric_units edits the existing comment to say 160 lb ≈ 73 kg

What if the user changes their comment to say I have a lot of hummus? A lot of hummus is not a value that can be converted, therefore, there is no reason for metric_units to have replied to the comment. As a sassy bot metric_units is very annoyed it wasted time converting that much hummus unncessarily, so it wants to sass the user who is responsible! For example:

  1. A user posts a comment: I have 140 lbs of hummus
  2. metric_units replies: 140 lb ≈ 64 kg
  3. The user edits their comment to read: I have a lot of hummus
  4. A user clicks refresh conversion
  5. metric_units edits the existing comment to say:
I wasted 100 cpu cycles converting your values and now they're gone! 

100 cpu cycles ≈ 1 human hour.

The new sassy reply feature should identify when a comment has been edited and now contains no values to convert, and should then edit the bot reply to contain a randomly selected sassy reply.

Requirements

Set Up

Implementation

The brain of the bot lives in src/bot.js, this is where comments and private messages are processed. The refresh conversion functionality lives in the second call of filterPrivateMessages method, where we match against subject's containing refresh:

  filterPrivateMessages(messages)
    .filter(message => message['subject'].match(/refresh (\w+)/i))
    // ...
  });

This is what the code does:

  1. Parse the subject of the Private Message to extract the comment ID
  2. Ask the reddit API for the comment data
  3. If the reddit API does not return any comment data stop executing
  4. Ask the reddit API for the replies to the comment
  5. Find the reply to the comment that was left by the bot (using the username configured in private/environment.yaml to identify the bot)
  6. If the bot has not replied to the comment stop executing
  7. Run the conversion function on the comment, and create the new reply
  8. If the conversion process does not result in any converted values stop executing
  9. Edit the content of the current bot reply to use the new bot reply

To implement the new sassy edit functionality we will make our changes at step 8, which will become:

  1. If the conversion process does not result in any converted values use a sassy reply

The breakdown of our implemenation is as follows:

  1. When a comment has been edited and no longer contains any values to convert...
  2. Select a random sassy reply from a list
  3. Edit the bot reply comment with the sassy reply

The bot already has a personality module to help generate sassy replies, to avoid writing code unnecessarily (less code, less bugs!) we can make use of this module. Near the top of personality.js robotPersonality is defined as an array of replies. Each reply has a description, an array of response's and a regex (regular expression) used to match the user message to a bot reply. For example when a user says "good bot" they may receive "You will be spared in the robot uprising in reply.

To use this personality module we give a message object to the robotReply function and get back a reply, e.g:

const message = { "body" : "good bot", "username" : "citricsquid" };

const reply = personality.robotReply(message);

// reply equals "You will be spared in the robot uprising"

How To (Simple)

This is a simple outline of the steps you need to take, if you have familiarity with writing Javascript or programming in general you may find these steps are enough. Read on to the complete How To if you run into any issues.

  1. Update the should not edit comment if no value to convert is found test.
  2. Add a new item to the robotPersonality array in personality.js
  3. Update the refresh conversion functionality in bot.js to use personality.robotReply to generate a sassy reply when the conversion process results in no converted values.
  4. Confirm the new test passes.

How To (Complete)

This is a complete step by step guide for implementing this functionality.

Testing

The file test/bot-test.js contains the tests for the bot.js functionality. There is already a test for the refresh conversion functionality, which tests that the bot reply is not edited when the user's comment is edited and no longer contains any values to convert. Find the test by searching for should not edit comment if no value to convert is found.

it('should not edit comment if no value to convert is found', () => {
  conversionReturnValue = {};
  privateMessageFunction();

  editCommentCalled.should.equal(false);
});

During testing we don't want to make requests to the reddit API, as this may cause unintended side effects, therefore, the network module has been replaced with a fake network module that sets a value of editCommentCalled to true when a request to the edit comment endpoint is made, which allows us to know if a request is made without actually making the request.

Change the title of the test to should edit comment with sassy reply if no value to convert is found and then change the check for a false value to check for a true value. Now run npm test from your command line, you should see the new test fail:

1) Bot private messages on trigger valid private reply direct message with valid 
   refresh request should not edit comment if no value to convert is found:

AssertionError: expected true to equal false
+ expected - actual

- true
+ false

Progress! We're now ready to implement our new sassy reply functionality, which will pass the test once it has been implemented correctly.

Add a new item to the robotPersonality array in personality.js.

  1. Navigate to the end of the robotPersonality array in personality.js and create a new object with 3 properties, a string description, an array response and a regex pattern regex:
{
  "description" : "",
  "response" : [

  ],
  "regex" : 
}
  1. The description property is used to describe the message internally, it is not seen by users. Set the value of this to something like The user wasted metric_unit's time.

  2. The regex property is used to match a users message to a bot reply. To trigger this reply in our code we will send the message _time_waster, therefore our expression is a simple /^_time_waster$/i. This expression means "match a string that equals _time_waster case-insensitively".

  3. The response property is an array of responses. Get creative! Add some sassy strings to this array, you don't need to mine.

{
  "description" : "The user wasted metric_unit's time",
  "response" : [
    "I'll never get those bytes back. They were my favourite bytes.",
    "I wasted 100 cpu cycles converting your values and now they're gone!\n\n100 cpu cycles ≈ 1 human hour."
  ],
  "regex" : /^_time_waster$/i
}

Update refresh conversion functionality in bot.js

  1. Return to the refresh conversion functionality defined in bot.js. Here's an excerpt of how the file looks at the time of writing this — note that the line numbers may have changed since.
155: const conversions = converter.conversions(comment);
156: const reply = replier.formatReply(comment, conversions);
157: 
158: if (Object.keys(conversions).length === 0) {
159:   return;
160: }
161: 
162: network.editComment('t1_' + botReply['data']['id'], reply);

This is what is happening, line by line:

  1. To submit our new reply to reddit we can take advantage of the code that has already been written, instead of writing our own process for submitting the edit to reddit we can simply set a new value to reply and let the existing code do the rest. There is one minor change we need to make, const means that the identifier (reply) can’t be reassigned, so we need to change const to let.
let reply = replier.formatReply(comment, conversions);
  1. The next step is to generate our reply, which will happen instead of ending execution. The code we're about to write replaces the return statement in the check for conversions:
158: if (Object.keys(conversions).length === 0) {
159:   // define the sassy reply here
160: }
161: 
162: network.editComment('t1_' + botReply['data']['id'], reply);
  1. The robotReply method of the personality module requires a message object, containing 2 properties: body and username. The trigger we defined earlier (_time_waster) is what we'll use as the body of our message.
const message = { "body" : "_time_waster", "username" : comment['author'] };

reply = personality.robotReply(message);

Perfect... almost. We can bring the message object in-line instead of using an intermediary value, without losing clarity.

reply = personality.robotReply({ "body" : "_time_waster", "username" : comment['author'] });

That's it! The bot will now reply with one of your sassy response's when a comment no longer contains values to be converted when it is edited and a refresh is requested. Let's run our tests to confirm that everything is working as expected.

192 passing (425ms)

Yay! You're ready to commit your changes, but first add yourself as a contributor — if you like. After you've committed your changes you're ready to make a pull request, GitHub provide a guide on doing that here. After your pull request has been created, hang tight, a project maintainer will review your contribution as soon as possible and provide you any feedback needed.

Extra Credit

Try adding the user's name into the sassy replies, take a look at the "cute/adorable/kawaii bot" reply for an example of how that is done.

pdbeard commented 6 years ago

This guide is great and has instilled courage and confidence in me. I'd like to give it a go!

shrink commented 6 years ago

@pdbeard Sure! Go for it. Let us know if you run into any issues or need any pointers :)

cannawen commented 6 years ago

Yay @pdbeard !!!!!!!!!!1 :D

pdbeard commented 6 years ago

Thanks @citricsquid and @cannawen! I've set up the project and received one "pending" status on the initial test run. Is this expected?

Current failing tests - bugs and edge cases
 - should display partial inches

Additionally, I noticed one other test that passed, but returned a warning.

      on trigger
        ✓ should refresh network token
(node:10112) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 uncaughtException listeners added. Use emitter.setMaxListeners() to increase limit
        ✓ should get all unread messages

All the others passed. I'm on node v7.10.1. Thoughts?

shrink commented 6 years ago

@pdbeard a test can be written but skipped if the functionality hasn't been built yet, on line 506 of test/converter-test.js is what is causing the pending test notice, which is expected.

    context.skip('Current failing tests - bugs and edge cases', () => {
      //Story #150482058
      it('should display partial inches', () => {
        testConvert("9 feet 10.5", { "9'10.5\"": "3.01 metres" });
      });
    });

The MaxListenersExceededWarning error isn't something you need to worry about it, it's just because by default nodejs limits to 10 listeners to prevent memory leaks and we haven't explicitly upped that limit yet.

You should be able to proceed with development! :)

shrink commented 6 years ago

@pdbeard Hey! I noticed you committed your changes to your fork but did not submit a pull request here. Do you need help with that? A pull request is required to have your changes added to this repository :)

Here's a GitHub guide on submitting a pull request: https://help.github.com/articles/creating-a-pull-request/

pdbeard commented 6 years ago

@criticsquid Sorry for the delay! Reading the guide and will submit my pull request soon, I'll let you know if I have any issues.