Skip to content

Conversation

IvanAnishchuk
Copy link
Contributor

Pass variables generated based on settings when initializing Server.

@coveralls
Copy link

coveralls commented Oct 19, 2019

Pull Request Test Coverage Report for Build 1212

  • 15 of 15 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 94.738%

Totals Coverage Status
Change from base Build 1186: 0.05%
Covered Lines: 5041
Relevant Lines: 5321

💛 - Coveralls

# expires_in is passed to Server on initialization
# custom server class can have logic to override this
expires = timezone.now() + timedelta(seconds=token.get(
'expires_in', oauth2_settings.ACCESS_TOKEN_EXPIRE_SECONDS,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks unrelated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally related, this changeset is about the way to configure Server instance and since we have the settings variable already it makes total sense to pass it there too now that a way to pass variables is added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, it's required for a token generator (that can be unaware of DOT and its settings) to have access to this value -- it's normally passed as a request attribute (or its result when it's a callable).

@IvanAnishchuk IvanAnishchuk force-pushed the configurable_token_generator branch from 98ab6cc to 9e16d82 Compare October 20, 2019 00:28
@IvanAnishchuk IvanAnishchuk force-pushed the configurable_token_generator branch 6 times, most recently from 9218d7c to 08ac02e Compare October 24, 2019 16:12
@MattBlack85
Copy link
Contributor

@IvanAnishchuk is a migration file really needed for tests only models?

@IvanAnishchuk IvanAnishchuk force-pushed the configurable_token_generator branch from 0459550 to 23e06e9 Compare October 25, 2019 10:00
Pass variables generated based on settings when initializing Server.
Add a property to settings class for convenience.
Update settings documentation to reflect additions.
Enable multi-env building.
Update environments used.
@IvanAnishchuk IvanAnishchuk force-pushed the configurable_token_generator branch from 23e06e9 to 99d12b4 Compare October 25, 2019 10:01
@IvanAnishchuk
Copy link
Contributor Author

I fixed the tests, not sure whether it should be a separate PR.

@IvanAnishchuk
Copy link
Contributor Author

@IvanAnishchuk is a migration file really needed for tests only models?

Yes, if we want tests to work on some of the versions and don't want to disable all migrations in tests. Otherwise everything just fails with setup error (caused by non-migrated models depending on migrated models as far as I could figure it out) but feel free to suggest alternative fixes if you find any, I don't like this either.

@MattBlack85
Copy link
Contributor

@IvanAnishchuk is a migration file really needed for tests only models?

Yes, if we want tests to work on some of the versions and don't want to disable all migrations in tests. Otherwise everything just fails with setup error (caused by non-migrated models depending on migrated models as far as I could figure it out) but feel free to suggest alternative fixes if you find any, I don't like this either.

which versions specifically?

@IvanAnishchuk
Copy link
Contributor Author

Django 3.0 there's also a similar error with django 2.2 on python 3.4 but that seems to be a separate problem.

tox.ini Outdated
django20: Django>=2.0,<2.1
django21: Django>=2.1,<2.2
django22: Django>=2.2,<3
django30: Django>=3.0a,<3.1
Copy link
Contributor

Choose a reason for hiding this comment

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

3.0b1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's >= so it'll be whatever the latest version is.

Copy link
Contributor

Choose a reason for hiding this comment

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

then 3.0a1 as it's on pypi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not gonna change what's installed but sure, changed.

Use fully-qualified 3.0a1 version specifier instead of 3.0a.
@IvanAnishchuk IvanAnishchuk requested a review from auvipy October 27, 2019 14:09
@aleberguer
Copy link

Hi all, I'm trying out this PR to add JWT access' token.
I'm using signed_token_generator trough EXTRA_SERVER_KWARGS settings.
I'm wondering if there is a way to enrich the generated JWT with some user data such as username and scopes.
Currently, the scopes returned in the JWT are null.
Thank you!

@MattBlack85
Copy link
Contributor

@IvanAnishchuk I will find some time this week to test properly the tox matrix, the migration still doesn't look too right to me so I'd like to investigate this a bit, other than that 👍

@IvanAnishchuk IvanAnishchuk merged commit 6c7260d into master Oct 30, 2019
@IvanAnishchuk IvanAnishchuk deleted the configurable_token_generator branch October 30, 2019 19:21
Copy link
Contributor

@n2ygk n2ygk left a comment

Choose a reason for hiding this comment

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

@IvanAnishchuk It's after-the-fact (already merged back in October) but I'm now discovering where you disabled several of the tox tests. The more appropriate solution would be to fix the code that causes the tests to fail.

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.

7 participants