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

feat: dont rely on sellerName when creating a request #195

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

Conversation

ghost
Copy link

@ghost ghost commented Aug 25, 2022

What problem is this solving?

Rely only on the sellerId to link a product to a seller. The solution was to get the seller name when resolving the objects that serve it to the front end.

How to test it?

Workspace

The requestId provided here is useful for testing because the sellerName was saved as an empty string. If you look for the sellerName on the graphqlIDE you should see the correct sellerName

{
  returnRequest(requestId: "1eed2aae-2399-11ed-835d-0e0377f22cbd") {
    items {
      sellerName
    }
  }
}

@vtex-io-ci-cd
Copy link

vtex-io-ci-cd bot commented Aug 25, 2022

Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖

Please select which version do you want to release:

  • Patch (backwards-compatible bug fixes)

  • Minor (backwards-compatible functionality)

  • Major (incompatible API changes)

And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.

  • No thanks, I would rather do it manually 😞

@vtex-io-docs-bot
Copy link

Beep boop 🤖

I noticed you didn't make any changes at the docs/ folder

  • There's nothing new to document 🤔
  • I'll do it later 😞

In order to keep track, I'll create an issue if you decide now is not a good time

  • I just updated 🎉🎉

@ghost ghost requested a review from filafb August 25, 2022 07:54
Copy link
Collaborator

@filafb filafb left a comment

Choose a reason for hiding this comment

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

Sorry for taking this long to review. I added a couple of questions / change requests. I mentioned a endpoint @cesarocampov mentioned to me, but I am not familiar with it. I am suggesting this because I am not sure if the seller name inside the order gets updated when it changes. Not sure how to test it, but we should try to test it. And if we go down the route to use this endpoint, we need to understand what it returns if an account is no longer a seller in a marketplace. It's worth discussing it.

@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

- Add IBAN validation in frontend and backend
- Remove IBAN and accountHolderNumber if refund method different than bank
- Stop relying on saving the sellerName at the creation of a return request. Instead get the seller name when resolving the objects that serve it to the front end.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please rebase and make sure changelog is right. The changes listed above are already released.

@@ -136,7 +136,7 @@ type ReturnRequestItem {
imageUrl: String!
unitMultiplier: Float!
sellerId: String!
sellerName: String!
sellerName: String
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we are resolving it before sending the response, we can keep it as non-nullable.

It's important to note that when you change something on GraphQL (also routes on node), depending on the changes we get a warning in the terminal like the below one. If we proceed with these change, we would have to release a new major.
Screen Shot 2022-08-30 at 6 21 18 PM

@@ -116,7 +116,7 @@
"imageUrl": { "type": "string" },
"unitMultiplier": { "type": "number" },
"sellerId": { "type": "string" },
"sellerName": { "type": "string" },
"sellerName": { "type": ["string", "null"] },
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need to make it null as well. The field is not listed as a required one, so it can be undefined.

Comment on lines +140 to +143
const { orderId, items: itemsList } = await returnRequestClient.get(
id as string,
['orderId', 'items']
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need to make another request to get the request. It is passed into this function as root, hence the type it has. Moreover, not sure the order should be the source of truth here. @cesarocampov told me about a endpoint that brings the seller name giving the seller id.

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.

2 participants