CI/CD update + a bug found by pytest-flakes? #26
Conversation
References: - Class dependencies: - jupyterhub.handlers.BaseHandler: https://github.com/jupyterhub/jupyterhub/blob/abb93ad799865a4b27f677e126ab917241e1af72/jupyterhub/handlers/base.py#L69 - tornado.web.RequestHandler: https://www.tornadoweb.org/en/stable/web.html#tornado.web.RequestHandler - Function dependencies: - login_user: https://github.com/jupyterhub/jupyterhub/blob/abb93ad799865a4b27f677e126ab917241e1af72/jupyterhub/handlers/base.py#L696-L715 - get_next_url: https://github.com/jupyterhub/jupyterhub/blob/abb93ad799865a4b27f677e126ab917241e1af72/jupyterhub/handlers/base.py#L587 - get_body_argument: https://www.tornadoweb.org/en/stable/web.html#tornado.web.RequestHandler.get_body_argument - Function overrided: jupyterhub.handlers.BaseHandler.post: https://github.com/jupyterhub/jupyterhub/blob/abb93ad799865a4b27f677e126ab917241e1af72/jupyterhub/handlers/base.py#L1289-L1313 Needs verification: 1. The user object was not consumed, perhaps it should be passed to the get_next_url function but wasn't. I made it so for now. 2. I also named parameters passed to get_body_argument to make it easier to read what it is about. 3. Is it correct that this function signature should only be (self)? 4. Is it correct that this function should not be decorated with something like @web.authenticated like the other BaseHandler functions like get(self, user_name, user_path) is?
consideRatio
changed the title
consideRatio
force-pushed
the
setup-travis-pypi-cd
branch
from
f85acc8
to
1dd9dc4
Compare
| body_argument = self.get_body_argument( | ||
| name='custom_next', | ||
| default=next_url, | ||
| ) |
There was a problem hiding this comment.
Considerations
- The user variable was never considered, should it be? The get_next_url has logic to consider a passed user object as a fallback to other logic. While I'm not confident this will or won't be relevant. I think it could be a bugfix of New user logins not effective when old user is logged in #11 to pass the user to get_next_url.
- I named parameters passed to get_body_argument to make it easier to read what it is about. I also verified that this was the naming some versions back as well to avoid potentially making this compatible by being explicit here.
There was a problem hiding this comment.
I think a bugfix for #11 would make this a major incompatible release, since users might rely on that now. I think if we do fix it, we should put it behind a flag.
There was a problem hiding this comment.
If we know this isn't going to change behavior around #11, I am happy to merge this now
There was a problem hiding this comment.
I dont get the outcome of passing user to get_next_route fully, other than it allows for a fallback for the returned value in the end of that function.
I think it may make sense but i lack knowledge about this.
i can revert this change of passing user to the get_next_route and open a discussion about it or another pr for that specifically.
i dont think i can get a proper understanding about this myself, i lack an experimental setup
There was a problem hiding this comment.
I've added a commit where I stop passing user to get_next_url again which should make this PR safe to merge again.
There was a problem hiding this comment.
I just looked at https://github.com/jupyterhub/jupyterhub/blob/master/jupyterhub/handlers/base.py#L587, and in our case passing user= is a no-op. I'll merge this then :)
|
<3 thanks for working on this!!! Generally looks good to me, although I am wary of fixing #11 without it being an opt-in |
Passing the user returned from self.login_user() to self.get_next_url() can perhaps impact the value returned by self.get_next_url in a way that fixes an issue. This is not obvious to me if it will or not, it just seems likely that it makes sense to do. Without further information and experimentation, I'll revert doing that as it could lead to a breaking change for users.
|
Thanks @consideRatio |
I updated .travis.yml to include static code inspection. That made me raise a lot of questions regarding a code segment that may relate to #11.
Todo
PYPI_PASSWORDenv var as a project scoped PyPI deployment token in TravisCI.PyPi: https://pypi.org/project/jupyterhub-ltiauthenticator/
TravisCI: https://travis-ci.org/jupyterhub/ltiauthenticator/settings