Thanks command improvement #83

Closed
wants to merge 8 commits into
from

Projects

None yet

4 participants

@abhisekp
Member
abhisekp commented Apr 16, 2016 edited

Improved thanks command implementation

- **Using `lodash` for better performance due to lazy evaluation** - **Discards non-existing users and self while thanking and thank only existing ones** (which also fixes thanking `@/all` as a bonus) i.e. if a user doesn't exist in gitter, then there will be no API calls to thank that user.

Command

thanks @camperbot @/all @all @belindarogers

Bot Response

@abhisekp sends brownie points to @camperbot and @belindarogers 👍
⚠️ could not find receiver for belindarogers
⭐️ 1560 | @camperbot | http://www.freecodecamp.com/camperbot

  • Improved bot response about giving brownie points

OLD

abhisekp sends brownie points to @rphares and @qmikew1 and @dwd2010 and @camperbot and @csegate2 👍

NEW

@abhisekp sends brownie points to @GitterBotX, @camperbot, @ltegman, @SaintPeter and @noncentz 👍



Tested in GitterBotX/playground


NOTE: Added dedent module which removes unwanted indentations from template strings. Please use npm install to install the new package 📦 .

Fixes #76
Fixes dcsan/gitterbot#170

@abhisekp abhisekp referenced this pull request Apr 16, 2016
Merged

Fix/thanks #78

@abhisekp abhisekp commented on an outdated diff Apr 16, 2016
lib/bot/cmds/thanks.js
- if (namesList.length > 0) {
- const toUserMessage = namesList.join(' and @');
- return '> ' + fromUser + ' sends brownie points to @' + toUserMessage +
- ' :sparkles: :thumbsup: :sparkles: ';
- } else {
- return '> sorry ' + fromUser + ', you can\'t send brownie points to ' +
- 'yourself! :sparkles: :sparkles: ';
- }
+ const thankList = _.chain(mentions)
+ .uniq('screenName')
+ .filter((user) =>
+ user.screenName.toLowerCase() !== fromUser.toLowerCase()
+ && !!user.userId
+ )
+ .map((user) => user.screenName)
+ .value();
@abhisekp
abhisekp Apr 16, 2016 edited Member

Please move the thankList iteration to the end of the method and no need to unwrap it before. This ensures best lazy evaluation performance.

@abhisekp abhisekp commented on an outdated diff Apr 16, 2016
lib/bot/cmds/thanks.js
- return '> ' + fromUser + ' sends brownie points to @' + toUserMessage +
- ' :sparkles: :thumbsup: :sparkles: ';
- } else {
- return '> sorry ' + fromUser + ', you can\'t send brownie points to ' +
- 'yourself! :sparkles: :sparkles: ';
- }
+ const thankList = _.chain(mentions)
+ .uniq('screenName')
+ .filter((user) =>
+ user.screenName.toLowerCase() !== fromUser.toLowerCase()
+ && !!user.userId
+ )
+ .map((user) => user.screenName)
+ .value();
+
+ thankList.forEach((toUser) => {
@abhisekp
abhisekp Apr 16, 2016 edited Member

Move this to the end as this will be executed asynchronously.

@abhisekp abhisekp commented on an outdated diff Apr 16, 2016
lib/bot/cmds/thanks.js
+ });
+
+ let message = '';
+
+ const THANKLIST_SIZE = thankList.length;
+ if (THANKLIST_SIZE > 0) {
+ const $symboledThankedList = _.chain(thankList)
+ .map((user) => `@${user}`);
+
+ const $lastUser = $symboledThankedList.last();
+ const $initialUsers = $symboledThankedList.take(THANKLIST_SIZE - 1);
+
+ let thankedUsersMsg = $initialUsers.value().join(', ');
+ // if more than one user is thanked
+ // then append an "and"
+ if (thankedUsersMsg !== '') {
@abhisekp
abhisekp Apr 16, 2016 edited Member

Please use a better logic so that it is obvious and appropriate what you're doing here.

e.g.

$initialUsers.size() > 0
@abhisekp
abhisekp Apr 17, 2016 edited Member

You may reuse the THANKLIST_SIZE constant here which would be more appropriate and better i.e.

THANKLIST_SIZE > 1
@abhisekp abhisekp commented on an outdated diff Apr 16, 2016
lib/bot/cmds/thanks.js
@@ -21,37 +23,63 @@ const thanksCommands = {
const mentions = input.message.model.mentions;
// just 'thanks' in a message
- if (mentions && mentions.length === 0) {
+ if (_.isEmpty(mentions)) {
Utils.warn('thanks', 'without any mentions', input.message.model);
return null;
@abhisekp
abhisekp Apr 16, 2016 edited Member

Use a consistent return value such as '' (empty string) as primary return value is a type of string. Please test it thoroughly so that consumer of this method handles this appropriately and there is no silent bugs.

If returning null, then mention in the documentation of the method. (And yes, please add documentation)

@abhisekp abhisekp commented on an outdated diff Apr 16, 2016
lib/bot/cmds/thanks.js
- return userList;
- }, []);
-
- if (namesList.length > 0) {
- const toUserMessage = namesList.join(' and @');
- return '> ' + fromUser + ' sends brownie points to @' + toUserMessage +
- ' :sparkles: :thumbsup: :sparkles: ';
- } else {
- return '> sorry ' + fromUser + ', you can\'t send brownie points to ' +
- 'yourself! :sparkles: :sparkles: ';
- }
+ const thankList = _.chain(mentions)
+ .uniq('screenName')
+ .filter((user) =>
+ user.screenName.toLowerCase() !== fromUser.toLowerCase()
+ && !!user.userId
@abhisekp
abhisekp Apr 16, 2016 Member

It would be nice if you could make this logic more intuitive and obvious (English-like?)
i.e. make this more intuitive ~-> !!user.userId

@abhisekp abhisekp commented on an outdated diff Apr 16, 2016
lib/bot/cmds/thanks.js
+ const $initialUsers = $symboledThankedList.take(THANKLIST_SIZE - 1);
+
+ let thankedUsersMsg = $initialUsers.value().join(', ');
+ // if more than one user is thanked
+ // then append an "and"
+ if (thankedUsersMsg !== '') {
+ thankedUsersMsg += ' and ';
+ }
+
+ thankedUsersMsg += $lastUser.value();
+ message += dedent`
+ > @${fromUser} sends brownie points to ${thankedUsersMsg} :sparkles: :thumbsup: :sparkles: `;
+ }
+
+ if (mentions.find(
+ (user) => user.screenName.toLowerCase() === fromUser.toLowerCase())
@abhisekp
abhisekp Apr 16, 2016 Member

Change variable name to toUser instead of user

(toUser) =>
@abhisekp abhisekp commented on an outdated diff Apr 16, 2016
lib/bot/cmds/thanks.js
- '&giver=' + fromUser;
- HttpWrap.callApi(apiPath, options, thanksCommands.showInfoCallback);
- userList.push(toUser);
- }
- return userList;
- }, []);
-
- if (namesList.length > 0) {
- const toUserMessage = namesList.join(' and @');
- return '> ' + fromUser + ' sends brownie points to @' + toUserMessage +
- ' :sparkles: :thumbsup: :sparkles: ';
- } else {
- return '> sorry ' + fromUser + ', you can\'t send brownie points to ' +
- 'yourself! :sparkles: :sparkles: ';
- }
+ const thankList = _.chain(mentions)
@abhisekp
abhisekp Apr 16, 2016 Member

Please add comments above the line detailing about what this chained invoking of the functions does.

@abhisekp abhisekp commented on an outdated diff Apr 17, 2016
lib/bot/cmds/thanks.js
+ user.screenName.toLowerCase() !== fromUser.toLowerCase()
+ && !!user.userId
+ )
+ .map((user) => user.screenName)
+ .value();
+
+ thankList.forEach((toUser) => {
+ const apiPath = `/api/users/give-brownie-points?receiver=${toUser.toLowerCase()}&giver=${fromUser.toLowerCase()}`;
+ HttpWrap.callApi(apiPath, options, thanksCommands.showInfoCallback);
+ });
+
+ let message = '';
+
+ const THANKLIST_SIZE = thankList.length;
+ if (THANKLIST_SIZE > 0) {
+ const $symboledThankedList = _.chain(thankList)
@abhisekp
abhisekp Apr 17, 2016 edited Member

Instead of $symboledThankedList, you may name it as $taggedThankList or $mentionedThankList

@SaintPeter
Member

Any progress?

As per ☝️ This Comment in HelpContributors

There is a new user with the name E-Tank - and just entering a reply or comment on one of their questions automatically gives them brownie points.

@abhisekp
Member
abhisekp commented Apr 24, 2016 edited

@SaintPeter that is addressed by @noncentz in https://github.com/FreeCodeCamp/camperbot/pull/78

Just need to remove the line linked here and then that can be merged.

@ltegman
Member
ltegman commented May 1, 2016

I'm really confused what is going on in this thread. @abhisekp Is this ready? I see you've written a bunch of critiques on your own PR so I'm not really sure what the status of this is.

@abhisekp
Member
abhisekp commented May 1, 2016

@ltegman it is not ready yet.. I've to implement the suggestions. Bit busy these days... Will correct them ASAP and ping you. 👍

abhisekp added some commits Apr 16, 2016
@abhisekp abhisekp Add dedent module 📦 b766b80
@abhisekp abhisekp Modify thanks method
- Use lodash
- Discard non-existing users and self and thank only existing ones (which also fixes thanking @/all as a bonus)
 E.g. if a user doesn't exist in gitter, then there will be no API calls to thank that user.
76a658b
@abhisekp abhisekp Add .editorconfig 9476bd7
@abhisekp abhisekp Improved thanks more
- fixed some messages
- formatted source a bit
04b9b6e
@abhisekp abhisekp Fix tests and format 0644cae
@abhisekp abhisekp Add editorconfig 7160f9b
@abhisekp
Member

🙅 DO NOT MERGE 🙅

abhisekp added some commits May 18, 2016
@abhisekp abhisekp Ava is love 💟 c26c6e1
@abhisekp abhisekp Remove unwanted comment 54f5305
@raisedadead raisedadead added the blocked label Jul 20, 2016
@raisedadead
Member

@abhisekp I know you are busy, but this would be great to have this soon. ;)

@ltegman
Member
ltegman commented Oct 16, 2016

Closing as stale. Please reopen when this is ready to be merged.

@ltegman ltegman closed this Oct 16, 2016
@ltegman ltegman removed the blocked label Oct 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment