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

super rough around the edges api for better spree paypal express #168

Open
wants to merge 6 commits into
base: 2-4-stable
Choose a base branch
from

Conversation

williscool
Copy link

basically just takes what the regular spree version does and lets you do
it passing json

also made simple poro model for returning the url to redirect to on
paypal

needs lots of code review, feedback, testing, and probably should be done with websockets or pusher for mobile clients but is fine for people just wanting interop with a frontend javascript framework which is my use case

basically just takes what the regular spree version does and lets you do
it passing json

also made simple poro model for returning the url to redirect to on
paypal
@williscool
Copy link
Author

refs #13

@MisinformedDNA
Copy link

Nice work!

Are the failing tests ones that existed beforehand?

@MisinformedDNA
Copy link

@alepore What's the plan going forward for merging PRs? I'm ending up with a franken-repo of different patches I need. We can't let broken UI tests stop progress on everything else. Especially when the UI is out of our control.

@alepore
Copy link
Member

alepore commented Jun 19, 2015

@MisinformedDNA right 👍
i'l try to do some updates next week, starting from disabling all failing tests


response = Spree::Paypal.new
response.redirect_url = url
respond_with response

Choose a reason for hiding this comment

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

I'm getting an error here on line 45:

Via a console app

        response = client.post("/api/paypal", {
          "order_id": order['number'],
          "payment_method_id": payment_method["id"],
          "confirm_url": "http://localhost:3001",
          "cancel_url": "http://localhost:3001/checkout/payment"
        })

Error log:

ActionView::MissingTemplate: Missing template spree/api/paypal/express, spree/api/base/express with {:locale=>[:en], :formats=>[:json], :variants=>[], :handlers=>[:erb, :builder, :raw, :ruby, :jbuilder, :coffee, :rabl], :versions=>[:v10, :v9, :v8, :v7, :v6, :v5, :v4, :v3, :v2, :v1]}. Searched in:
  * "/vagrant/dragondoor/app/views"
  * "/vagrant/dragondoor/app/views"
  * "/vagrant/Misinformed/better_spree_paypal_express/app/views"
  * "/home/vagrant/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/spree_gateway-003416462d5a/lib/views/backend"
  * "/home/vagrant/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/spree_gateway-003416462d5a/lib/views/frontend"
  * "/home/vagrant/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/spree_promo_variant_rule-1ebdeb453880/app/views"

Error log from my custom web app (called similarly):

  CACHE (0.1ms)  SELECT  "spree_payment_methods".* FROM "spree_payment_methods"  WHERE "spree_payment_methods"."deleted_at" IS NULL AND "spree_payment_methods"."id" = $1 LIMIT 1  [["id", 3]]
I, [2015-06-19T22:08:39.767190 #30749]  INFO -- : Action: SetExpressCheckout
I, [2015-06-19T22:08:39.767480 #30749]  INFO -- : Request[post]: https://api-3t.sandbox.paypal.com/2.0/
I, [2015-06-19T22:08:45.550440 #30749]  INFO -- : Response[200]: OK, Duration: 5.783s
  CACHE (0.2ms)  SELECT  "spree_payment_methods".* FROM "spree_payment_methods"  WHERE "spree_payment_methods"."deleted_at" IS NULL AND "spree_payment_methods"."id" = $1 LIMIT 1  [["id", 3]]
ActionController::UnknownFormat
/home/vagrant/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/actionpack-4.1.9/lib/action_controller/metal/mime_responds.rb:440:in `retrieve_collector_from_mimes'
/home/vagrant/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/bundler/gems/spree-c0089b1b5e65/core/lib/spree/core/controller_helpers/respond_with.rb:9:in `respond_with'
/vagrant/Misinformed/better_spree_paypal_express/app/controllers/spree/api/paypal_controller.rb:45:in `express'
/home/vagrant/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/actionpack-4.1.9/lib/action_controller/metal/implicit_render.rb:4:in `send_action'
/home/vagrant/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/actionpack-4.1.9/lib/abstract_controller/base.rb:189:in `process_action'
/home/vagrant/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/actionpack-4.1.9/lib/action_controller/metal/rendering.rb:10:in `process_action'

Am I missing something?

Copy link
Author

Choose a reason for hiding this comment

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

Very strange. In my app it just picks a layout for the json. Doesn't have anything about the view it uses in the log.

It looks like your app is not understanding to render the json even though spree's api at least purports to do that here

https://github.com/spree/spree/blob/2-4-stable/api/lib/spree/api/controller_setup.rb

so I guess I need to wrap those render :json calls in respond_to blocks ala

http://stackoverflow.com/questions/22943892/actioncontrollerunknownformat/22944769#22944769

and/or

add :defaults => { :format => 'json' }

http://stackoverflow.com/questions/22943892/actioncontrollerunknownformat/28750863#28750863

to those api route definitions

Edit:
on second thought yeah thats almost certainly whats going on @MisinformedDNA .

I bet your client.post call is not properly setting a json mime type so the rails server can't figure out how to respond to it

Choose a reason for hiding this comment

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

My test is to simply alter the spree_api_examples to include the PayPal API. I've never had a problem with this setup in accessing any other API.

Can you run walkthrough-paypal.rb against your instance and see how it goes?

Copy link
Author

Choose a reason for hiding this comment

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

I can't even get the regular walkthrough running my friend

https://github.com/spree-contrib/spree_api_examples/blob/master/examples/checkout/walkthrough.rb

opened and issue with one of the many issues I'm running into

spree-contrib/spree_api_examples#5

the routes never seem to exist no matter how I try to change them to

which branch of that repo are your running @MisinformedDNA ?

Choose a reason for hiding this comment

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

I'm running 2-4-stable, 2.4.8.beta.

To run the api examples, just set up a clean sandbox. Alternately, you'd have to alter the baseUrl and all the calls since its not mounted at root.

Choose a reason for hiding this comment

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

Co-worker figured out the problem. We need a RABL file. See Infigic@60ab41b.

Copy link
Author

Choose a reason for hiding this comment

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

Ah yeah the app im working with sets up json responses differently.

@alepore
Copy link
Member

alepore commented Jun 20, 2015

p.s: i removed red tests on master 🙈

@MisinformedDNA
Copy link

Thanks! Could we port that to 2-4 as well?

@alepore
Copy link
Member

alepore commented Jun 22, 2015

👍 done

@alepore
Copy link
Member

alepore commented Jun 22, 2015

didn't looked at the code yet but i think this PR should target master and not old stable branches

@MisinformedDNA
Copy link

spree_wombat uses gem.add_dependency 'active_model_serializers', '0.9.0.alpha1'. This PR should specify 0.9.0.alpha1 so the plugins remain compatible.

See https://github.com/spree/spree_wombat/blob/2-4-stable/spree_wombat.gemspec#L17

@MisinformedDNA
Copy link

@williscool specifically targeted 2-4-stable in this PR, so it should definitely be merged into 2-4-stable.

@MisinformedDNA
Copy link

@alepore Is there anything preventing this from getting merged?

@alepore
Copy link
Member

alepore commented Jul 6, 2015

@MisinformedDNA i read "needs lots of code review, feedback, testing" and is targeting an old branch, where we don't add new features and breaking changes...

@MisinformedDNA
Copy link

The code is pretty straightforward. I've reviewed and done integration testing. It is working fine for me. And while it is a new feature, there are no breaking changes.

But per Spree's Release Policy, I understand that we shouldn't add new features or APIs:

Two branches “back” from master (currently 2-4-stable) receives patches for major and minor issues, and security problems. Absolutely no new features, tweaking or API changes.

I'll just use a fork until I can get upgraded.

Thanks for your help.

@williscool
Copy link
Author

No prob thanks man.

@alepore
Copy link
Member

alepore commented Jul 7, 2015

this a is s good candidate for the master branch, of course :)

@givanse
Copy link

givanse commented Jul 31, 2015

@williscool I know you built this for a spree-ember app, why is it targeting branch 2-4 instead of 3-0? Also, thanks!

@williscool
Copy link
Author

@givanse The app my team just shipped is on 2.4 would be happy to accept some commits to get it to merge with 3.0. Would love to see it in mainline

@givanse
Copy link

givanse commented Aug 3, 2015

I created a PR for the branch 3-0-stable here #177

post '/paypal/confirm', :to => "paypal#confirm", :as => :confirm_paypal_api
post '/paypal/cancel', :to => "paypal#cancel", :as => :cancel_paypal_api
get '/paypal/notify', :to => "paypal#notify", :as => :notify_paypal_api
Copy link

Choose a reason for hiding this comment

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

I don't see a method that handles this route.

Copy link
Author

Choose a reason for hiding this comment

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

there isnt one I just copied that from the the other paypal stuff I think it was some old thing that never got removed. I would search the history of the repository for it

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.

5 participants