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

Pangolin 3184: staged quote model #433

Merged
merged 8 commits into from
Nov 21, 2023
Merged

Conversation

ByronDWall
Copy link
Contributor

This PR adds a new package for the StagedQuote and StagedQuoteDraft models.

Copy link

changeset-bot bot commented Nov 21, 2023

🦋 Changeset detected

Latest commit: 4437a73

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 28 packages
Name Type
@commercetools-test-data/quote-request Minor
@commercetools-test-data/staged-quote Minor
@commercetools-test-data/core Minor
@commercetools-test-data/graphql-types Minor
@commercetools-test-data/business-unit Minor
@commercetools-test-data/cart-discount Minor
@commercetools-test-data/cart Minor
@commercetools-test-data/category Minor
@commercetools-test-data/channel Minor
@commercetools-test-data/commons Minor
@commercetools-test-data/custom-view Minor
@commercetools-test-data/customer-group Minor
@commercetools-test-data/customer Minor
@commercetools-test-data/discount-code Minor
@commercetools-test-data/inventory-entry Minor
@commercetools-test-data/order Minor
@commercetools-test-data/payment Minor
@commercetools-test-data/product-discount Minor
@commercetools-test-data/product-selection Minor
@commercetools-test-data/product-type Minor
@commercetools-test-data/product Minor
@commercetools-test-data/review Minor
@commercetools-test-data/shipping-method Minor
@commercetools-test-data/shopping-list Minor
@commercetools-test-data/store Minor
@commercetools-test-data/tax-category Minor
@commercetools-test-data/zone Minor
@commercetools-test-data/utils Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

stateRef: TReferenceGraphql | null;
__typename: 'StagedQuote';
};
export type TStagedQuoteDraftGraphql = TStagedQuoteDraft;
Copy link
Contributor

Choose a reason for hiding this comment

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

Always a sigh of relief when they are symmetrical

Copy link
Contributor

@jaikamat jaikamat left a comment

Choose a reason for hiding this comment

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

Spent time comparing types and they look good to me (with one nit)! My thinking is if those are good then the rest should flow from there.

Nice work as always 🏆

… reflect that there is only a cartRef returned by the graphql api, it does not return the full cart data, also update staged quote types to reflect that quotationCartRef and quoteRequestRef are non-nullable
@@ -56,7 +57,7 @@ const generator = Generator<TQuoteRequest>({
directDiscounts: [],
state: null,
purchaseOrderNumber: null,
cart: fake(() => Cart.random()),
cart: fake(() => Reference.random().typeId('cart')),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes an error I made in building the graphql data for QuoteRequest, cart is the only field that only returns a Reference, and not the full data, i.e. the graphql api returns a cartRef field, but does not return cart
Screenshot 2023-11-21 at 10 28 05 AM

@ByronDWall ByronDWall requested a review from jaikamat November 21, 2023 16:02
@@ -139,7 +133,7 @@ const transformers = {

const cartRef: TReferenceGraphql = Reference.presets.cartReference
.cartReference()
.id(fields.cart?.id)
.id(fields.cart?.id || '')
Copy link
Contributor

Choose a reason for hiding this comment

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

Ninja move!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, this is exactly the sort of thing that I'm never sure is best practice in TS, so I'm glad it's not throwing up any flags for you

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be me, and honestly there could be a way better action to take here, but there are instances where the graphql types seem to back us into a corner a little bit. @jaikamat do you have thoughts?

Copy link
Contributor

@jaikamat jaikamat Nov 21, 2023

Choose a reason for hiding this comment

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

Something like

const cartRef: TReferenceGraphql | null = fields.cart
  ? Reference.presets.cartReference
      .cartReference()
      .id(fields.cart.id)
      .typeId('cart')
      .buildGraphql()
  : null;

could be more in line with the schema since cartRef is nullable, and this way we know id is available

Copy link
Contributor

Choose a reason for hiding this comment

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

(full disclosure I did that here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jaikamat updated

@ByronDWall ByronDWall merged commit 3622e5d into main Nov 21, 2023
@ByronDWall ByronDWall deleted the PANGOLIN-3184-staged-quote-model branch November 21, 2023 18:36
@ct-changesets ct-changesets bot mentioned this pull request Nov 21, 2023
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