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

Add Logout Callback Functionality #39

Merged
merged 5 commits into from
Oct 30, 2024

Conversation

Zingzy
Copy link
Contributor

@Zingzy Zingzy commented Oct 5, 2024

Overview

This PR introduces the ability to register and execute callback functions upon user logout in the SimpleLogin class. This enhancement allows users to perform custom actions, such as logging events or clearing additional session data, immediately after a user logs out.

Changes

  • Modified logout Method:

    • The logout method now iterates over a list of registered callbacks and executes each one after clearing the session and flashing the logout message.
  • New Method register_on_logout_callback:

    • Added a new method register_on_logout_callback to allow users to register their custom callback functions.
  • Updated Tests:

    • Enhanced the existing logout test to verify that the registered callbacks are executed upon logout.

Code Example

Here's how users can register their custom callbacks:

def my_post_logout_callback():
    print("User has logged out")

def another_post_logout_callback():
    print("Another action after logout")

app = Flask(__name__)
simple_login = SimpleLogin(app)
simple_login.register_on_logout_callback(my_post_logout_callback)
simple_login.register_on_logout_callback(another_post_logout_callback)

Testing

  • Manual Testing:

    • Verified the functionality by running a local Flask application and observing the console output upon logout.
  • Automated Testing:

    • Updated the test suite to include checks for the new callback functionality.
    • Ran the tests using pytest to ensure all tests pass successfully.

Additional Notes

  • Users can register multiple callbacks, and they will be executed in the order they were registered.

This PR addresses issue #36

@Zingzy
Copy link
Contributor Author

Zingzy commented Oct 8, 2024

Hello @cuducos please review this pr 😄

@cuducos
Copy link
Member

cuducos commented Oct 8, 2024

Thank you for the great PR description! I appreciate the effort you put into it.

I kindly ask that you refrain from commenting for reviews, as it adds unnecessary noise to my inbox. I have a full-time job and a personal life, along with my involvement in this and other open-source projects. Please rest assured that I will review your code as soon as possible, but I do have other priorities now. Thank you for your understanding 💜

@Zingzy
Copy link
Contributor Author

Zingzy commented Oct 8, 2024

Thank you for the great PR description! I appreciate the effort you put into it.

I kindly ask that you refrain from commenting for reviews, as it adds unnecessary noise to my inbox. I have a full-time job and a personal life, along with my involvement in this and other open-source projects. Please rest assured that I will review your code as soon as possible, but I do have other priorities now. Thank you for your understanding 💜

Hey, I am really sorry, I do understand... Please take your time to review this PR 😀

Copy link
Member

@cuducos cuducos left a comment

Choose a reason for hiding this comment

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

This PR is great, thank you so much.

Before moving forward, I would like to ask you two things:

  • Can you add something about the new feature to the docs? You have great content as the PR description you could just move there
  • Are you familiar with unittest.mock? I suggest we refactor the test you added

None of my comments are blocking, I just wanna you to let me know if you are ki to address them, so I can keep this PR open. Otherwise, I'll merge and fix them in follow up work.

flask_simplelogin/__init__.py Outdated Show resolved Hide resolved
tests/test_app.py Outdated Show resolved Hide resolved
@Zingzy
Copy link
Contributor Author

Zingzy commented Oct 10, 2024

This PR is great, thank you so much.

Before moving forward, I would like to ask you two things:

  • Can you add something about the new feature to the docs? You have great content as the PR description you could just move there
  • Are you familiar with unittest.mock? I suggest we refactor the test you added

None of my comments are blocking, I just wanna you to let me know if you are ki to address them, so I can keep this PR open. Otherwise, I'll merge and fix them in follow up work.

Hey @cuducos thank you for reviewing my PR, I would like to fix all of the issues 😄

@Zingzy
Copy link
Contributor Author

Zingzy commented Oct 10, 2024

@cuducos could you please specify which part of the existing documentation should I add the docs about this feature and which things do I need to mention there?

@cuducos
Copy link
Member

cuducos commented Oct 10, 2024

I was thinking about a section in Configuring. The examples there should give you an idea about what to include, right?

@Zingzy

This comment was marked as outdated.

@Zingzy

This comment was marked as outdated.

@cuducos

This comment was marked as outdated.

@cuducos cuducos merged commit 0d663bf into flask-extensions:main Oct 30, 2024
4 checks passed
@cuducos
Copy link
Member

cuducos commented Oct 30, 2024

Looks great ✨ Thank you so much, @Zingzy 💜

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.

2 participants