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

[NV-PHP-5] Add tenants support #41

Merged
merged 2 commits into from
Oct 3, 2023
Merged

[NV-PHP-5] Add tenants support #41

merged 2 commits into from
Oct 3, 2023

Conversation

kraynel
Copy link
Contributor

@kraynel kraynel commented Oct 2, 2023

Hello !
This should add support for tenants and solve #33.

I tried composer lint and composer test, but it updates almost all files in the repo, is it really used?

@unicodeveloper
Copy link
Contributor

unicodeveloper commented Oct 3, 2023

Thanks for the contribution @kraynel. It's not used. So no need to run them. I'll do a proper clean up soon of the linting soon.

}

/**
* Fetch one subscriber.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be Fetch a tenant

*/
public function getTenant($tenantId)
{
$subscriber = $this->get("tenant/{$tenantId}")['data'];
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should be $tenant, and not $subscriber.

{
$subscriber = $this->get("tenant/{$tenantId}")['data'];

return new Tenant($subscriber, $this);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should be $tenant, and not $subscriber.

public $environmentId;

/**
* Return the array form of NotificationTemplate object.
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should be Tenant object, and not NotificationTemplate object.

*
* @param string $tenantId
*
* @return \Novu\SDK\Resources\Subscriber
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should return a Tenant Resource instead of a Subscriber resource

*/
public function updateTenant($tenantId, array $data)
{
$subscriber = $this->patch("tenants/{$tenantId}", $data)['data'];
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should be $tenant, and not $subscriber.

{
$subscriber = $this->patch("tenants/{$tenantId}", $data)['data'];

return new Tenant($subscriber, $this);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should be $tenant, and not $subscriber.

/**
* Create a new tenant.
*
* @return \Novu\SDK\Resources\Tenant
Copy link
Contributor

@unicodeveloper unicodeveloper Oct 3, 2023

Choose a reason for hiding this comment

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

Please add the docs @param for this method

@unicodeveloper
Copy link
Contributor

Please attend to the comments @kraynel

@kraynel
Copy link
Contributor Author

kraynel commented Oct 3, 2023

Thanks for the review @unicodeveloper, sorry for the poor copy/paste quality. It should be better now.

@unicodeveloper
Copy link
Contributor

Perfect. LGTM! ⭐

@unicodeveloper unicodeveloper merged commit f117237 into novuhq:master Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants