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

Deprecate LogoutButton #2899

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

leeagustin
Copy link
Contributor

@leeagustin leeagustin commented Jun 22, 2024

Fixes #2894.

PR will output a DeprecationWarning when the LogoutButton is used.

I tried making the changes from #2894 (comment), but it seems that it only validates the initial page layout and not the components used in callbacks.

As seen in the modified test in this PR, the test doesn't output a DeprecationWarning and fails when the LogoutButton is in the callback but not in the initial page layout. On the other hand, it passes when LogoutButton is in the initial page layout.

I'm assuming we would also want to show a DeprecationWarning if LogoutButton is used in a callback. What would be a good way to approach this? Or would it be simpler to remove the LogoutButton component entirely?

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • Outputs DeprecationWarning if LogoutButton is used in layout
    • Outputs DeprecationWarning if LogoutButton is used in callback
  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follows
    • this GitHub #PR number updates the dash docs
    • here is the show and tell thread in Plotly Dash community

@@ -422,6 +423,15 @@ def validate_layout(layout, layout_value):
component_ids = set()

def _validate(value):
def _validate_type(comp):
deprecated_types = ["LogoutButton"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure there is no collision, we should also check the _namespace attribute. Also could add a custom deprecation message to the component.

I would refactor the deprecated_types to a collections.defaultdict(lambda: collections.defaultdict(dict)) then set:

deprecated_components["dash_core_components"]["LogoutButton"] = "''
The Logout Button is no longer used with Dash Enterprise and can be replaced with a html.Button or html.A.
eg: html.A(href=os.getenv('DASH_LOGOUT_URL'))
"""

The for the check you could have:

_type = getattr(comp, "_type",  "")
_ns = getattr(comp, "_namespace", "")
deprecation_message = deprecated_components[_ns][_type]
if deprecation_message:
    ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 87385a9

@T4rk1n T4rk1n mentioned this pull request Jun 25, 2024
- Checks for namespace on top of the type
- Modifies warning message
- Modifies test to capture new warning message
dash_dcc.start_server(app)
time.sleep(1)
dash_dcc.percy_snapshot("Core Logout button")
with pytest.warns(
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to add a condition if add_initial_logout_button for testing the deprecation notice otherwise it fails. Maybe better to just test the deprecation notice in a new test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. My worry is that DeprecationWarning is not being output when the LogoutButton is being used inside a callback—only when the LogoutButton is defined in app.layout.

This is the reason I was checking for the warning in the two separate scenarios. However, if we don't need to output a DeprecationWarning when the LogoutButton is used inside a callback, then I can just create a new test.

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.

Remove LogoutButton
2 participants