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

Patch Ajax request to include headers per heroku api mandate #14

Merged
merged 6 commits into from
Aug 29, 2017

Conversation

quadrophobiac
Copy link
Collaborator

@quadrophobiac quadrophobiac commented Aug 21, 2017

per @pezholio’s link https://api.heroku.com/apps/hello
this is intended to fix issue #10

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.454% when pulling 6371516 on patch-for-heroku-api-changes into 76b2b26 on master.

@quadrophobiac
Copy link
Collaborator Author

So I figure a breaking issue like this warrants testing. However the existing test suite does not fail (see any one of the other PRs currently open) in light of new API changes.
Testing this locally is also fraught. Any GET to '/' results in a redirect to the live site (which is the expected behaviour, as exercised in bothan_deploy_spec and spec_helper.
The way around that is to disable some heroku .env variables, and then there is CORS to be worked around.
I tried to use Heroku review apps but then a whole can of worms opened there.
So in short - if this looks like a resolution to issue #10 can it be merged in?

@quadrophobiac quadrophobiac temporarily deployed to bothan-deploy-pr-14 August 21, 2017 15:53 Inactive
@quadrophobiac
Copy link
Collaborator Author

quadrophobiac commented Aug 21, 2017

got review apps working - this is not good to merge

@quadrophobiac quadrophobiac changed the title Amend Ajax request to include headers per heroku api mandate [WiP] Amend Ajax request to include headers per heroku api mandate Aug 21, 2017
@quadrophobiac
Copy link
Collaborator Author

EDIT EDIT what I said RE review apps - I'm not sure that heroku review apps can be used to test this either because they too redirect to https://deploy.bothan.io/ rather than their actual URL.
@pezholio, @Floppy I'm out of knowledge on the best practice here - what's the best way to test that this is operational in the same way it will be live.
I get the sense that the redirect behaviour is a function of heroku-bouncer, but I'm not certain. Looking for an expedient resolution to this if it exists but I don't know where to begin in terms of amending the redirect behaviour that's causing the testing issue

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.454% when pulling d4dd150 on patch-for-heroku-api-changes into 76b2b26 on master.

@quadrophobiac
Copy link
Collaborator Author

Have tested this manually like so

  • remove HEROKU_OAUTH_ID and HEROKU_OAUTH_SECRET to disable heroku bouncer middleware
  • run app via RACK_ENV=test bundle exec rackup config.ru
  • navigate to localhost and enter text into the Title text field. Existing apps like demo should return a 200 code, a string of characters that has no matching heroku app should return a 400

@quadrophobiac quadrophobiac changed the title [WiP] Amend Ajax request to include headers per heroku api mandate Patch Ajax request to include headers per heroku api mandate Aug 23, 2017
@Floppy
Copy link
Contributor

Floppy commented Aug 29, 2017

Live's broken anyway so let's give it its final test there :)

@Floppy Floppy merged commit 590352c into master Aug 29, 2017
@Floppy Floppy deleted the patch-for-heroku-api-changes branch August 29, 2017 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants