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

Add #total_on_hand method to Variant model #3427

Closed

Conversation

rubenrails
Copy link
Contributor

We already have that for Product, but there's no way of getting the total_on_hand for a given variant. This commit fixes that.

cc @radar


it 'should match quantifier total_on_hand' do
variant = build(:variant)
expect(variant.total_on_hand).to eq(Spree::Stock::Quantifier.new(variant).total_on_hand)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how I feel about this calling to Spree::Stock::Quantifier again. I think just a number would be more suitable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I know, I had the same impression. I didn't want to test more specifics cause there's a bunch of tests for it in the quantifier_spec, so I wasn't sure about the right approach. Any ideas?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you just need to make sure that the Variant total_on_hand sends the proper message to Quantifier. So I guess one single spec here should be enough.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alrighty, makes sense. Thank you @rubenrails + @huoxito.

radar pushed a commit that referenced this pull request Jul 26, 2013
@radar radar closed this in 6b44bc7 Jul 26, 2013
anthonycreates pushed a commit to anthonycreates/spree that referenced this pull request Jul 31, 2013
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.

None yet

3 participants