switch from creating new organization to setting organization to tenant

[MooseCowBear]

🔗 Issue

Fix for flaky HomeControllerTest (hopefully).

✍️ Description

Creating a new organization for the test using the factory led to @organization.location occasionally coming up nil. Switching to using the tenant seems to resolve that (ran a few hundred trials without failure).

As for why...

This part is sort of baffling to me because the factory for organization called in the setup block always created a profile and the profile factory always created a location, and the location was always accessible with @organization.profile.location but not always with @organization.location.

It seems that something about setting the tenant in the test helper setup block interferes when generating the organization with a factory because an alternative way that also seems to resolve the issue (at least for a few hundred trials) is to remove the setup block in test helper.

📷 Screenshots/Demos

[mononoken]

I assume this has to do with ActsAsTenant scoping behavior. I think your solution is good, as I think creating an organization in individual tests generally should not be done.

The test_helper setup makes an organization already and sets up that organization as the current_tenant/test_tenant in the test environment. Making an organization like this test does can be confusing, because even though it does create the organization record, the tenant that is set in the environment has already been set in the test_helper and ActsAsTenant is using it to enforce scopes.

I assume that is why removing the setup fixes the error in your investigation because then I presume ActsAsTenant does not have a current_tenant set to scope with.

I am having trouble replicating this flaky test to investigate further. I do think I noticed this test fail once before, but I have a shell running all tests in a loop right now and can't seem to replicate a failure. Do you happen to know how I could reliably replicate the failure?

[mononoken]

I commented too soon! Got an error in my shell loop.

Actually, there is a chance this may not even be related to ActsAsTenant. I still think your solution is worth pushing though.

I noticed in the failure, that the @organization.profile was not valid:

(ruby@bin/rails#31838) @organization
#<Organization:0x000000010ed8e118
 id: 6878,
 name: "Marvin, Berge and Kautzer",
 created_at: Sun, 03 Mar 2024 04:13:41.115549000 UTC +00:00,
 updated_at: Sun, 03 Mar 2024 04:13:41.115549000 UTC +00:00,
 slug: "weimann2">

(ruby@bin/rails#31838) ActsAsTenant.current_tenant
#<Organization:0x000000010ea5fbb8
 id: 6877,
 name: "Carroll Group",
 created_at: Sun, 03 Mar 2024 04:13:41.103760000 UTC +00:00,
 updated_at: Sun, 03 Mar 2024 04:13:41.103760000 UTC +00:00,
 slug: "test">              

(ruby@bin/rails#31838) @organization.location
nil 

(ruby@bin/rails#31838) @organization.profile.location
#<Location:0x000000010ef0f7d0
 id: 7605,
 country: "Country27",
 city_town: "Thompsonchester",
 province_state: "Delaware",
 created_at: Sun, 03 Mar 2024 04:13:41.110607000 UTC +00:00,
 updated_at: Sun, 03 Mar 2024 04:13:41.110607000 UTC +00:00,
 zipcode: "02401-9622">

(ruby@bin/rails#31838) @organization.profile.location.valid?
true

(ruby@bin/rails#31838) @organization.profile.valid?
false       

(ruby@bin/rails#31838) @organization.profile.errors.full_messages
["Phone number Phone Number is invalid"] 

It looks like the reason nil is returned in this failure is because the OrganizationProfile of @organization is actually not valid. Seems to do with the phone number. I wonder if you can replicate this? I hope it helps with the investigation. Let me know if you'd like help looking more into it.

[MooseCowBear]

Did a bit more investigating... And I'm also seeing that the phone number is the culprit. It looks like the invalid phone numbers are failing the check for possible. What is strange is that the tenant doesn't seem to have the same rate of failure. I'd expect some failure after many repeated trials.

[kasugaijin]

Ok this has reminded be of some weird stuff happening on OrganizationProfile /organization_profile/edit on prod site.

• The country and state are both not populated, despite the attributes being set in the creation of the OrganizationProfile seed. This is the relevant PR: https://github.com/rubyforgood/pet-rescue/pull/136/files. I am not sure why the initial value is not populating, but it may be something like a mismatch between the provided attribute e.g. "USA" as provided in the seed file, versus United States as provided by the CityState gem's country list. It may be as simple as a fix as updating the seed (update, yes fixing the seed to match the gem's values works, I will make a PR). Anyway, this is a side issue that I just thought of when you brought up the above.
• More relevant to this issue, in the seed data we provide phone_number: "123 456 7891" for the OrganizationProfile. This seems to work fine with no validation errors when creating it (satisfies possible). However, if I go to the OrganizationProfile /organization_profile/edit on prod site, it shows the formatted phone number as +1234567891 and if I try to save the form (after adding the missing country/state per above) it will throw a validation error on the phone number. If I remove the + from the phone number it will save without error. So, I am not sure why phonelib gem is accepting the number as valid first time round as "123 456 7891", but then refusing it once it has been formatted. One possible approach here is that we remove phone number altogether from the tests...which is not ideal as it would be useful to test. Another approach, we could try using a real phone number in the seed, and the test file, and see if that impacts this issue at all. To test the second option in practice, in prod, I entered my own phone number and it save first time, and second time (after formatting). I re-tried "123 456 7891" and it saved first time, then failed the second time after formatting.

[MooseCowBear]

@kasugaijin This is both fascinating and bizarre. In the Rails console, I get the same things Phonelib.parse("123 456 7891").possible? and Phonelib.parse("1234567891").possible? come up true, Phonelib.parse("+1234567891").possible? comes up false. But if I explicitly add an additional 1 for a country code, like Phonelib.parse("+11234567891").possible? it comes back true. I also tried a real NYC number and it came back false with the + and no country code, but true with the country code added. But for other numbers (both real and fake) it didn't matter if the 1 was added or not.

[kasugaijin]

@kasugaijin This is both fascinating and bizarre. In the Rails console, I get the same things Phonelib.parse("123 456 7891").possible? and Phonelib.parse("1234567891").possible? come up true, Phonelib.parse("+1234567891").possible? comes up false. But if I explicitly add an additional 1 for a country code, like Phonelib.parse("+11234567891").possible? it comes back true. I also tried a real NYC number and it came back false with the + and no country code, but true with the country code added. But for other numbers (both real and fake) it didn't matter if the 1 was added or not.

hmmm I bet Phonelib documentation and/or issues will have an explanation for this. It must be something that has come up before :)

[MooseCowBear]

I think I know what's happening. If there's no + then phone lib is treating the country as the default country, but if it is there it assumes the number starts with the country code.

[kasugaijin]

I think I know what's happening. If there's no + then phone lib is treating the country as the default country, but if it is there it assumes the number starts with the country code.

Interesting...so I wonder why it will format '1234567891' with a '+' which will subsequently fail? Maybe because it is not a real number (area code) and cannot find the country? I think that's what you're saying? If I enter my 10 digit number without country, it saves and then formats it with '+1' which saves the second time, probably because it finds a country based on area code. So, I think we can use a real number in the seeds...or at least one with a real area code and prefix https://linkedphone.com/blog/what-are-the-different-parts-of-a-phone-number-called/

[MooseCowBear]

@kasugaijin It might not be that sophisticated. It might just be a problem that the number started with a 1.

It could be that the methods used to normalize the phone number won't add the additional 1 if the number starts with a 1 already.

[kasugaijin]

@kasugaijin It might not be that sophisticated. It might just be a problem that the number started with a 1.

It could be that the methods used to normalize the phone number won't add the additional 1 if the number starts with a 1 already.

Ah yes that makes sense thank you @MooseCowBear . I think we have learned something about making valid seed data here! So is this PR still needed given #492

[MooseCowBear]

@kasugaijin There's still the mystery of why creating a new organization in the test setup would sometimes create phone numbers that fail but the tenant somehow doesn't.

We could just change the organization profile factory to set the phone number to a string not starting with a 1 instead of using Faker::PhoneNumber which we can't control instead of the change made in this PR.

[kasugaijin]

@kasugaijin There's still the mystery of why creating a new organization in the test setup would sometimes create phone numbers that fail but the tenant somehow doesn't.

We could just change the organization profile factory to set the phone number to a string not starting with a 1 instead of using Faker::PhoneNumber which we can't control instead of the change made in this PR.

Yes let's do that. I cannot be sure the Faker numbers never start with a 1 (have not looked into it), but hard coding a string that we know will satisfy seems like an easy, reliable approach. @MooseCowBear