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

(feat): getProduct method #3430

Merged
merged 2 commits into from Jan 24, 2018

Conversation

3 participants
@kieckhafer
Copy link
Member

kieckhafer commented Dec 22, 2017

Add getProduct method to ReactionProduct API.

@kieckhafer kieckhafer requested a review from spencern Dec 22, 2017

@pmn4

This comment has been minimized.

Copy link
Collaborator

pmn4 commented Jan 2, 2018

Hi @kieckhafer, this change seems reasonable to me, but I'm curious, when would you expect to use this method?

I was thinking that to get a product you would subscribe to the "Product" Meteor event.

Of course, this only gives you the "current" product, as defined by the URL, so I understand if you are looking to find other products. That said -- and, assuming I understand things correctly! -- in the client-side code Products.findOne(id); does not go to the database, and will return null unless 1) the "Products" event has triggered and 2) the Product you are looking for is contained in the subset of products returned in the "Products" query

Even after that, I am not saying this is not worthy of adding, I just think it could cause some headaches when called from the client... so maybe just add some additional documentation?

further, please let me know if I am misunderstanding anything!

@kieckhafer

This comment has been minimized.

Copy link
Member Author

kieckhafer commented Jan 4, 2018

@pmn4 I think you're understanding correctly.

This method stemmed from a client project, where we needed product info of a related product on our product grid. Since it was on the product grid, the related product was also published via the publication, so the data was available.

When building it out, we realized we already had ReactionProduct methods for getting getVariant and getVariantParent and getTopLevelVariant, and wanted to add this related method to make it easier to directly get product info.

@spencern
Copy link
Member

spencern left a comment

No issues with this.

@spencern spencern changed the base branch from master to release-1.6.6 Jan 24, 2018

@spencern spencern merged commit 625b5f0 into release-1.6.6 Jan 24, 2018

4 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
WIP ready for review
Details
ci/circleci Your tests passed on CircleCI!
Details
security/snyk No new issues
Details

@spencern spencern deleted the feat-kieckhafer-getProductMethod branch Jan 24, 2018

@spencern spencern referenced this pull request Jan 24, 2018

Merged

Release 1.6.6 #3565

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.