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

added support for batch creation of gift cards. need to write tests #22

Closed
wants to merge 3 commits into from

Conversation

schontz
Copy link

@schontz schontz commented Jun 18, 2013

I haven't written the tests yet because I can't get the tests to run and would need some direction. Right now, this code is living in vendor/spree_gift_card in my spree app. When I run the rspec tests from the app it doesn't get these tests and if I try and run the tests in vendor/spree_gift_card it doesn't work because it isn't a running environment. Any advice?

Also, this is my first attempt at sharing my updates into an existing rails application so if you notice any newbie issues with style or code please let me know.

@JDutil
Copy link
Owner

JDutil commented Jun 27, 2013

Thanks for the effort you've put into this, but I'd really like to get some specs around the behavior before merging in. I'll take a look and see what I can do about getting the specs running for you.

@schontz
Copy link
Author

schontz commented Jun 27, 2013

That's my thought as well. I mainly put in the pull request to get the ball rolling in that direction.

On Thu, Jun 27, 2013 at 12:37 PM, Jeff Dutil [email protected]
wrote:

Thanks for the effort you've put into this, but I'd really like to get some specs around the behavior before merging in. I'll take a look and see what I can do about getting the specs running for you.

Reply to this email directly or view it on GitHub:
#22 (comment)

@JDutil
Copy link
Owner

JDutil commented Sep 7, 2013

FYI I've fixed the spec suite with the changes I made today. If you could work on some specs for this now that would be great.

@JDutil
Copy link
Owner

JDutil commented Feb 20, 2014

I'm sorry thank you for your work on this, but I am no longer going to be adding new features as work has begun on a replacement extension for 2.2.x+ If you would like to resubmit with specs I will still consider merging, but there has been months without movement so closing out

@JDutil JDutil closed this Feb 20, 2014
@schontz
Copy link
Author

schontz commented Feb 20, 2014

Not a problem. If this is something that the community would like in the future I can see about updating it for the latest release. I don't have time to maintain or polish it right now.

On Feb 20, 2014, at 1:39 PM, Jeff Dutil [email protected] wrote:

I'm sorry thank you for your work on this, but I am no longer going to be adding new features as work has begun on a replacement extension for 2.2.x+ If you would like to resubmit with specs I will still consider merging, but there has been months without movement so closing out


Reply to this email directly or view it on GitHub.

@JDutil
Copy link
Owner

JDutil commented Feb 20, 2014

That fine I'm sure people would enjoy it, but I'd like to be tested before it's merged. Please also read #49 which is the reasoning behind this decision. I know we will want batch creation for the new ext so I'd rather save effort for there.

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.

2 participants