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

Add domainData to Organization create and update methods, replacing deprecated domains #223

Conversation

mthadley
Copy link
Contributor

@mthadley mthadley commented May 4, 2024

Description

Adds a new domainData option to be passed to Organization create and update calls, instead of the domains parameter which is being deprecated.

Documentation

Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.

[ ] Yes

If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.

@mthadley mthadley requested review from amadeo-workos, a team and cadef and removed request for a team May 4, 2024 01:28
Copy link

linear bot commented May 4, 2024

Copy link
Contributor

@amadeo-workos amadeo-workos left a comment

Choose a reason for hiding this comment

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

Just a few small nits from me.

@@ -14,12 +14,39 @@ import com.workos.organizations.models.OrganizationList
* The OrganizationsApi provides convenience methods for working with WorkOS Organizations.
*/
class OrganizationsApi(private val workos: WorkOS) {
/**
* Options when setting the domains for an organization.
Copy link
Contributor

@amadeo-workos amadeo-workos May 5, 2024

Choose a reason for hiding this comment

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

Nit: when I built this I tried to not have any class/enum inlined into another, and have them all separated, within the types folder. I think it makes of a cleaner, easier way to find and reuse things. But I also understand this is more compact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! I'll move it there.

*/
enum class OrganizationDomainDataState {
@JsonProperty("verified")
VERIFIED,
Copy link
Contributor

Choose a reason for hiding this comment

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

Another nit, no other enum uses all uppercase to define possible values in an enum, they all use first letter in the word as uppercase, rest as lowercase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I hadn't seen the others. Will keep it consistent.

/**
* Parameters for [createOrganization].
* Use `CreateOrganizationOptions.builder()` to create a new builder instance.
*
* @param name The name of the organization.
* @param allowProfilesOutsideOrganization Whether the Connections within this Organization should allow Profiles that do not have a domain that is present in the set of the Organization's User Email Domains.
* @param domainData A list of data for the domains of the organization.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use @deprecated here for the documentation too? https://docs.oracle.com/javase/1.5.0/docs/guide/javadoc/deprecation/deprecation.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, are these Javadoc comments or KDoc?

https://kotlinlang.org/docs/kotlin-doc.html#suppress

KDoc does not support the @deprecated tag. Instead, please use the @deprecated annotation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's right. Good to know about this.

@mthadley mthadley merged commit 20e29d9 into main May 6, 2024
2 checks passed
@mthadley mthadley deleted the feature/auth-3096-kotlin-add-domains_data-to-sdks-and-deprecate-domains branch May 6, 2024 22:08
@mthadley mthadley mentioned this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants