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(simple-pricing): Compute displayPrice as resolver as needed #6025

Merged
merged 2 commits into from
Jan 16, 2020

Conversation

focusaurus
Copy link
Contributor

Impact: minor
Type: feature

Issue

With the existing simple-pricing core plugin flow, catalogProduct.product.pricing.displayPrice is computed at publication time (ahead of time) and stored in mongodb. For the in-development Catalog Publisher component, doing the computation of displayPrice at publish time would require the full set of currency formatting I18N data and logic to be available in kafka and kafka streams.

Solution

Allow displayPrice to be computed at query time via a graphql resolver if necessary. This implementation works seamlessly with both the core simple-pricing plugin as before, but computes displayPrice as needed if the catalog plugin is disabled in favor of the catalog publisher service in a particular deployment. No configuration is necessary and data could in theory even be mixed between catalog plugin and catalog publisher.

Breaking changes

This should be fully compatible with existing clients and installations.

Testing

A unit test has been added. Manual testing is still planned/underway so I'll convert to ready for review when that is completed.

At the moment catalog publisher output data is arriving in the Catalog2 collection in mongodb which is not used at all by reaction core or plugins. To test this we'd need to retarget or rename that so it's the real Catalog collection. Undoubtedly the first time we do that some bugs will surface which we'll have to fix. Once those are fixed though, we should now be able to test this PR. If you query graphql with a query similar to below, and the displayPrice attribute looks correct, this change is working properly.

query {
  catalogItemProduct(shopId: "cmVhY3Rpb24vc2hvcDpKOEJocTN1VHRkZ3daeDNyeg==", slugOrId: "timberlands") {
    product {
      pricing {
        currency {
          rate
        }
        compareAtPrice {
          amount
        }
        displayPrice
        
      }
    }
  }
}# Write your query or mutation here

This creates compatibility with both the core catalog plugin and
catalog publisher.

Signed-off-by: Peter Lyons <pete@peterlyons.com>
Signed-off-by: Peter Lyons <pete@peterlyons.com>
Copy link
Contributor

@rosshadden rosshadden left a comment

Choose a reason for hiding this comment

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

I approve the idea and the implementation. I haven't tested manually or run the newly-added test, but when this moves out of draft mode I'll spin it up and do so.

@focusaurus
Copy link
Contributor Author

I was able to checkout with this. Seems to be working OK.

@focusaurus focusaurus marked this pull request as ready for review January 16, 2020 15:13
@focusaurus focusaurus merged commit 41e3180 into release-3.0.0 Jan 16, 2020
@focusaurus focusaurus deleted the simple-pricing-display-price-resolver branch January 16, 2020 20:58
@kieckhafer kieckhafer mentioned this pull request Jan 17, 2020
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.

3 participants