spree-contrib / spree_digital

A Spree extension to enable downloadable products
http://spreecommerce.org
MIT License
117 stars 97 forks source link

Inherit from ShippingCalculator and use correct shipping method name #68

Closed davekiss closed 9 years ago

davekiss commented 10 years ago

These adjustments fixed the shipping calculator not showing up when creating a new shipping method for me. Changes were following the guide at http://guides.spreecommerce.com/developer/calculators.html

halo commented 9 years ago

Sorry, one year later. I haven't tested it, but the specs are green. And it definitely looks like in the guide :) Even though I'm having a hard time figuring out which version the guide refers to. I guess 2ish? 3ish?

@davekiss Do you still feel like merging this in?

@JDutil Is this something for the 2-4-stable branch you've been working on, too? I'm not actively using spree right now so I have a hard time keeping all the threads together. But I'll do my best to keep this gem alive.

JDutil commented 9 years ago

@halo I'm no longer working on projects using this extension either, but I think it's still pretty important to the community in general. The changes here do look correct though. I'm hesitant to want to merge this into older stable branches though at this point if it's been working just fine as is. Perhaps just merge this to master and/or 3-0-stable.

Also we could transfer this repo over to spree-contrib to solicit more help maintaining this there.

halo commented 9 years ago

@JDutil Sounds like a plan! It's hard to give my baby away, but I'd totally do it if you think it would be beneficial for the community. Babies have to grow up ;) Do I just transfer it? Would you go ahead and catch it? Or maybe you want to fork it and I would empty it and point to spree-contrib in the readme? Don't know what's best. You tell me.

@jspizziri Would you be willing to double-check if it works with Spree 3.0 as intended? If nothing obvious breaks, I think we can go ahead and merge this into 3-0-stable (and master).

JDutil commented 9 years ago

@halo you'll still be added to it, but it'll be easier for others to also commit if that okay with you. If you add me as having admin access to this repo I can transfer it.

halo commented 9 years ago

@JDutil I'm pretty sure there is no shared admin access for personal repos. I will simply transfer it to spree-contrib now trusting that you'll find it safe and sound on the other side ;)

halo commented 9 years ago

Well look at that. It said

You don't have admin rights to spree-contrib

when I tried to transfer it.

halo commented 9 years ago

Haha, so I tried to transfer it to you.

JDutil/spree_digital already exists

:)

JDutil commented 9 years ago

I sent you an invite to spree_digital admin team for spree-contrib maybe once you accept that it will let you.

halo commented 9 years ago

@JDutil Awesome! Thank you. Github even put a redirect in place from the old location, so I don't have to think about that.

If you feel like it, you may take me off the admin team again, since I won't be adding much at the moment. But it's fine if you leave me in there, too. It would be my honor to just stick around until I may bring in something useful ;)

JDutil commented 9 years ago

Well I believe your only admin for repos I assign to the team and ones you transfer.

JDutil commented 9 years ago

I've just updated the team back to just write access that way if we want to add more committers I don't need to create yet another team.

halo commented 9 years ago

Perfect. This scales, too ;)

jspizziri commented 9 years ago

@halo sorry for the delay on this. I tested it in my 3.0 installation and didn't see any problems. For what its worth however, without this PR I'm still able to see the Digital Delivery Calculator upon creating a new Shipping Method

jspizziri commented 9 years ago

@halo, are we going to get this merged in?

halo commented 9 years ago

@jspizziri Yes, but not in the suggested 2-2-stable branch. I'd like this in master and 3-0-stable. Haven't had the time to create the pull request myself. I would merge it right away. Or, rather cherry-pick because I think there is a merge conflict.

jspizziri commented 9 years ago

Do you want me to create a PR for master?

halo commented 9 years ago

That would be lovely :)

halo commented 9 years ago

Merged into master via https://github.com/spree-contrib/spree_digital/pull/86