Skip to content

Set BLACKLIST_AFTER_ROTATION by default to False at docs#455

Merged
Andrew-Chen-Wang merged 3 commits intojazzband:masterfrom
mohmyo:patch-1
Sep 19, 2021
Merged

Set BLACKLIST_AFTER_ROTATION by default to False at docs#455
Andrew-Chen-Wang merged 3 commits intojazzband:masterfrom
mohmyo:patch-1

Conversation

@mohmyo
Copy link
Copy Markdown
Contributor

@mohmyo mohmyo commented Sep 17, 2021

No description provided.

Copy link
Copy Markdown
Member

@Andrew-Chen-Wang Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

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

Please also set the default value in the Python code itself in settings.py. I've done it. If there are any tests that fail, please resolve them with override_settings. Thanks.

@Andrew-Chen-Wang Andrew-Chen-Wang linked an issue Sep 17, 2021 that may be closed by this pull request
@mohmyo
Copy link
Copy Markdown
Contributor Author

mohmyo commented Sep 18, 2021

There is only one test case that makes tests fails on circleci, because of this line when we set BLACKLIST_AFTER_ROTATION to False by default BlacklistedTokenis not imported by default too.
This wasn't the case for the previous version, it was imported by default so it didn't cause any troubles.

@mohmyo
Copy link
Copy Markdown
Contributor Author

mohmyo commented Sep 18, 2021

So should we just import BlacklistedToken anyway and remove that conditional import line or there is a better approach here?

@Andrew-Chen-Wang
Copy link
Copy Markdown
Member

@mohmyo just monkeypatch it. Use pytest's init hook and monkey patch api_settings to return True for the module.

Regarding "should we just import BlacklistedToken anyway", I don't know if this is true, but if you don't have the app installed, then it will raise some form of ModuleNotFound error. But please test it! I'd actually much rather have it be imported, but I suspect the conditional import is there for a reason.

@mohmyo
Copy link
Copy Markdown
Contributor Author

mohmyo commented Sep 18, 2021

I tried to remove the condition and import it anyway, tests passed like normal as before. so let's go this way?

@Andrew-Chen-Wang
Copy link
Copy Markdown
Member

Please try creating a Django project. I suspect it works because of an INSTALLED_APP

@mohmyo
Copy link
Copy Markdown
Contributor Author

mohmyo commented Sep 18, 2021

mmmm, without trying I think you are right, I missed that part for INSTALLED_APPS. I will go with monkeypatch as you suggested.

@mohmyo
Copy link
Copy Markdown
Contributor Author

mohmyo commented Sep 18, 2021

To be more clear as I think there is a bit of confusion here, this is the error:
FAILED tests/test_token_blacklist.py::TokenVerifySerializerShouldHonourBlacklist::test_token_verify_serializer_should_honour_blacklist_if_blacklisting_enabled - NameError: name 'BlacklistedToken' is not defined

The problem is serializers.py doesn't import BlacklistedToken as it was loaded by default without it, so how should I force reloading the serializers.py when BLACKLIST_AFTER_ROTATION is set to True hence it will import our model?

I'm not familiar with pytest, I tried but I'm not going anywhere.

@Andrew-Chen-Wang
Copy link
Copy Markdown
Member

@mohmyo it's fine; I got it.

@Andrew-Chen-Wang
Copy link
Copy Markdown
Member

thanks for creating the PR!

@mohmyo
Copy link
Copy Markdown
Contributor Author

mohmyo commented Sep 19, 2021

Thank you for maintaining this great package!

@Andrew-Chen-Wang Andrew-Chen-Wang merged commit d9667dd into jazzband:master Sep 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BlackListedToken has no attribute 'objects'

2 participants