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

[FEC-88] Migrating test data model Inventory Entry #703

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

jmcreasman
Copy link
Contributor

As part of the new test data models implementation patterns, we not only will be using it for new data models but we also need to apply it for existing ones. That means we will need to eventually migrate all the current data models those new implementation patterns.

We already have documentation about how this process looks like but we also need to have a reference example engineers can use to compare. That's what this PR is for using the InventoryEntry data model.

Copy link

changeset-bot bot commented Oct 31, 2024

🦋 Changeset detected

Latest commit: 8ea98a2

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

This PR includes changesets to release 47 packages
Name Type
@commercetools-test-data/inventory-entry Minor
@commercetools-test-data/core Minor
@commercetools-test-data/graphql-types Minor
@commercetools-test-data/associate-role Minor
@commercetools-test-data/attribute-group 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-application 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/customers-search-list-my-view Minor
@commercetools-test-data/discount-code Minor
@commercetools-test-data/discounts-custom-view Minor
@commercetools-test-data/filter-values Minor
@commercetools-test-data/order Minor
@commercetools-test-data/organization-extension Minor
@commercetools-test-data/organization Minor
@commercetools-test-data/payment Minor
@commercetools-test-data/platform-limits Minor
@commercetools-test-data/product-discount Minor
@commercetools-test-data/product-projection Minor
@commercetools-test-data/product-selection Minor
@commercetools-test-data/product-type Minor
@commercetools-test-data/product Minor
@commercetools-test-data/project-extension Minor
@commercetools-test-data/project Minor
@commercetools-test-data/quote-request Minor
@commercetools-test-data/quote Minor
@commercetools-test-data/review Minor
@commercetools-test-data/shipping-method Minor
@commercetools-test-data/shopping-list Minor
@commercetools-test-data/staged-quote 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/user Minor
@commercetools-test-data/zone Minor
@commercetools-test-data/utils Minor
@commercetools-test-data/generators 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

@jmcreasman jmcreasman self-assigned this Oct 31, 2024
@jmcreasman jmcreasman added 🚧 Status: WIP fe-chapter-rotation Tasks coming from frontend chapter work labels Oct 31, 2024
@jmcreasman
Copy link
Contributor Author

I wasn't sure if this PR was also supposed to delete the two unneeded files or if that would come later?

__typename: 'InventoryEntry',
supplyChannelRef: fake((f) => f.string.uuid()),
},
postBuild: (model) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not 100% sure if I did this part correctly?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me 👍

@@ -7,6 +7,9 @@ import type { TReferenceGraphql } from '@commercetools-test-data/commons';
import type { TBuilder } from '@commercetools-test-data/core';

// Default
/**
* @deprecated use `TInventoryEntryRest` instead
*/
export type TInventoryEntry = Omit<InventoryEntry, 'supplyChannel'> & {
supplyChannel: Channel;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this PR do I need to do anything about the InventoryEntryDraft? I didn't see anything about that in the docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we need to migrate that model as well.

/**
* @deprecated Use `InventoryEntryRest` or `InventoryEntryGraphql` exported models instead of `InventoryEntry`.
*/
export const deprecatedInventoryEntry = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally I had this looking like the doc example but I was getting this error when trying to commit:

models/inventory-entry/src/index.ts:8:1
✖ 8:1 Multiple exports of name random. import/export
✖ 8:1 Multiple exports of name presets. import/export
✖ 26:14 Multiple exports of name random. import/export
✖ 27:14 Multiple exports of name presets. import/export

So I changed it to this and that made it happier. Let me know if that's not correct though.

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 that was because the deraft model has not been migrated yet.

@jmcreasman jmcreasman marked this pull request as ready for review November 1, 2024 16:27
@jmcreasman jmcreasman requested a review from a team as a code owner November 1, 2024 16:27
__typename: 'InventoryEntry',
supplyChannelRef: fake((f) => f.string.uuid()),
},
postBuild: (model) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me 👍

@@ -1,3 +1,29 @@
const presets = {};
import type { TBuilder } from '@commercetools-test-data/core';
Copy link
Contributor

Choose a reason for hiding this comment

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

The index file of presets should export just plain objects with the presets for each representation.
In this case, we don't have any presets implemented for this model, so it would be something like this:

export const restPresets = {};
export const graphqlPresets = {};

.changeset/metal-mice-sit.md Outdated Show resolved Hide resolved
models/inventory-entry/src/index.ts Show resolved Hide resolved
Comment on lines 1 to 2
// export { default as random } from './builder';
// export { default as presets } from './presets';
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this commented code

CompatInventoryEntryModelBuilder,
} from './builders';
import * as modelPresets from './presets';

export * as InventoryEntryDraft from './inventory-entry-draft';
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to export the three models with heir own names:

Suggested change
export * as InventoryEntryDraft from './inventory-entry-draft';
export * from './inventory-entry-draft';

@CarlosCortizasCT
Copy link
Contributor

@jmcreasman We still need to refactor the sample data presets. I think it would be helpful to have a meeting to clarify this point.

@jmcreasman
Copy link
Contributor Author

jmcreasman commented Nov 7, 2024

@jmcreasman We still need to refactor the sample data presets. I think it would be helpful to have a meeting to clarify this point.

@CarlosCortizasCT - Yes I generated it from the test data generation repo and added it in, but then realized the data it generated wasn't in the new format/implementation even when I made it v2 like in the guide. I tested other entities like Channel and it worked as expected so I've asked the First Contact team about this and should have a reply this morning (many of them were out yesterday).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fe-chapter-rotation Tasks coming from frontend chapter work 🚧 Status: WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants