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

[Seller Quotes] feat: verify if seller accepts quotes to process splitting and notify #59

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

tiago-freire
Copy link

@tiago-freire tiago-freire commented Dec 11, 2024

What problem is this solving?

Calls b2b-seller-quotes to verify if seller accepts quotes and to notify when a new quote is created.

How to test it?

  • Clone branch feat/verify-and-notify-quotes-endpoints of b2b-seller-quotes an execute pnpm dev:vtex
  • Clone the branch of this PR, switch to bravtexfashionb2b account and link b2b-quotes-graphql on workspace b2bsellerquoteslocalhost (the same workspace name generated by pnp:dev:vtex on b2b-seller-quotes in the indicated branch of previous step)
  • Create a quote that contains product of seller shopfashion568: https://b2bsellerquoteslocalhost--bravtexfashionb2b.myvtex.com/camisa-feminina-gray/p
  • Observe the server logs of b2b-seller-quotes, where there should be 2 received requests:
    • /b2b-seller-quotes/_v/0/verify-quote-settings
    • /b2b-seller-quotes/_v/0/notify-new-quote

Workspace

Copy link

vtex-io-ci-cd bot commented Dec 11, 2024

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 🎉🎉

@tiago-freire
Copy link
Author

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 😞

@tiago-freire
Copy link
Author

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 🎉🎉

@tiago-freire tiago-freire changed the title [Seller Quotes] feat: verify if seller accepts quotes and notify seller with quote as payload [Seller Quotes] feat: verify if seller accepts quotes to process splitting and notify Dec 14, 2024
@@ -59,6 +59,10 @@ type Quote {
viewedBySales: Boolean
viewedByCustomer: Boolean
salesChannel: String
seller: String
approvedBySeller: Boolean
parentQuote: String
Copy link
Contributor

@Matheus-Aguilar Matheus-Aguilar Dec 17, 2024

Choose a reason for hiding this comment

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

After the split, all quotes are independent. So there is no need to keep a parent quote or children data.

Copy link
Author

Choose a reason for hiding this comment

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

Without a link between the sellers quotes and the "parent" quote, it would not be possible to create this screen, for example: https://www.figma.com/design/SDETDCU5obc4Kg4WtmG6FH/RedCloud-%C2%B7-Seller-Quotes-%5BCubos%5D?node-id=263-24328&t=daYlrwPapSKRVfZN-4.

Copy link
Author

Choose a reason for hiding this comment

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

The where assembly that brings the quotes at marketplace side was changed because of this, always adding parentQuote is null in searches: https://github.com/vtex-apps/b2b-quotes-graphql/pull/59/files#diff-9766252b6f3131ecfc785f3869d3129273b19c4a91b52963b62c5d3803c964d7

Copy link
Contributor

Choose a reason for hiding this comment

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

The "link" between the quotes should be just a name or label and not a parent quote. It's name displayed in this screen

Copy link
Author

Choose a reason for hiding this comment

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

a relationship based on a string that is input by the user?

@@ -59,6 +59,10 @@ type Quote {
viewedBySales: Boolean
viewedByCustomer: Boolean
salesChannel: String
seller: String
approvedBySeller: Boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need to have a field only for the seller's approval. A seller quote should use the same status as a regular quote. The only additional information is the seller's name, so you can query this information later.

Copy link
Author

Choose a reason for hiding this comment

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

We understand that once the seller accepts the quote, there is no need to make the request to the seller again. approvedBySeller starts with false when the seller is notified and can become true depending on their approval. The first time the marketplace requests this data, it can save it so that it does not need to be requested again if the seller has approved the quote.

Copy link
Author

Choose a reason for hiding this comment

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

On second thought, i think you are right. We can just use the status for seller approval control.

Copy link
Author

Choose a reason for hiding this comment

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

Done at f6a1117

email,
const parentQuoteItems = hasSellerQuotes
? items.filter(
(item) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't there a more efficient way of doing this filtering? Like, filter all items with seller === '1' (opposite of the previous filter)?

Copy link
Author

@tiago-freire tiago-freire Dec 17, 2024

Choose a reason for hiding this comment

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

No, because there may be a seller in the quote items who:

  • does not have b2b-seller-quotes installed
  • does have b2b-seller-quotes installed, but has the setting to not accept quotes

In these cases, these quote items remains under the management of the marketplace.

// subtotal
// )
// : subtotal
const parentQuote = createQuoteObject({
Copy link
Contributor

Choose a reason for hiding this comment

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

If there are no items from the marketplace, the parent quote should not be created.

Copy link
Author

Choose a reason for hiding this comment

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

There needs to be a link between the sellers quotes and the "parent" quote, as we said in these comment.

@@ -345,7 +352,10 @@ export const checkConfig = async (ctx: Context) => {
return null
}

if (!settings?.adminSetup?.cartLifeSpan) {
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this check be an OR instead of an AND?

Copy link
Author

Choose a reason for hiding this comment

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

This is a check to return to the default settings if no setting is filled in yet. Therefore, it has to be AND.

})

// The ternary check is to not request again from the same seller
const verifyResponse = quoteBySeller[seller]
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 many calls would still be made if there was a seller who does not accept quotes with multiple items, since it would not be included in the quoteBySeller map.

Copy link
Author

Choose a reason for hiding this comment

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

makes sense. let's analyze this.

const { seller } = item

const next = async () =>
splitItemsBySeller({
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a recursive approach really necessary here? I think that an iterative approach would be simpler and more efficient.

Copy link
Author

Choose a reason for hiding this comment

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

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.

3 participants