Refactor bot commands #23

zbraniecki · 2016-10-14T16:35:35Z

No description provided.

zbraniecki · 2016-10-15T22:56:32Z

stasm

Thanks for the PR, @zbraniecki! For now I left a few general comments about the coding style. I would also like to spend some time thinking about the log collection. Perhaps an in-memory list would suffice for now? What would you say to that?

Would you also mind editing the initial comment in this PR and explaining the goals of this change and summarize what's new?

stasm · 2016-10-16T13:10:34Z

api/src/restful.js


  router.use(cors);
  router.use(bodyParser.json());

  router.route('/reports').get(reports.get(reportsColl));
  router.route('/reports/current').get(reports.current(reportsColl));
  router.route('/reports').post(reports.create(reportsColl));
+  router.route('/reports/remove/:id').post(reports.remove(reportsColl));


Use HTTP DELETE verb here?

stasm · 2016-10-16T13:11:25Z

api/src/restful.js

  router.route('/updates/:id').post(updates.update(updatesColl));
  router.route('/resolve').post(updates.resolve(updatesColl));

+  router.route('/log').get(log.get(logColl));
+  router.route('/log').post(log.insert(logColl));


Why do you feel a mongo collection is needed for the log? I'd guess keeping the log in memory should be easier and sufficient for now. When we want to ensure persistency we can add Redis or something similar.

The main reason is ability to debug actions. db log seems like a good place to store all actions and be able to come back and rewind later if needed looking into each operation.

Especially with ability to undo things it may be later hard for the maintainer to understand what has led to the state which you're debugging. Log may help.

Also, I'm not sure what's hard about log?

One thought I had was that logging could happen in the API app.

I'm not against in-memory logging per-se, I just don't think it would be easier and it would limit the value of log to just this single use-case (reverting action in bot).

This also enables #10

No, I think #10 is enabled by aggregating the updates collection. I have a WIP.

I'd like to avoid increasing the complexity of the code for as long as possible. Also, let's try to avoid over-engineering the code by designing for future requirements. If an in-memory array serves the current purpose well, I say let's go with it, please. It's simple and synchronous.

stasm · 2016-10-16T13:24:56Z

api/src/updates.js

@@ -60,18 +68,27 @@ function getUpdates(coll, raw) {
 function createUpdate(coll, body) {
  delete body._id;
  const d = new Date();
-  return coll.insert(Object.assign(body, {
+
+  let o = Object.assign(body, {


stasm · 2016-10-16T13:52:04Z

api/src/updates.js

-      ({ops}) => res.json(ops[0])
-    ).catch(console.error);
+    createUpdate(coll, req.body)
+      .then(resp => res.json(resp))


We rely on the API returning the full update object to assign not only the id but also proper creation dates.

stasm · 2016-10-16T13:53:01Z

bot/src/bot.js

-  'at your service',
-  'sounds good'
-];
+import { Commands } from './commands/index';


from './commands'; should be sufficient here.

stasm · 2016-10-16T13:54:11Z

bot/src/bot.js

-    return Promise.resolve('that\'s not a valid date');
+  if (message.startsWith('test ')) {
+    message = message.substr(5);
+    test = true;


Maybe delegate this flow to a separate function, testCommand?

stasm · 2016-10-16T14:03:48Z

bot/src/commands/add-inbox-item.js

+
+  matches: (str) => {
+    return true;
+  },


Consider using the shorthand method definitions:

matches(str) { return true; }

…om the bot

…ds in separate file and use API

zbraniecki · 2016-10-18T08:16:21Z

@stasm moves log to in-memory

stasm

Alright, this is getting really close and I really like the change overall. Thanks!

stasm · 2016-10-18T18:56:40Z

api/src/reports.js

@@ -64,8 +75,17 @@ function createReport(coll, body) {
  const month = reportDate.getUTCMonth() + 1;
  const day = reportDate.getUTCDate();
  const slug = `${year}-${pad(month)}-${pad(day)}`;
-  return coll.insert(Object.assign(body, {
+
+  let o = Object.assign(body, {


Please use const here.

stasm · 2016-10-18T18:58:08Z

api/src/reports.js

-  }));
+  });
+  return coll.insertOne(o).then(() => {
+    return o._id;


Can we keep this consistent with createUpdate and return the whole object? I'd like to revisit this in the near future and in fact I'd prefer to return the id like here, but for now I vote consistency.

stasm · 2016-10-18T18:58:36Z

api/src/restful.js


  router.route('/updates').get(updates.get(updatesColl));
  router.route('/updates').post(updates.create(updatesColl));
+  router.route('/updates/delete/:id').post(updates.remove(updatesColl));


What are the benefits of POST over DELETE here?

Idk, I don't know how delete works vs. post.

Here you'll need to use the delete method:

router.route('/updates/:id').delete(updates.remove(updatesColl));

And then probably write a new helper in utils.js:

export function delete(url) { return fetch(url, { method: 'DELETE', // not sure if headers are required }).then(resp => resp.json()); }

stasm · 2016-10-18T18:59:20Z

api/src/updates.js

+  });
+  return coll.insertOne(o).then(() => {
+    return o;
+  })


In such cases please use an implicit-return syntax:

return coll.insertOne(o).then(() => o);

stasm · 2016-10-18T19:00:49Z

bot/src/bot.js

-  if (isNaN(ts)) {
-    return Promise.resolve('that\'s not a valid date');
+function parseCommand(bot, author, channel, message) {
+  console.log(message);


Would you mind changing this to something like:

console.log(` --- got a command: ${message}`);

It was for debugging, I removed it. You're logging the message in the listener anyway.

stasm · 2016-10-18T19:26:17Z

bot/src/commands/create-report.js

+        command: CreateReport.name,
+        id: resp,
+      });
+    }).then(


Please use the implicit-return syntax.

stasm · 2016-10-18T19:28:39Z

bot/src/bot.js

-  );
+  for (let command of bot.commands) {
+    if (command.matches(message)) {
+      return command.execute(bot, author, channel, message);


Please handle errors from execute with console.error here. You'll need to return Promise.reject() from commands in case of errors.

stasm · 2016-10-18T19:29:32Z

bot/src/bot.js

+    if (!this.actionLog.has(hash)) {
+      this.actionLog.set(hash, []);
+    }
+    const log = this.actionLog.get(hash);


Use getActionLog() here?

stasm · 2016-10-18T19:33:32Z

bot/src/commands/revert-last-command.js

+      bot.logAction(author, channel, {
+        command: RevertLastCommand.name,
+        id: entry.object.id
+      });


Is there a reason to add it to the log if it's not revertable?

stasm · 2016-10-18T19:36:13Z

bot/src/commands/revert-last-command.js

+import { get, post } from '../utils';
+import { Commands } from './index';
+
+function getCommandByName(name) {


If you change commands' names to match the names of the exports (or write a dash-to-pascal-case helper), you can do:

import * as commands from './index';

and:

return commands[name];

I can't. The babel then break all iterations over the result object. I can't for ... of, and I can for ... in, but with an additional __esModule property which I have to filter out.

I changed commands names, but I left it as import/export.

Ah, I see, that's a shame. I wonder why Babel doesn't make this additional property non-enumerable.

zbraniecki · 2016-10-19T06:26:12Z

ok, I think I'm done with this round. :) Please re-review and land at will :)

stasm · 2016-10-19T09:01:19Z

bot/src/bot.js

+    if (command.matches(message)) {
+      return command.execute(bot, author, channel, message).catch(e => {
+        console.log(`Command ${command.name} failed with error "${e}"`);
+      });


console.error and no braces, please:

return command.execute(bot, author, channel, message).catch( e => console.error(`Command ${command.name} failed with "${e}"`) );

stasm · 2016-10-19T09:04:40Z

bot/src/commands/revert-last-command.js

+import { get, post } from '../utils';
+import { Commands } from './index';
+
+function getCommandByName(name) {


Ah, I see, that's a shame. I wonder why Babel doesn't make this additional property non-enumerable.

stasm · 2016-10-19T09:06:24Z

bot/src/commands/revert-last-command.js

+    const log = bot.getActionLog(author, channel);
+    if (log.length === 0) {
+      return Promise.resolve('There is no command to revert');
+    }


A nit here and in a few other places: please insert a blank line after all terminating if blocks.

stasm · 2016-10-19T09:18:55Z

bot/src/bot.js

+    const hash = `${user}-${channel}`;
+    if (!this.actionLog.has(hash)) {
+      this.actionLog.set(hash, []);
+    }


Does it make sense to move this.actionLog.set(hash, []); into getActionLog too and remove the above 4 lines?

then, every attempt to retrieve values for non-existing key will create it.

yeah, I know, I thought that maybe that was okay since it's not a common scenario, but on second thought it's probably better to keep it the way you wrote it right now. thanks!

stasm · 2016-10-19T15:42:31Z

zbraniecki force-pushed the bot-to-cards branch from e397f5c to 030a0ac Compare October 15, 2016 22:56

zbraniecki force-pushed the bot-to-cards branch from 030a0ac to 058224b Compare October 15, 2016 23:09

stasm requested changes Oct 16, 2016

View reviewed changes

Zibi Braniecki added 9 commits October 17, 2016 15:14

Add a number of commands to allow placing items in the right boxes fr…

c845f15

…om the bot

more work on commands including logging, reversion and testing

b647cbf

Remodel commands and bot to be stateful, refactor code to have comman…

630d970

…ds in separate file and use API

Remove last reference to mongo from the bot

ecaffae

Make logging great again

5a8e068

Readd revert and improve logging

a29179b

Finalize the commands

a7087ed

address feedback

f924b01

Move log to in-memory storage

b70bf09

zbraniecki force-pushed the bot-to-cards branch from 7b2e43e to b70bf09 Compare October 17, 2016 23:05

stasm requested changes Oct 18, 2016

View reviewed changes

Zibi Braniecki added 2 commits October 18, 2016 23:22

Address stas feedback

1a049a6

more feedback addressed

882514c

stasm reviewed Oct 19, 2016

View reviewed changes

stasm requested changes Oct 19, 2016

View reviewed changes

Zibi Braniecki added 2 commits October 19, 2016 08:07

more nits

a5e7c80

switch to http delete

d18f698

stasm changed the title ~~Bot to cards~~ Refactor bot commands Oct 19, 2016

stasm approved these changes Oct 19, 2016

View reviewed changes

stasm merged commit d01ec9c into stasm:master Oct 19, 2016

stasm added the project-bot label Jan 24, 2017

stasm modified the milestone: 0.1 Jan 24, 2017

stasm removed project-bot project-www labels Jan 24, 2017

Refactor bot commands #23

Refactor bot commands #23

Conversation

zbraniecki commented Oct 14, 2016

zbraniecki commented Oct 15, 2016

stasm left a 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

zbraniecki commented Oct 18, 2016

stasm left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbraniecki commented Oct 19, 2016

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

stasm commented Oct 19, 2016