Update creat tenant factory #409
Conversation
|
But if everyone feels that |
There was a problem hiding this comment.
create_tenant(3) is confusing. I would assume that it would create 3 tenants
Yes, I would assume that as well too. Good catch.
Everything looks good. I made a couple comments.
There are 6 commits in this PR, can you rebase/remove the irrelevant ones and force push?
Thanks!
| tenant_1 = create_tenant() | ||
| tenant_2 = create_tenant() | ||
| keep_tenant = create_tenant() | ||
| tenant_1 = create_tenant([]) |
There was a problem hiding this comment.
I'm not fond of having to know that an empty array needs to be passed in to the fixture in order for the method call to work. Can we make the parameter optional?
|
|
||
| def test_create_tenant_with_staff(self, create_tenant, create_join_staff): | ||
| staff = [create_join_staff().id for _ in range(3)] | ||
| tenant = create_tenant(staff) |
There was a problem hiding this comment.
This is good.
Do you think it would be beneficial to create the staff inside of the create_tenant fixture instead?
Maybe do something like? create_tenant(staff=3). But then there is the downside of not being able to control how the staff is created. Perhaps the create_tenant fixture could be smart. If it's given a list of staff objects then it simply assigns those, if its provided an integer then it creates that many staff members and assigns them. If there are no arguments then it creates a tenant without any assigned staff.
What do you think?
yunnie
force-pushed
the
UpdateCrateTenantFactory
branch
from
d77f561 to
350371e
Compare
Description
What issue is this solving?
Closes codeforpdx/dwellingly-app/issues/640
I know the request was to add a
join_staff=3variable to thecreate_tenant()fixture, but I thinkcreate_tenant(3)is confusing. I would assume that it would create 3 tenants, but it creates a single tenant with 3 assigned staff.I've set it up so that you need to use the
create_join_stafffixture directly.It's an additional step, but I think the intension is clearer.