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

labhub.py: Alert when @mention is not in room #630

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

abhishalya
Copy link
Member

The fix introduces removal of is_room_member
function and instead uses callback_message to warn
whenever @mention is not in room.

Closes #603

Reviewers Checklist

  • Appropriate logging is done.
  • Appropriate error responses.
  • Handle every possible exception.
  • Make sure there is a docstring in the command functions. Hint: Lookout for
    botcmd and re_botcmd decorators.
  • See that 100% coverage is there.
  • See to it that mocking is not done where it is not necessary.

@abhishalya
Copy link
Member Author

Obviously the tests will fail as I have removed the is_room_member function.

Copy link
Member

@jayvdb jayvdb left a comment

Choose a reason for hiding this comment

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

tests need to be fixed.

@abhishalya
Copy link
Member Author

@jayvdb Can you please check the warning message so that I can make changes to the tests?

@jayvdb
Copy link
Member

jayvdb commented Oct 23, 2018

tests need to be fixed.

Shouldn't that be a new issue?

No. Fixing/adding unit tests are an integral part of changing code.

The fix introduces removal of is_room_member
function and instead uses callback_message to warn
whenever `@mention` is not in room.

Closes coala#603
@abhishalya
Copy link
Member Author

Blocked until errbotio/err-backend-gitter#38 is solved.

@@ -193,6 +185,26 @@ def callback_message(self, msg):
self.send(msg.frm, response)
self.hello_world_users.add(user)

mentioned = []
room_members = msg.frm.room.occupants
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to fetch the people once reducing the number of API calls and eventually update the list as soon as there are new people in the room, will improve this later on once we've fixed backend.

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

Successfully merging this pull request may close these issues.

Alert whenever a @mention is of someone not in the room
3 participants