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

FCT-702: add Associate and Associate-Role-Assignment models #481

Merged
merged 7 commits into from
Feb 14, 2024

Conversation

tylermorrisford
Copy link
Contributor

Adds the Associate and Associate-Role-Assignment models, and splits the types/constants for the Business-Unit package so that Company and Division have their own types/constants.

Reviewers: Beyond the addition of the two models, this PR will appear large but is mostly rearranging the location of types and (admittedly duplicating) constants. Would love your thoughts on the colocation and whether or not we should document this pattern.

Copy link

changeset-bot bot commented Feb 12, 2024

🦋 Changeset detected

Latest commit: 7c7992d

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

This PR includes changesets to release 34 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/standalone-price Minor
@commercetools-test-data/state Minor
@commercetools-test-data/store Minor
@commercetools-test-data/tax-category Minor
@commercetools-test-data/type 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

models/business-unit/src/associate/generator.ts Outdated Show resolved Hide resolved
models/business-unit/src/associate/generator.ts Outdated Show resolved Hide resolved
models/business-unit/src/associate/types.ts Outdated Show resolved Hide resolved
Comment on lines +34 to +35
const customerRef: TReferenceGraphql = Reference.presets.customerReference
.customerReference()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really the syntax? customerReference repeated twice??

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is in fact legal syntax, should probably change this

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, who did that??

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do change it, make sure to change it everywhere it's used
Screenshot 2024-02-13 at 1 34 30 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made a ticket for this 😎

.changeset/polite-ladybugs-smile.md Outdated Show resolved Hide resolved
__typename: 'BusinessUnit';
storesRef: KeyReference;
parentUnitRef: null;
parentUnit?: BusinessUnitKeyReference;
Copy link
Contributor

Choose a reason for hiding this comment

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

In a Company the parentUnit is always null and the topLevelUnit is a self-reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @NickDevG! ❤️ I am confused about the distinction between the two in regard to their Graphql entities. I may tag this and work on it in another PR, but is there a good place to read on their differences other than the HTTP or Impex Docs?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tylermorrisford this is in the http API docs, but maybe is what youre looking for?
https://docs.commercetools.com/api/customers-overview#hierarchies-within-business-units

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NickDevG We're making a new ticket to fix the Company type and to add the CompanyDraft and DivisionDraft transformers in a separate PR.

Copy link
Contributor

@ByronDWall ByronDWall left a comment

Choose a reason for hiding this comment

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

Pulled it down, ran the tests, checked the shapes, this looks good to me aside from Nico's comments

@tylermorrisford tylermorrisford merged commit 041993b into main Feb 14, 2024
3 checks passed
@tylermorrisford tylermorrisford deleted the FCT-702-associate branch February 14, 2024 21:07
@ct-changesets ct-changesets bot mentioned this pull request Feb 14, 2024
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.

7 participants