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

Issue 2868: Products at mulitple Stock Locations appear as unique variants. #2898

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@mayanktap

mayanktap commented Oct 6, 2018

Please review the change and suggest if this requires some more work.

Fix for Issue: #2868

Update variant.rb
Please review the change and suggest if this requires some more work.

@mayanktap mayanktap changed the title from Issue 2868: Repetation of same variant in case of different stock location. to Issue 2868: Products at mulitple Stock Locations appear as unique variants. Oct 6, 2018

@ericsaupe

This comment has been minimized.

Show comment
Hide comment
@ericsaupe

ericsaupe Oct 11, 2018

Contributor

I think this addresses it nicely.

A couple of notes, I would redo your commit message to better describe what is actually being changed and, if possible, write a regression test that fails for current versions of Solidus but is resolved with your fix.

Thanks!

Contributor

ericsaupe commented Oct 11, 2018

I think this addresses it nicely.

A couple of notes, I would redo your commit message to better describe what is actually being changed and, if possible, write a regression test that fails for current versions of Solidus but is resolved with your fix.

Thanks!

@jacobherrington

This comment has been minimized.

Show comment
Hide comment
@jacobherrington

jacobherrington Oct 11, 2018

Contributor

Great first PR! 🎉

Regarding Eric's suggestion on the commit messages, we have some great resources for good commit messages in contributing.md, if you're interested.

Thanks for fixing this bug!

Contributor

jacobherrington commented Oct 11, 2018

Great first PR! 🎉

Regarding Eric's suggestion on the commit messages, we have some great resources for good commit messages in contributing.md, if you're interested.

Thanks for fixing this bug!

@mayanktap

This comment has been minimized.

Show comment
Hide comment
@mayanktap

mayanktap Oct 15, 2018

@ericsaupe , @jacobherrington Thanks for your review comments.

Actually, I am familiar in using GIT from terminal instead from UI and because of that I was not able to figure out where GIT commit message is to be changed. I will make the necessary changes in the commit message.

Regarding the failing test, I check the test case and tried to run the test with and without my code which leads to the failing of same test. So as per my understanding, the failing of test case is not because of the this change.
@ericsaupe , Can you please specify what kind of regression test are you talking about?

mayanktap commented Oct 15, 2018

@ericsaupe , @jacobherrington Thanks for your review comments.

Actually, I am familiar in using GIT from terminal instead from UI and because of that I was not able to figure out where GIT commit message is to be changed. I will make the necessary changes in the commit message.

Regarding the failing test, I check the test case and tried to run the test with and without my code which leads to the failing of same test. So as per my understanding, the failing of test case is not because of the this change.
@ericsaupe , Can you please specify what kind of regression test are you talking about?

@ericsaupe

This comment has been minimized.

Show comment
Hide comment
@ericsaupe

ericsaupe Oct 17, 2018

Contributor

@mayanktap sure! Basically, a test that would do something like this:

it "returns unique products" do
  Spree::Variant.suppliable.count == variants.count
end

That's a very simple test that is obviously mostly pseudocode but the idea is that you're going to call the suppliable method and see how many variants come back and compare it with the distinct number of variants we expect. Without your fix this should fail since the suppliable method will return duplicated variants but after your fix the test should pass.

Does that make sense?

Contributor

ericsaupe commented Oct 17, 2018

@mayanktap sure! Basically, a test that would do something like this:

it "returns unique products" do
  Spree::Variant.suppliable.count == variants.count
end

That's a very simple test that is obviously mostly pseudocode but the idea is that you're going to call the suppliable method and see how many variants come back and compare it with the distinct number of variants we expect. Without your fix this should fail since the suppliable method will return duplicated variants but after your fix the test should pass.

Does that make sense?

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