core: added feature flag and removed signals. #109
Conversation
alejandromumo
force-pushed
the
add_feature_flag
branch
from
1360d0f to
2752bdd
Compare
alejandromumo
force-pushed
the
add_feature_flag
branch
from
1ede6c2 to
c31676c
Compare
| """Return OAuth access token.""" | ||
| if self.user_id: | ||
| return RemoteToken.get(self.user_id, self.remote.consumer_key).access_token | ||
| return self.remote.get_request_token()[0] |
There was a problem hiding this comment.
minor: this might be needed for when the user links their OAuth GitHub account, after which we have the special account setup handler that goes to GitHub and syncs repository info. During this request the GitHub RemoteToken is not committed yet in the database, but is temporarily stored somewhere in the Flask request (this needs a deep-dive in the get_request_token() method to verify).
There was a problem hiding this comment.
I think we're using this token getter:
It sets the RemoteToken in the session and returns the RemoteToken.token(), which is a tuple: https://github.com/inveniosoftware/invenio-oauthclient/blob/master/invenio_oauthclient/models.py#L140
Since both are returning RemoteToken.get(), they should be similar. However the get_request_token() sets it in the session, if it exists.
Nevertheless, I will revert the change :)
invenio_github/api.py
Outdated
|
|
||
| def disable_repo(self, repo): | ||
| """Disables all current user repositories.""" | ||
| assert repo.user_id == self.user_id |
There was a problem hiding this comment.
minor/question: Is this check transferred over from the models? Since we're in the "business layer" Instead of an assert I would go with raising an exception (that the caller can then reliably catch and handle).
There was a problem hiding this comment.
Yes, it comes from the model. Before we were checking it on access (e.g. Repository.get()) and that was moved to the business logic.
This "disable_repo" was moved from the disconnect handler, to wrap the repo functionality in an API call.
I realised we also have the permission check to access the repo and that can be reused here.
There was a problem hiding this comment.
However, since we are hiding the enabling/disabling actions behind the API class and checking for permissions it can have an impact on support. E.g. we'd have to impersonate a user to enable/disable the repo
invenio_github/oauth/handlers.py
Outdated
| for repo in db_repos: | ||
| Repository.disable(repo) | ||
| # Disable every GitHub webhooks from our side | ||
| repos = github.user_repositories |
There was a problem hiding this comment.
| repos = github.user_repositories | |
| repos = github.user_repositories.all() | |
| repos_with_hooks = [(r.github_id, r.hook) for r in repos if r.hook] |
We need the .all() here to "cache" the results, because after we disable each repo, this information will be lost, and we need it to delete the remaining repo hooks.
There was a problem hiding this comment.
Also, we were already iterating the repos so it could've been done in one iteration.
Thanks!
closes #107