Initial WorkOS package with SSO by henrylamchan · Pull Request #4 · workos/workos-python

henrylamchan · 2020-01-02T16:06:37Z

Overview

Initial package with SSO functionality

workos/utils/requests.py

rohanjadvani · 2020-01-03T16:22:55Z

workos/sso.py

+
+class SSO(object):
+    def __init__(self):
+        required_settings = ['api_key', 'project_id', ]


Nit: don't think you need the trailing comma.

I typically would prefer it for lists and dicts, habit. For multiline dicts and lists, diffs are cleaner. For lists, this also protects us from accidental implicit string concatenation as well.

Cool sounds good 👍

Is there a linter/formatter we can use for our Python stuff so style comments don't need to come up in code reviews?

We use Rubocop for Ruby, which enforces and autofixes coding style. And on the Typescript side, we're using Prettier.

Could be interesting to use black, I've heard the most yelling about that at klaviyo. Looks like it's modeled off of prettier. I think the draw to using black is the lack of configuration for the most part and how hard lined it is.

codecov · 2020-01-07T00:48:56Z

Codecov Report

Merging #4 into master will decrease coverage by 6.49%.
The diff coverage is 93.5%.

@@           Coverage Diff            @@
##           master      #4     +/-   ##
========================================
- Coverage     100%   93.5%   -6.5%     
========================================
  Files           1       5      +4     
  Lines           2      77     +75     
========================================
+ Hits            2      72     +70     
- Misses          0       5      +5

Impacted Files	Coverage Δ
workos/__about__.py	`100% <100%> (ø)`
workos/resources/sso.py	`100% <100%> (ø)`
workos/client.py	`100% <100%> (ø)`
workos/exceptions.py	`84.21% <84.21%> (ø)`
workos/utils/request.py	`93.33% <93.33%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0555a67...a00f5ec. Read the comment docs.

marktran

👍 not a Pythonista but this seems to cover SSO as far as I can tell

I'll leave it to @rjvani to approve

rohanjadvani

Few minor suggestions, but LGTM otherwise 🎉

rohanjadvani · 2020-01-15T15:46:52Z

README.md

 # workos-python
-API Client for WorkOS
+
+Pyhon SDK to conveniently access the [WorkOS] API(https://workos.com).


Super nit, but I think for markdown you might need to do WorkOS API.

rohanjadvani · 2020-01-15T15:55:18Z

tests/test_client.py

+
+    def test_initialize_sso_missing_api_key(self, set_project_id):
+        with pytest.raises(ConfigurationException):
+            client.sso


Nit: don't have to, but can be more explicit if you want to case on the setting that's missing

rohanjadvani · 2020-01-15T15:55:49Z

tests/test_client.py

+        with pytest.raises(ConfigurationException):
+            client.sso
+
+


Can maybe also add a test if both are missing

rohanjadvani · 2020-01-15T16:05:49Z

workos/__init__.py

+
+api_key = None
+project_id = None
+base_api_url = BASE_API_URL


I don't really have a strong opinion here, but since the variable is used only once, can directly set base_api_url to the string.

I'd be down for that

rohanjadvani · 2020-01-15T16:07:54Z

workos/resources/sso.py

+        Returns:
+            WorkOSProfile: The WorkOS profile of a User
+        '''
+        profile_data = response['profile']


Maybe use get in case of potential KeyError?

Not sure how I'd want the user to encounter this just yet but I at the very least would want an exception because something I'm expecting isn't there

rohanjadvani · 2020-01-15T16:08:33Z

workos/resources/sso.py

+
+        profile = cls()
+        for field in WorkOSProfile.OBJECT_FIELDS:
+            setattr(profile, field, profile_data[field])


Same here, would consider using get

henrylamchan added the in progress label Jan 2, 2020

henrylamchan force-pushed the 201912_sso branch 2 times, most recently from 2245c60 to 61c3bc4 Compare January 2, 2020 21:47

rohanjadvani reviewed Jan 3, 2020

View reviewed changes

Henry Chan added 23 commits January 14, 2020 00:32

Initial SSO code

0c55ced

Cleanup

89b2a3c

SSOProfile class

fba1a1f

Cleanup

612a268

Raise exception on missing settings

10a6000

Update .gitignore for vscode

9363720

Basic exception handling for requests

cb0f4df

Remove use of redirect uri for getting profile

1f2994c

Extract request data for exceptions

7d2724d

Different way to do version

b860ede

Base headers for api requests

da046fd

Update requirements for setup.py

5f5a0b8

Raise the correct Auth exceptions for requests

c401626

Docstrings for the code and also rename SSOProfile -> WorkOSProfile

86b3f26

Update user agent with language

129295c

README

f49145e

Fill in some missing SSO comments

c4aa2cf

Test requirements

1ed57b3

Fix passing in SSO and update semaphore settings

01da060

Change field ordering in WorkOSProfile resource

7a85936

Exclude tests in setup.py

030ebfb

Client and SSO tests

547763d

Refactor mocking response

d21b01a

henrylamchan force-pushed the 201912_sso branch from 89d2f1b to 2ff9b49 Compare January 14, 2020 05:33

Tests for request helper

0799ebe

henrylamchan force-pushed the 201912_sso branch from 2ff9b49 to 0799ebe Compare January 14, 2020 05:35

henrylamchan added ready for review and removed in progress labels Jan 14, 2020

marktran reviewed Jan 15, 2020

View reviewed changes

rohanjadvani approved these changes Jan 15, 2020

View reviewed changes

Henry Chan added 2 commits January 15, 2020 13:33

PR Feedback

faa305c

Update version to 0.0.1

a00f5ec

henrylamchan merged commit 4a0c888 into master Jan 15, 2020

henrylamchan deleted the 201912_sso branch January 15, 2020 18:39

Conversation

henrylamchan commented Jan 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jan 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

marktran left a comment

Choose a reason for hiding this comment

Uh oh!

rohanjadvani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

henrylamchan commented Jan 2, 2020 •

edited

Loading

codecov bot commented Jan 7, 2020 •

edited

Loading