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

chore(business-unit): graphql transformers for Company and Division models #439

Merged
merged 5 commits into from
Dec 15, 2023

Conversation

ChristianMoll
Copy link
Contributor

This PR aims to replace the current scaffolding in the Company and Division models with the real implementation.

  • Adds parentUnit field as null value in Company to prevent associated errors because of missing field
  • Adds parentUnit and topLevelUnit to buildFields where applicable
  • Adds ancestors and inheritedStores fields to graphQL transformer sprevent associated errors because of missing fields
  • Replaces topLevelUnit field with itself for Company in graphQL transformer to match referenceExpansion
  • Replaces parentUnit and topLevelUnit with Companies in Division graphQL transformer to match referenceExpansion

Copy link

changeset-bot bot commented Nov 29, 2023

🦋 Changeset detected

Latest commit: fbc0ab4

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

This PR includes changesets to release 32 packages
Name Type
@commercetools-test-data/business-unit Minor
@commercetools-test-data/quote-request Minor
@commercetools-test-data/quote Minor
@commercetools-test-data/staged-quote Minor
@commercetools-test-data/core Minor
@commercetools-test-data/graphql-types Minor
@commercetools-test-data/associate-role 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-object 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/state 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

Copy link
Contributor

@tylermorrisford tylermorrisford left a comment

Choose a reason for hiding this comment

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

Would we want a test for the new graphql transformers?

@ChristianMoll
Copy link
Contributor Author

Would we want a test for the new graphql transformers?

good catch! I will add some

replaceFields: ({ fields }) => ({
...fields,
topLevelUnit: {
...fields,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this may just need typeId: 'business-unit' but I am not yet certain as to why 😿

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm finding some other fields that may be missing from the graphql representation and should be added to the transformer: parentUnitRef, topLevelUnitRef, storesRef.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're completely right, I missed those! I'll add them to the transformer and the tests

@tylermorrisford
Copy link
Contributor

This is looking good! Do you think it would make sense to create different types for company and division? I'm wondering if that would allow for better typing, or if it would not accomplish much. I will try and get you some more eyes on this 🙏

@ChristianMoll
Copy link
Contributor Author

This is looking good! Do you think it would make sense to create different types for company and division? I'm wondering if that would allow for better typing, or if it would not accomplish much. I will try and get you some more eyes on this 🙏

Afaik the only difference is the parent unit / ref always being null/undefined for the parent unit and the unitType being Company or Division accordingly, so we would get a type that's a bit stricter for those properties and there are already Company and Division types from the SDK to use as the base. I think it's a good idea.

@ChristianMoll ChristianMoll force-pushed the CUT-business-unit-graphql-transforms branch from 6d70521 to 9561d11 Compare December 8, 2023 17:37
@valoriecarli
Copy link
Contributor

valoriecarli commented Dec 12, 2023

This is looking good! Do you think it would make sense to create different types for company and division? I'm wondering if that would allow for better typing, or if it would not accomplish much. I will try and get you some more eyes on this 🙏

Afaik the only difference is the parent unit / ref always being null/undefined for the parent unit and the unitType being Company or Division accordingly, so we would get a type that's a bit stricter for those properties and there are already Company and Division types from the SDK to use as the base. I think it's a good idea.

@tylermorrisford / @ChristianMoll I really like this and went back and forth on this issue it when I first built this. If we move forward with breaking it out, we should do the same for draft so there is symmetry (TCompany/ TDivision vs TBusinesUnit).

@tylermorrisford
Copy link
Contributor

This is looking good! Do you think it would make sense to create different types for company and division? I'm wondering if that would allow for better typing, or if it would not accomplish much. I will try and get you some more eyes on this 🙏

Afaik the only difference is the parent unit / ref always being null/undefined for the parent unit and the unitType being Company or Division accordingly, so we would get a type that's a bit stricter for those properties and there are already Company and Division types from the SDK to use as the base. I think it's a good idea.

@tylermorrisford / @ChristianMoll I really like this and went back and forth on this issue it when I first built this. If we move forward with breaking it out, should we back and do the same for draft so there is symmetry (TCompany/ TDivision vs TBusinesUnit)?

I agree. Reduce the potential smell.

@tylermorrisford tylermorrisford self-requested a review December 15, 2023 18:15
Comment on lines +20 to 25
export type TCompanyDraftGraphql = TCompanyDraft & {
__typename: 'BusinessUnitDraft';
};
export type TDivisionDraftGraphql = TDivisionDraft & {
__typename: 'BusinessUnitDraft';
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Love this - thank you for all this work!

Copy link
Contributor

@valoriecarli valoriecarli left a comment

Choose a reason for hiding this comment

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

@ChristianMoll Thank you so much for adding these much needed improvements to our initial round 💯

@ChristianMoll
Copy link
Contributor Author

@valoriecarli @tylermorrisford
thank you very much for all the great feedback and suggestions!

@ChristianMoll ChristianMoll merged commit 291a0ea into main Dec 15, 2023
3 checks passed
@ChristianMoll ChristianMoll deleted the CUT-business-unit-graphql-transforms branch December 15, 2023 22:50
@ct-changesets ct-changesets bot mentioned this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants