Skip to content
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

[WIP] - Feature absences spec #82

Conversation

Laszlo-JFLMTCO
Copy link

Overview

Based on discussion with @ericfreese Schedule an Absence feature test is currently missing from application test suite.

Details

Key change points:

  • Changing test database from sqlite3 to psql
  • add launchy gem to support troubleshooting effort of Capybara tests through save_and_open_page feature
  • create absences feature spec file
  • write first test case covering use case when user without any assignments is trying to schedule an absence

Notes

This PR with adding a single test case serves the following purpose:

  • review and confirm coding style
  • agree on necessary refactors
  • set direction for developing additional test case development

…bsences_controller_spec. Feature spec file is still WIP
…test issues. Add volunteer with region assigned to factory
…lper will not work. Delete integration_helpers file as it is not necessary for Capybara troubleshooting. Finalize first test case within volunteer_schedule_absence_spec file, covering use case when volunteer without assignments trying to schedule an absence.
host: localhost
pool: 5
timeout: 5000

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit message says:

Change test database from sqlite3 to psql

I would have expected this commit to change the test section of this file, moving from sqlite3 to postgresql, but leave the rest of the file alone.

I think it would be good to keep this file around for now, since it's used as kind of a template for people getting the project running locally. The readme has a step: "Next, copy /config/database.yml.dist to /config/database.yml and make any necessary changes".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ericfreese Thank you for the comment. You are right. I just realized that database.yml was listed in .gitignore. I will have this corrected.

v.assigned = true
v.save
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this added factory actually ends up doing the same thing as the :volunteer_with_assignment factory. Volunteers are assigned to regions through the Assignment model. When the assignment is created on line 21, a new region is also created by this line in the Assignments factory.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ericfreese
For this particular test case this is being used to evaluate functionaliry when a volunteer without any assignment is trying to schedule an absence. It means not having an assignment is a precondition.
Since other tests are relying on the already existing, previously created factories, I didn't want to rename any previously created volunteer factories.
First I tried to rely on volunteer factory as it does not have assignments either, but that no regions assigned and its login fails with the Your account has not been assigned to a region yet. A region administrator must do this before you can sign in. error message.
This new factory is being called under let(:volunteer_without_assignment) within volunteer_schedule_absence_spec.rb It is already being called as volunteer_without_assignment, hoping that would clear up any possible confusion around the purpose of this factory.
Is there another factory I might have missed that already has volunteer with regions assigned without assignments?

Copy link
Member

@ericfreese ericfreese Mar 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the new factory (volunteer_with_region) actually does end up producing a Volunteer with an Assignment.

If we put a binding.pry in the test and poke around a bit, we can see there is in fact an associated Assignment:

> volunteer_without_assignment.assignments.count
=> 1

The region = create(:region); v.regions << region lines in the volunteer_with_region factory must create an Assignment to link the Region to the Volunteer. Because the regions association on Volunteer is a has_many :through, there is no way to have a Volunteer associated to a Region without an Assignment to join them, so those lines effectively create both a Region and an Assignment linked to that Volunteer.

This actually is the same outcome as the volunteer_with_assignment factory. The a = create(:assignment,volunteer:v); v.assignments << a lines will create both a Region and an Assignment and associate them with the Volunteer. This is because the assignment factory is set up to build an associated Region by default.


This explains why you're not seeing the login failure error for this user. If the user truly had no assignments, they would never be able to log in.

For this particular test case this is being used to evaluate functionaliry when a volunteer without any assignment is trying to schedule an absence.

Because it is not possible for a volunteer with no assignments to log into the system to get to the "Schedule an Absence" form, I think it would be best to test other cases. I think it would be best to start with the happy path: A volunteer with a shift successfully scheduling an absence for that shift.

end
end
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation is a little funky in this file. It looks like lines 9-28 are indented by two extra spaces and lines 14-23 and line 25 are indented by an extra space.


# describe 'GET #create' do

# end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to comment out the lines of this test?

expect(current_path).to eq(absences_path)
within('.alert') do
expect(page).to have_content('No shifts of yours was found in that timeframe')
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation still looks a little off for lines 14-23 and line 25.

…ath, but setup is still incorrect. Struggling with what is actually a shift. Need to confirm with Eric.
@rylanb
Copy link
Collaborator

rylanb commented Mar 8, 2018

Closing for now. Issue here to re-open and get up-to-date. Thanks all!

#148

@rylanb rylanb closed this Mar 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants