-
Notifications
You must be signed in to change notification settings - Fork 373
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
Defines the TenantManager class and its underlying methods. #617
Conversation
Adds unit tests for this new class. Unit tests were copied from the exising auth.spec.ts file. All deprecated methods will be removed from the Auth class in a follow up PR.
const tenantId = 'tenant_id'; | ||
const serverResponse: TenantServerResponse = { | ||
name: 'projects/project_id/tenants/tenant_id', | ||
displayName: 'TENANT_DISPLAY_NAME', |
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.
Use only letters, digits and hyphens to to be consistent with requirements
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.
Done.
…ers, digits and hyphens.
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.
LGTM with a few nits.
src/auth/tenant-manager.ts
Outdated
return new Tenant(response); | ||
}); | ||
} | ||
} |
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.
Add newline at eof.
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.
Done
let rejectedPromiseAccessTokenTenantManager: TenantManager; | ||
|
||
|
||
beforeEach(() => { |
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.
May be before/after is enough instead of beforeEach/afterEach?
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.
Done
}); | ||
|
||
it('should be rejected given an invalid tenant ID', () => { | ||
const invalidTenantId = ''; |
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.
Should we test for other arguments? null, int, object etc?
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.
Done
passwordRequired: true, | ||
}, | ||
}; | ||
const serverResponse: TenantServerResponse = { |
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.
This seems to be duplicated in multiple describe blocks. Perhaps move it one level up and turn into a constant like GET_TENANT_RESPONSE
so all tests can use the same without duplication.
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.
done
}); | ||
|
||
it('should be rejected given an invalid max result', () => { | ||
const invalidResults = 5000; |
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.
I suppose 1000 is the allowed max. I'd mention that in the test description or change the assignment to something like:
const moreThanMax = 1000 + 1;
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.
Both suggestions accepted.
Thanks for the quick review! |
|
||
it('should return a TenantAwareAuth with read-only tenant ID', () => { | ||
expect(() => { | ||
(tenantManager.authForTenant(TENANT_ID) as any).tenantId = 'OTHER_TENANT_ID'; |
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.
Use only letters, digits and hyphens to to be consistent with real tenant id
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.
done
}); | ||
|
||
describe('updateTenant()', () => { | ||
const tenantId = 'tenant_id'; |
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.
Same here
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.
done
}, | ||
}; | ||
const serverResponse: TenantServerResponse = { | ||
name: 'projects/project_id/tenants/tenant_id', |
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.
Same here for both project_id and tenant_id
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.
done
}); | ||
|
||
describe('createTenant()', () => { | ||
const tenantId = 'tenant_id'; |
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.
Same here
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.
done
describe('createTenant()', () => { | ||
const tenantId = 'tenant_id'; | ||
const tenantOptions: TenantOptions = { | ||
displayName: 'TENANT_DISPLAY_NAME', |
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.
Same here
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.
done
}; | ||
const serverResponse: TenantServerResponse = { | ||
name: 'projects/project_id/tenants/tenant_id', | ||
displayName: 'TENANT_DISPLAY_NAME', |
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.
Same here
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.
done
Adds unit tests for this new class. Unit tests were copied from the
exising auth.spec.ts file.
All deprecated methods will be removed from the Auth class in a follow
up PR.