[TC-184] Tenancy table creation #371
Conversation
traffic_ops/app/lib/API/Tenant.pm
Outdated
| return $self->alert("Tenant '$name' has children tenant(s): e.g '$existing_child'. Please update these tenants and retry."); | ||
| } | ||
|
|
||
| #The order of the below tests is intentional - allowing UT to cover all cases - TODO(nirs) remove this comment when a full "tenancy" UT is added, including permissions and such (no use in putting effort into it yet) |
There was a problem hiding this comment.
Can you just remove this part and then it will come in with your PR that adds tenant to ds?
There was a problem hiding this comment.
No Problem.
Note that you'll need to be careful on the merge when the next 3 PRs are added - as the order of the tests is important for the UT, I hope to fix it later on.
traffic_ops/app/lib/API/Tenant.pm
Outdated
| #TODO(nirs) - add back when making available: return $self->alert("Tenant '$name' is assign with delivery-services(s): e.g. '$existing_ds'. Please update/delete these delivery-services and retry."); | ||
| #TODO(nirs) - add back when making available: } | ||
|
|
||
| #TODO(nirs) - add back when making available: my $existing_cdn = $self->db->resultset('Cdn')->search( { tenant_id => $id })->get_column('name')->first(); |
There was a problem hiding this comment.
Can you just remove this part and then it will come in with your PR that adds tenant to cdn?
There was a problem hiding this comment.
No Problem.
Note that you'll need to be careful on the merge when the next 3 PRs are added - as the order of the tests is important for the UT, I hope to fix it later on.
traffic_ops/app/lib/API/Tenant.pm
Outdated
| #TODO(nirs) - add back when making available: return $self->alert("Tenant '$name' is assign with CDNs(s): e.g. '$existing_cdn'. Please update/delete these CDNs and retry."); | ||
| #TODO(nirs) - add back when making available: } | ||
|
|
||
| #TODO(nirs) - add back when making available: my $existing_user = $self->db->resultset('TmUser')->search( { tenant_id => $id })->get_column('username')->first(); |
There was a problem hiding this comment.
Can you just remove this part and then it will come in with your PR that adds tenant to user?
There was a problem hiding this comment.
No Problem.
Note that you'll need to be careful on the merge when the next 3 PRs are added - as the order of the tests is important for the UT, I hope to fix it later on.
| use Digest::SHA1 qw(sha1_hex); | ||
|
|
||
| my %definition_for = ( | ||
| ## id => 1 |
There was a problem hiding this comment.
can you get rid of this comment? it's misleading.
|
Also, you forgot one thing....documentation. I'll help you get started on that...I'll send a PR your way. |
traffic_ops/app/lib/API/Tenant.pm
Outdated
|
|
||
| my $is_active = $params->{active}; | ||
|
|
||
| if ( !$params->{active} && $self->isRootTenant($id)) { |
There was a problem hiding this comment.
maybe you should just make "active" required on update. otherwise, if tenant.active=true (and not root) and I send thru no active parameter, I end up with tenant.active=false. it doesn't seem like a lot to ask of the api consumer to pass thru active=true|false...
There was a problem hiding this comment.
I do not fully understand your point here.
The highlighted block does not come to verify that the parameter is set. It comes to block the ability to disable a "root" tenant - a change that cannot be fixed back without working directly with the DB.
I also believe "active" should be mandatory. Should I test the the params->{active} "exists"?
Nir
|
I created a PR into your branch for the documentation - https://github.com/nir-sopher/incubator-trafficcontrol/pull/4 If it looks ok to you, just hit the merge button and my commit will be added to this PR. |
Thank you Jeremy!
…r/incubator-trafficcontrol into tenancy-table-creation
|
Thanks Jeremy for your comments and help with documentation. |
|
As far as the "active" flag being required on update, it's no big deal. either was is fine with me. i guess i was just thinking it might be confusing to a user that if I omit the active flag on an update, it would flip tenant.active back to false. It makes sense on create - if you don't supply it, it gets its default value of false. but i'm not sure about update... up to you. |
|
I'll add the test, also as all other fields in the tenant module are tested upon update, so it is more consistent. |
This PR is the first step in adding "org tenancy" to TC. It comes to start and replace PR-322
It define the tenants table, including an "active" flag as well as "parent-tenant".
This PR is pre-requisite for adding the tenancy to the Users/CDN/DS tables