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

feat(Notion): Added Notion provider. #3316

Merged
merged 11 commits into from
Aug 3, 2023
Merged

Conversation

synchronizing
Copy link
Contributor

Submitting Pull Requests

General

  • Make sure you use semantic commit messages.
    Examples: "fix(google): Fixed foobar bug", "feat(accounts): Added foobar feature".
  • All Python code must formatted using Black, and clean from pep8 and isort issues.
  • JavaScript code should adhere to StandardJS.
  • If your changes are significant, please update ChangeLog.rst.
  • If your change is substantial, feel free to add yourself to AUTHORS.

Provider Specifics

In case you add a new provider:

  • Make sure unit tests are available.
  • Add an entry of your provider in test_settings.py::INSTALLED_APPS and docs/installation.rst::INSTALLED_APPS.
  • Add documentation to docs/providers.rst.
  • Add an entry to the list of supported providers over at docs/overview.rst.

@synchronizing
Copy link
Contributor Author

Closes #3112.

@synchronizing
Copy link
Contributor Author

@pennersr Any interest on this addition?

q = parse_qs(p.query)

# Always false.
pkce_enabled = False
Copy link
Owner

Choose a reason for hiding this comment

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

Question: There are a few if pkce_enabled down below, but here it is hardcoded to false. Is that intentional?

Copy link
Contributor Author

@synchronizing synchronizing Jul 12, 2023

Choose a reason for hiding this comment

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

Notion doesn't support PKCE. Looked around when I first implemented this, but did not find a decent way to properly disabled its test in the old codebase. Resorted to bringing over the login test and manually setting pkce_enabled to False with minor mods to get it working.

I see that the OAuthTestsMixin.login has been modified in the last few weeks, and just tried

class NotionTests(OAuth2TestsMixin, TestCase):
    provider_id = NotionProvider.id
    pkce_enabled_default = False

with no luck either due to OAuth2TestMixin.test_provider_has_pkce_params which overwrites self.provider.pkce_enabled_default. Unsure if this is a bug or intended.

Whatever the case, just committed a new change that cleans up the function a bit more and removes pkce_enabled entirely, while also adding pkce_enabled_default = False as a class variable (just in case, and to be more verbose of the fact). If you think there are any better way to go about this do let me know!

@coveralls
Copy link

coveralls commented Jul 12, 2023

Coverage Status

coverage: 91.281% (-0.2%) from 91.489% when pulling d7010ce on synchronizing:master into bd61beb on pennersr:main.

@pennersr
Copy link
Owner

@pennersr Any interest on this addition?

Definitely!

@pennersr
Copy link
Owner

pennersr commented Aug 1, 2023

There are conflicts -- can you please rebase?

@synchronizing
Copy link
Contributor Author

There are conflicts -- can you please rebase?

Merged in main.

@pennersr
Copy link
Owner

pennersr commented Aug 1, 2023

Merged in main.

There still seem to be conflicts...

@synchronizing
Copy link
Contributor Author

synchronizing commented Aug 1, 2023

Merged in main.

There still seem to be conflicts...

Don't see it, tbh. In my branch seeing

This branch is 11 commits ahead of pennersr:main.

and in this MR

This branch has no conflicts with the base branch

Branch looks to be in sync with your main. Coveralls went down 0.2% which caused the check failure (due to sync), and tests look all good. Everything else (other than your change request) looks ok. If you don't mind directing me towards where you see conflicts, I would appreciate it. 😄

@pennersr pennersr merged commit 00c9bc9 into pennersr:main Aug 3, 2023
23 checks passed
@pennersr pennersr mentioned this pull request Aug 4, 2023
9 tasks
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.

3 participants