-
Couldn't load subscription status.
- Fork 21
Add generate_link function to Portal module #39
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
Add generate_link function to Portal module #39
Conversation
willmanduffy
commented
Sep 14, 2020
- Adds an SDK method to generate an Admin Portal link.
Codecov Report
@@ Coverage Diff @@
## master #39 +/- ##
==========================================
+ Coverage 94.07% 94.16% +0.09%
==========================================
Files 13 13
Lines 253 257 +4
==========================================
+ Hits 238 242 +4
Misses 15 15
Continue to review full report at Codecov.
|
b16ddef to
0f006b4
Compare
| Args: | ||
| intent (str): The access scope for the generated Admin Portal link. Valid values are: ["sso"] | ||
| organization (string): The ID of the organization the Admin Portal link will be generated for | ||
| return_url (str): The URL that the end user will be redirected to upon exiting the generated Admin Portal. If none is provided, the default redirect link set in your WorkOS Dashboard will be used. (Optional) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move return_url into a Kwargs section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a discussion for some other time but... it's called return_url but we're using the default reDirect_uri if one is not supplied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had the same question as I was writing this. @maxchehab?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are deciding to keep this as is. See slack convo.
workos/portal.py
Outdated
| "return_url": return_url, | ||
| } | ||
| return self.request_helper.request( | ||
| "portal/generate_link", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you create a constant for this for consistency?
| self._request_helper = RequestHelper() | ||
| return self._request_helper | ||
|
|
||
| def create_organization(self, organization): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious if Organizations are going to be used for anything else in the future. If so, would it make sense to put it into a Portal module or should it go somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had this discussion as we kicked things off and it was decided that while it's not the best future home it is the one most consistent with the approach we've taken so far. In the future we might re-organize things to a more resource based approach, but that would have enough breaking changes we'd want it to be a major version.
0f006b4 to
d033e28
Compare
* Adds an SDK method to generate an Admin Portal link.
a046a92 to
8d52d0d
Compare
|
@henrylamchan I've updated with all the outstanding changes. Ready for a another review! |