Move the checks of users existence and activity to hubot-routines #164

ya7on · 2019-01-28T17:14:08Z

close #158
close #161

Please make sure all below checkboxes in the Mandatory section have been checked before submitting your PR (also don't forget to pay attention to the Depending on Circumstances section)

Mandatory

The commit message is an imperative and starts with a capital letter (for example, Add something, Remove something, Fix something, etc.).
I made sure I hadn't put a period at the end of the the commit message(s).

Depending on Circumstances

Some of the items in the section become mandatory if you are a first-time contributor and/or your changes are relevant to the items.

Adding a new parameter I didn't forget to reflect it in README.md.
Modifying a list of constants or imports I didn't brake an alphabetical order.
(for first-time contributors) One of the commits in the pull request adds me to the Acknowledgements section in AUTHORS.md.

polina-popova · 2019-02-08T08:52:23Z

src/birthder.js

        const username = msg.message.user.name
-        const user = Object.values(brain).filter(item => item.name === username).shift()
+        const user = (await routines.getAllUsers(robot)).filter(item => item.name === username).shift()


Looks like you should use findUserByName instead.

polina-popova · 2019-02-08T08:58:51Z

src/birthder.js

-      userNames = users.map(user => `@${user.name}`)
-      message = `${userNames.join(', ')}`
+      const userNames = users.map(user => `@${user.name}`)
+      const message = `${userNames.join(', ')}`



I am not sure about it - first you delete block of variable declaration, then add it again in other place. May be will be less changes and they will be more clear if you change type of variables in block upper?

polina-popova · 2019-02-08T09:05:17Z

src/birthder.js

-      let users = []
-
-      if (!await routines.isAdmin(robot, msg.message.user.name.toString())) {
+      if (!await routines.isAdmin(robot, msg.message.user.name)) {


This string looks like refactoring and not relative to the commit message refactoring.

polina-popova · 2019-02-08T09:07:41Z

src/birthder.js

-        }
-      }
+      const name = msg.match[2].trim()
+      const user = await routines.findUserByName(robot, name)


And again you combine small changes with moving block of variable declaration.

polina-popova

Let's exclude refactoring from the PR.

polina-popova · 2019-02-12T12:59:01Z

src/birthder.js

      } else {
        return msg.send(`I have never met ${name}.`)
      }
    })

    // Print the users names whose birthdays match the specified date.
    robot.respond(routes.check, async (msg) => {
+      let allUsers
      let date
      let message
      let users = []


Now the variable users doesn't need the initial value.

polina-popova · 2019-02-12T13:01:00Z

src/birthder.js

@@ -204,16 +181,9 @@
        title = `First working days`
      }

-      let message


Leave message here to not redefine it lower.

polina-popova · 2019-02-12T13:02:49Z

src/birthder.js


      if (!await routines.isAdmin(robot, msg.message.user.name.toString())) {
        msg.send(utils.MSG_PERMISSION_DENIED)
        return
      }

-      name = msg.match[2].trim()
-      date = msg.match[3]


Let's don't rewrite date and name variables.

polina-popova · 2019-02-12T13:04:14Z

src/utils.js

-      users.push(bdayUser)
-    }
-  }
+  const targetDay = moment().add(-exp.BIRTHDAY_CHANNEL_TTL, 'day')


Let's don't refactor targetDay variable.

polina-popova

The reminding about users without birthday is broken.

ya7on changed the title ~~Refactor isUserActive~~ [WIP] Refactor isUserActive Jan 28, 2019

ya7on force-pushed the user-active-list branch 2 times, most recently from 21ac052 to b800b89 Compare January 30, 2019 13:26

ya7on force-pushed the user-active-list branch from b800b89 to dea94db Compare February 6, 2019 10:02

ya7on changed the title ~~[WIP] Refactor isUserActive~~ Move the checks of users existence and activity to hubot-routines Feb 6, 2019

ya7on force-pushed the user-active-list branch from dea94db to 620fdf6 Compare February 6, 2019 13:13

polina-popova reviewed Feb 8, 2019

View reviewed changes

ya7on force-pushed the user-active-list branch 2 times, most recently from 70900aa to 1127c33 Compare February 12, 2019 11:14

polina-popova reviewed Feb 12, 2019

View reviewed changes

ya7on force-pushed the user-active-list branch 3 times, most recently from 2fbc2fb to 0d15a39 Compare February 13, 2019 14:34

polina-popova suggested changes Feb 25, 2019

View reviewed changes

ya7on force-pushed the user-active-list branch from 0d15a39 to aea3376 Compare February 26, 2019 15:16

Refactor isUserActive

aea3376

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move the checks of users existence and activity to hubot-routines #164

Move the checks of users existence and activity to hubot-routines #164

ya7on commented Jan 28, 2019 •

edited

Loading

polina-popova Feb 8, 2019

polina-popova Feb 8, 2019

polina-popova Feb 8, 2019

polina-popova Feb 8, 2019

polina-popova left a comment

polina-popova Feb 12, 2019

polina-popova Feb 12, 2019

polina-popova Feb 12, 2019

polina-popova Feb 12, 2019

polina-popova left a comment

Move the checks of users existence and activity to hubot-routines #164

Are you sure you want to change the base?

Move the checks of users existence and activity to hubot-routines #164

Conversation

ya7on commented Jan 28, 2019 • edited Loading

Please make sure all below checkboxes in the Mandatory section have been checked before submitting your PR (also don't forget to pay attention to the Depending on Circumstances section)

Mandatory

Depending on Circumstances

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

polina-popova 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

polina-popova left a comment

Choose a reason for hiding this comment

ya7on commented Jan 28, 2019 •

edited

Loading