search

spree-contrib / better_spree_paypal_express

A better Spree PayPal Express Extension.
http://guides.spreecommerce.org
BSD 3-Clause "New" or "Revised" License
110 stars 269 forks source link

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

Open williscool opened 9 years ago

williscool commented 9 years ago

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

williscool commented 9 years ago

refs #13

MisinformedDNA commented 9 years ago

Nice work!

Are the failing tests ones that existed beforehand?

MisinformedDNA commented 9 years ago

@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 commented 9 years ago

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

alepore commented 9 years ago

p.s: i removed red tests on master :see_no_evil:

MisinformedDNA commented 9 years ago

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

alepore commented 9 years ago

:+1: done

alepore commented 9 years ago

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

MisinformedDNA commented 9 years ago

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 commented 9 years ago

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

MisinformedDNA commented 9 years ago

@alepore Is there anything preventing this from getting merged?

alepore commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

No prob thanks man.

alepore commented 9 years ago

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

givanse commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

I created a PR for the branch 3-0-stable here https://github.com/spree-contrib/better_spree_paypal_express/pull/177