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-38] Fix ProductProjection model reference properties #638

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

Conversation

CarlosCortizasCT
Copy link
Contributor

@CarlosCortizasCT CarlosCortizasCT commented Aug 20, 2024

Description

It all started when migrating some tests in the MC Products application which started to fail when I used the ProductProjection model.

The issue was in this specific function as we are accessing some properties of the productType property of the rest representation for the ProductProjection model.

In the test data models, whenever we need to deal with a reference, we use the Reference model which creates an object with an id and typeId properties by default. Also, in the rest transformers, we create an obj property which is supposed to hold the referenced object model but we're creating that one only with its ID and nothing else. That means that, when tests need to access any property of that referenced model, it's not there.

First thing I did is to update the Reference model to allow customization of the obj property so consumers can actually populate the reference with an actual referenced object.
Then I had to update the ProductProjection model so all references are created with a proper obj property.

After that, I also found another models that were not using references correctly so I needed to update them as well.

Technically speaking this could be a breaking change because we're changing the types of some models properties (specially in the REST version) but I see them more like a fix because the types we were using didn't seem correct.

TODO

  • There's a build error I don't understand. Any help would be appreciated.

Copy link

changeset-bot bot commented Aug 20, 2024

⚠️ No Changeset found

Latest commit: defe86c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@CarlosCortizasCT CarlosCortizasCT self-assigned this Aug 20, 2024
@CarlosCortizasCT CarlosCortizasCT added the fe-chapter-rotation Tasks coming from frontend chapter work label Aug 20, 2024
@CarlosCortizasCT CarlosCortizasCT changed the title Fix ProductProjection model reference properties [FEC-38] Fix ProductProjection model reference properties Aug 21, 2024
let stateRef: TReferenceGraphql | null = null;
if (fields.state) {
const restState = buildField(fields.state, 'rest');
stateRef = buildField(fields.state, 'graphql') as TReferenceGraphql;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the nested model in the default representation holds a Reference, it would be great that we could build the obj property of the reference for the graphql output but we can't build nested properties in isolation and the graphql transformer of the Reference model removes the obj property since it's not part of that representation. So the alternative I found is to build the rest representation and use some its values to create a new graphql model and populate it with it.

Technically this is not 100% right as we would need to overwrite all create graphql instance with the values from the rest one but I can't see a way to do it in a maintainable way.

@CarlosCortizasCT CarlosCortizasCT marked this pull request as ready for review August 21, 2024 08:01
@CarlosCortizasCT CarlosCortizasCT requested review from a team as code owners August 21, 2024 08:01
@CarlosCortizasCT CarlosCortizasCT requested review from a team and tdeekens August 21, 2024 08:01
@nima-ct
Copy link
Contributor

nima-ct commented Aug 21, 2024

Hey @CarlosCortizasCT, I just finished a fix on the StandalonePrice model transformers. If that can be of any help, let me know
#639

@CarlosCortizasCT CarlosCortizasCT requested a review from a team as a code owner August 21, 2024 19:16
@CarlosCortizasCT
Copy link
Contributor Author

Hey @CarlosCortizasCT, I just finished a fix on the StandalonePrice model transformers. If that can be of any help, let me know #639

Hi @nima-ct

Thanks for the heads up.

It seems the work of your PR is part of what I've done here.
I'll take a deeper look to yours in case I've missed something.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants