Add initial test for org scoping

[lenikadali]

Added an initial test for organizational scoping
in the supervisor weekly email

What github issue is this PR for, if any?

Resolves #5640

What changed, and why?

This was a fix to remove unknown names from the weekly supervisor email

How is this tested? (please write tests!) 💖💪

Running bundle exec rspec spec/mailers/supervisor_mailer_spec.rb
Note: if you see a flake in your test build in github actions, please post in slack #casa "Flaky test: " :) 💪
Note: We love capybara tests! If you are writing both haml/js and ruby, please try to test your work with tests at every level including system tests like https://github.com/rubyforgood/casa/tree/main/spec/system

Screenshots please :)

N/A

Feelings gif (optional)

[elasticspoon]

Does this code differ from the code right above it on lines 121-126?

it "display no_recent_sign_in section" do
  expect(mail.body.encoded).to match("volunteers have not signed in or created case contacts in the last 30 days")
  expect(mail.body.encoded).to match(volunteer.display_name)
  expect(mail.body.encoded).not_to match(volunteer_for_other_supervisor_same_org.display_name)
  expect(mail.body.encoded).not_to match(volunteer_for_other_org.display_name)
end

It feels like maybe the issue should have actually been closed earlier and the tests already existed?

[lenikadali]

I think so. At least from my understanding, tests are meant to check behaviour not implementation.
Maybe it needs trying to reproduce the behaviour that the original PR was meant to fix and thereafter writing tests that confirm the behaviour is now what we want it to be.

Another thing we can check is if in the original bug report, these tests were not failing already.

@FireLemons's suggestion on Discord was:

The goal of the test is to check that the PR from earlier works so if your test fails without the changes from the PR and passes with the changes it should be good.
It would be nice to have proof that the test fails without the PR in your PR.

so that's the direction I'm going to explore.

[elasticspoon]

The goal of the test is to check that the PR from earlier works so if your test fails without the changes from the PR and passes with the changes it should be good.

I would agree with that assessment. So sounds like you have a direction to explore. 👍

[lenikadali]

Hey @elasticspoon

So I've tried to experiment with this and whereas going by the conversation in the issue, it looks to be fixed; on the other hand, the addition of the filter doesn't break the existing tests.

# original
 @inactive_volunteers = supervisor.inactive_volunteers

# change
 @inactive_volunteers = supervisor.inactive_volunteers.where(casa_org_id: @casa_organization.id)

which is technically covered by the following expectations:

expect(mail.body.encoded).to match("volunteers have not signed in or created case contacts in the last 30 days")
expect(mail.body.encoded).to match(volunteer.display_name)
expect(mail.body.encoded).not_to match(volunteer_for_other_supervisor_same_org.display_name)
expect(mail.body.encoded).not_to match(volunteer_for_other_org.display_name)

since the filter makes sure that only inactive volunteers from the same org as the supervisor are returned. The query as is already returns volunteers who are being supervised by the supervisor in question.

I tried doing

 let!(:volunteer_2) do
    create(:volunteer, casa_org: other_org, supervisor: supervisor, last_sign_in_at: 31.days.ago)
end

and adding the expectation expect(mail.body.encoded).not_to match(volunteer_for_other_org.display_name) but that still passed with or without the added filter.

If you can reproduce the behaviour, that would be helpful. For me, I've hit the end of what I can try in order to reproduce the issue.

cc @FireLemons