Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mentioning by guild nickname #147

Merged
merged 19 commits into from
Nov 14, 2016
Merged

Conversation

SparxySys
Copy link
Collaborator

resolves issue #137

@coveralls
Copy link

coveralls commented Nov 9, 2016

Coverage Status

Coverage decreased (-1.3%) to 97.222% when pulling 6770cf7 on DarkSpyro003:server-nicknames into 0d75a01 on reactiflux:master.

@coveralls
Copy link

coveralls commented Nov 9, 2016

Coverage Status

Coverage decreased (-1.3%) to 97.222% when pulling e5efeb8 on DarkSpyro003:server-nicknames into 0d75a01 on reactiflux:master.

Test to make sure you can't mention someone by username if their nickname differs
if (userDetails) {
return userDetails.nickname || user.username;
}
return user.username;
Copy link
Collaborator Author

@SparxySys SparxySys Nov 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check if the last line is actually ever reachable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is reached when sent by a webhook. Looks like it resolves #146

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@coveralls
Copy link

coveralls commented Nov 9, 2016

Coverage Status

Coverage decreased (-1.3%) to 97.222% when pulling dcd8f4f on DarkSpyro003:server-nicknames into 0d75a01 on reactiflux:master.

Copy link
Member

@ekmartin ekmartin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, looks great! Had one small comment, looks good to go other than that.

if (user) {
const serverUser = guild.members.find('id', user.id);
if (serverUser.nickname && serverUser.nickname !== search) {
// User has a guild nickname which is different from the search query
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, but maybe it wouldn't be a terrible idea to just let people be highlighted by usernames as well?
Nonetheless, I think this could be refactored slightly by returning early instead. Example:

const nickUser = guild.members.find('nickname', search);
if (nickUser) {
  return nickUser;
}

const user = this.discord.users.find('username', search);
if (user && guild.members.find('id').nickname === search) {
  return user;
}

return match;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ekmartin I'm not sure that's a good idea. The people on IRC would be unable to see what the usernames of users on Discord are, and could possibly even inadvertently mention someone they weren't trying to mention.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to change this refactor around a bit so it still works for people without nicknames.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't guild.members.find('id').nickname just be null/undefined for users without nicknames? If so you'd just be comparing search to that anyway, which would never be true since search is the match from your regex - right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ekmartin If user found by username and nickname = null it means the "visible nickname" is a match and should return true.


it('should not convert username mentions from IRC if nickname differs', function () {
const testUser = new discord.User(this.bot.discord, { username: 'testuser', id: '123', nickname: 'somenickname' });
this.findUserStub.withArgs('username', testUser.username).returns(testUser);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice sinon usage!

if (userDetails) {
return userDetails.nickname || user.username;
}
return user.username;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@coveralls
Copy link

coveralls commented Nov 14, 2016

Coverage Status

Coverage decreased (-0.5%) to 97.931% when pulling 0d9177c on DarkSpyro003:server-nicknames into 0d75a01 on reactiflux:master.

@ekmartin ekmartin merged commit bb5b9a8 into reactiflux:master Nov 14, 2016
@ekmartin
Copy link
Member

I've added you as a collaborator @DarkSpyro003 - thanks for the help :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants