Refactor taxonomy js tree #2622

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@codeodor
Contributor

codeodor commented Feb 26, 2013

As we discussed in #2192, I re-did my changes on the split_core branch.

You've been doing a lot of great work!

I only needed to change a couple of lines on this branch to get the desired functionality: I wanted to be able to re-use the taxonomy tree editor elsewhere, which basically just required turning it into a function which accepts taxonomy_id, instead of having it load automatically based on the global taxonomy_id variable.

(On my original PR, I made a lot more changes, but looks like the Spree::API stuff takes care of all that now).

One last note: has the procedure for running the tests changed? I'm confident this change doesn't break anything in admin/taxonomies/edit, but I don't know if there were other places that used the code, because I couldn't get the tests to run.

I generated the sample app and ran bundle exec rake spec (and tried bundle exec rspec too), with no results. Also, the generated sample app doesn't seem to be there, even though it said it was generating and took a while to run.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Feb 26, 2013

Member

Hey, thanks for updating this :)

Take a look at the build.sh file in the root of the Spree repo for steps to run the tests. You need to run bundle exec rake test_app either at the root or within the component you care about to generate the dummy application for that component that Spree's tests will run against. Running bundle exec rspec spec within the component you want to test will run all of the tests.

Member

radar commented Feb 26, 2013

Hey, thanks for updating this :)

Take a look at the build.sh file in the root of the Spree repo for steps to run the tests. You need to run bundle exec rake test_app either at the root or within the component you care about to generate the dummy application for that component that Spree's tests will run against. Running bundle exec rspec spec within the component you want to test will run all of the tests.

@codeodor

This comment has been minimized.

Show comment
Hide comment
@codeodor

codeodor Feb 26, 2013

Contributor

Ok, I was doing it wrong - I tried to run the tests from the root instead of within each component.

Thanks for letting me know. I went ahead and ran ./build.sh and all of the tests pass that passed in the original split_core branch.

I stored a diff of the test outputs between the PR branch and split_core so you can determine if that's acceptable or not.

Contributor

codeodor commented Feb 26, 2013

Ok, I was doing it wrong - I tried to run the tests from the root instead of within each component.

Thanks for letting me know. I went ahead and ran ./build.sh and all of the tests pass that passed in the original split_core branch.

I stored a diff of the test outputs between the PR branch and split_core so you can determine if that's acceptable or not.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Feb 27, 2013

Member

Looks good. I'll merge to radar/spree now and run it over the CI server.

Member

radar commented Feb 27, 2013

Looks good. I'll merge to radar/spree now and run it over the CI server.

@radar radar closed this in bb4ca63 Feb 28, 2013

@codeodor

This comment has been minimized.

Show comment
Hide comment
@codeodor

codeodor Feb 28, 2013

Contributor

👍

Contributor

codeodor commented Feb 28, 2013

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment