Skip to content

Consider numeric category ID in facets to obtain ads#33

Closed
amandascm wants to merge 4 commits into
mainfrom
feat/search-category-id
Closed

Consider numeric category ID in facets to obtain ads#33
amandascm wants to merge 4 commits into
mainfrom
feat/search-category-id

Conversation

@amandascm

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?

To consider numeric category IDs and translate them to their corresponding category names before sending to the Newtail API.

Test workspace with sponsored shelf containing a facet c with numeric ID

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Refactor (non-breaking change which doesn't change any functionalities)
  • Bug fix (non-breaking change which fixes an issue)
  • Chore (changes configuration files, GitHub workflows, etc.)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Requires change to documentation, which has been updated accordingly.

@amandascm amandascm requested review from fltiago and rodorgas May 6, 2025 16:30
@vtex-io-ci-cd

vtex-io-ci-cd Bot commented May 6, 2025

Copy link
Copy Markdown

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
Copy Markdown

Beep boop 🤖

I noticed you're not using the Docs Builder properly yet, if you need help to set that up please go to IO Documentation

@fltiago fltiago left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We really need to start testing this resolver as soon as possible. What do you think about addressing this issue and also adding some unit tests later?

Also, how much of a problem do you think it is to make an additional request just to check the category?

(facet) => CATEGORY_FACET_KEYS.includes(facet.key?.toLowerCase())
) || []

let categoryName: string | undefined

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This logic could be extracted into a separate method or class. Including it here makes it difficult to understand the main purpose of this method, which is to retrieve sponsored products.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I totally agree! will work on that if we keep this PR

@amandascm

Copy link
Copy Markdown
Contributor Author

Also, how much of a problem do you think it is to make an additional request just to check the category?

@fltiago In our current scenario of dealing with facets and translating them to the ad server API, at least one additional request will be required. This PR makes it for each category if they are represented as a numeric value, but it would be more like a quick win for a specific scenario than a final solution. I will keep this PR closed until we have designed the final solution!

@amandascm amandascm closed this May 7, 2025
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