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

Add new census map UI test #20995

Merged
merged 10 commits into from
Mar 29, 2018
Merged

Add new census map UI test #20995

merged 10 commits into from
Mar 29, 2018

Conversation

drewsamnick
Copy link
Contributor

Addressing PR feedback from #20978. No rush on reviewing this.

A bunch of small changes plus I added a test of the new functionality to confirm that after selecting a school from the map dropdown and clicking the info window link that the school id in the form is set correctly.

@drewsamnick
Copy link
Contributor Author

Trying to figure out why the new test works fine for me but then is failing in circle.

@drewsamnick drewsamnick changed the title Census new map cleanup Add new census map UI test Mar 24, 2018
@drewsamnick
Copy link
Contributor Author

I was finally able to get this new test to pass and it is ready to be reviewed.

In addition to the test there are two other changes that were needed:

  1. Add an X-Requested-With header to the school autocomplete fetch call. This was needed to prevent getting a 403 response in the circle environment. The 403 was coming from Rack::Protection::JsonCsrf. That runs after the Rails controller and so the logs showed the controller had successfully handled the request with a 200 status code. Setting the X-Requested-With header causes request.xhr? to return true and thus avoids the 403. Without the header the code falls through to checking referrer(request.env) != request.host. On my dev environment those are both localhost-studio.code.org so the inequality test fails and there is no 403. On circle the referrer is localhost.code.org and the host is localhost-studio.code.org so we get the 403. My best guess for that difference is that the referrer(request.env) call will return the host if there is no HTTP_REFERER in the request. We remove that header and I suspect that it doesn't get removed in circle. If I remove Referer from the list of REMOVED_HEADERS then I can repro the 403 locally.
  2. Make the census fusion table id available in non-production environments. We can't test the map functionality without that. Originally I set it only for production to avoid any accidental updates with non-production data. Since it is very unlikely that anyone will run the update script on those environments I relaxed the constraint to set it everywhere other then development. We may eventually want to create separate fusion tables for the different environments.

Then /^I scroll the "([^"]*)" element into view$/ do |selector|
@browser.execute_script("$('#{selector}')[0].scrollIntoView(true)")
end

Then /^I scroll the save button into view$/ do
@browser.execute_script('$(".uitest-saveButton")[0].scrollIntoView(true)')
Copy link
Member

Choose a reason for hiding this comment

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

Should we refactor so that this function calls the new one above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the more specific step isn't actually in use anywhere so I'm just going to remove it.

Then I click selector "button.close" if it exists

# Chose school from the map school dropdown
Copy link
Member

Choose a reason for hiding this comment

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

nit: Choose :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose to fix this typo.

@drewsamnick drewsamnick merged commit 998ea6c into staging Mar 29, 2018
@drewsamnick drewsamnick deleted the census-new-map-cleanup branch March 29, 2018 20:11
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.

None yet

3 participants