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

And/refactor rocketchat #23

Merged
merged 8 commits into from
May 15, 2018
Merged

And/refactor rocketchat #23

merged 8 commits into from
May 15, 2018

Conversation

andrey-canon
Copy link
Contributor

@andrey-canon andrey-canon commented Apr 27, 2018

Description

Refactor

Reviewers

@codecov-io
Copy link

codecov-io commented Apr 27, 2018

Codecov Report

Merging #23 into master will increase coverage by 33.26%.
The diff coverage is 99.45%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #23       +/-   ##
===========================================
+ Coverage   60.39%   93.66%   +33.26%     
===========================================
  Files           5        6        +1     
  Lines         505      742      +237     
===========================================
+ Hits          305      695      +390     
+ Misses        200       47      -153
Impacted Files Coverage Δ
rocketc/openedx_dependencies.py 0% <ø> (ø) ⬆️
rocketc/api_teams.py 97.05% <100%> (+57.05%) ⬆️
rocketc/tests/test_rocketChat.py 100% <100%> (ø) ⬆️
rocketc/api_rocket_chat.py 100% <100%> (ø)
rocketc/rocketc.py 80.09% <97.39%> (+37.6%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c48844...3066989. Read the comment docs.

self.server_url = server_url
self._login(user, password)

def _login(self, user, password):
Copy link
Contributor

Choose a reason for hiding this comment

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

better if we name this method _login_admin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any user can do login in rocketchat , if you want to use this api in another context you could use with any user but any user can not use all the methods that depends on the user's configuration in rocketchat

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, you're right, in fact what we are doing in this method is to get api tokens, so in that order of ideas the method name could be changed. But definitively this method is not exclusive for admins, is for users who have enough privileges to communicate with the API's

self._login(user, password)

def _login(self, user, password):
""""""
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add docstring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

LOG.info("Auth_token: %s, User_id: %s ", self.headers[
"X-Auth-Token"], self.headers["X-User-Id"])

def _request_rocket_chat(self, method, url_path, data=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

important to mention here that admin credentials are used to perform these requests

LOG.info("Request rocketChat status code = %s", response.status_code)
return response.json()

def add_to_group(self, user_id, room_id):
Copy link
Contributor

Choose a reason for hiding this comment

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

better add_user_to_group? what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it

LOG.info("Method Add to Group: %s with this data: %s", response, data)
return response

def change_role(self, user_id, role):
Copy link
Contributor

Choose a reason for hiding this comment

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

change_user_role

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it

def _request_rocket_chat(self, method, url_path, data=None):
return False

def _update_user(self, user_id, user_data):
Copy link
Contributor

Choose a reason for hiding this comment

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

please, brief docstring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok


def _add_to_group(self, user_id, room_id):
def _join_groups(self, user_id, user_data):
Copy link
Contributor

Choose a reason for hiding this comment

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

the method name should be "_join_user_to_configuration_groups"? The desition of which groups the user should be added to is taken based on xblock configuration


LOG.info("Login method: result create token: %s", data)

return data

def create_token(self, username):
def _add_to_course_group(self, group_name, user_id):
Copy link
Contributor

Choose a reason for hiding this comment

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

_add_user_to_course_group?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

team = api.get_user_team(course_id, username)
print team
LOG.info("Get Team response: %s", team)
if team:
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, here I see the user can have more than one team

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The user only can be in one team, in this case the response is an array with zero or one element

LOG.info("Add to team group response: %s", response)
return response["success"]

def _get_team(self, username, course_id):
Copy link
Contributor

Choose a reason for hiding this comment

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

_get_user_lms_team ??

@jfavellar90
Copy link
Contributor

And finally just correct tests :)

@jfavellar90
Copy link
Contributor

jfavellar90 commented May 9, 2018

Please bump version and merge @andrey-canon

@jfavellar90 jfavellar90 merged commit 9631a6b into master May 15, 2018
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.

3 participants