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

Simplify static files and remove login manager #370

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

sebschrader
Copy link
Member

Simplify the restricted document handling introduced for #281 in #319 and remove the SipaLoginManager.

Please make sure though you did the following (tick off everything you
already did):

  • Run the tests and see them pass
  • Rebase your branch on top of develop
  • Include tests for features you introduced / bugs you fixed

login_manager.login_view = "generic.login"
login_manager.login_message = "Bitte melde Dich an, um die Seite zu sehen."
Copy link
Contributor

Choose a reason for hiding this comment

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

I've not run the code, but self.localize_callback = gettext seems to miss compared to SipaLoginManager?

from flask_login import LoginManager


class SipaLoginManager(LoginManager):
Copy link
Collaborator

@lukasjuhrich lukasjuhrich Mar 25, 2018

Choose a reason for hiding this comment

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

If you delete things, it's good practice to run a grep through the project so one notices superfluous usages before the CI does.

tl;dr you forgot to delete the test that's associated with that module.

@lukasjuhrich
Copy link
Collaborator

I'm just noticing that your changes rendered the tests unusable, because they just tested the custom login manager, and not the blueprint themselves. Unfortunately, I don't have time to look into this, so you will need to fix this before I can merge.

@lukasjuhrich
Copy link
Collaborator

Since – judging by current observations – our user_loader exemption does not even work at the moment, we can

  • rebase this branch
  • implement a member_required decorator to account for ad779d1
  • test the functionality

The decorator could look like this (analogous to the login_required implementation):

def member_required(func):
    @wraps(func)
    def decorated_view(*args, **kwargs):
        if request.method in EXEMPT_METHODS or current_app.config.get("LOGIN_DISABLED"):
            pass
        elif not current_user.is_authenticated \
                or current_user.is_authenticated and not current_user.is_member:
            return current_app.login_manager.unauthorized()
        return func(*args, **kwargs)

    return decorated_view

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

Successfully merging this pull request may close these issues.

3 participants