Skip to content

Conversation

@albertm805
Copy link
Contributor

What problem is this solving?
Removes formatting from skuId if the value from the admin is itemId

How should this be manually tested?
This can be tested here

Screenshots or example usage:
Contour Vest - Dunnes Stores 2025-07-30 at 1 29 54 AM

@albertm805 albertm805 requested a review from a team as a code owner July 30, 2025 07:30
@albertm805 albertm805 requested review from leo-prange-vtex, vmourac-vtex and vsseixaso and removed request for a team July 30, 2025 07:30
@vtex-io-ci-cd
Copy link

vtex-io-ci-cd bot commented Jul 30, 2025

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

vtex-io-docs-bot bot commented Jul 30, 2025

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

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes GTIN value handling by conditionally skipping formatting when the gtinValue configuration is set to 'itemId'. The change prevents unnecessary padding of item IDs that should remain in their original format.

  • Adds conditional logic to skip GTIN formatting for itemId values
  • Updates changelog to document the fix

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
react/Product.js Implements conditional GTIN formatting logic based on gtinValue type
CHANGELOG.md Documents the fix for GTIN formatting when value is itemId

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

react/Product.js Outdated
Comment on lines 228 to 233
const gtin =
rawGTIN != null
? gtinValue === 'itemId'
? rawGTIN // don't format itemId
: formatGTIN(rawGTIN)
: null
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

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

[nitpick] The nested ternary operator creates complex logic that's hard to read. Consider refactoring to use an if-else structure or extracting the logic into a helper function for better maintainability.

Suggested change
const gtin =
rawGTIN != null
? gtinValue === 'itemId'
? rawGTIN // don't format itemId
: formatGTIN(rawGTIN)
: null
let gtin;
if (rawGTIN != null) {
if (gtinValue === 'itemId') {
gtin = rawGTIN; // don't format itemId
} else {
gtin = formatGTIN(rawGTIN);
}
} else {
gtin = null;
}

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

@copilot can you simplify the nested if-else statements? for instance, by initializing the gtin value with null?

Copy link
Contributor

@iago1501 iago1501 left a comment

Choose a reason for hiding this comment

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

Hi @albertm805 , first of all, thanks for the PR

Just did the tech code review

After reviewing the code, I have one question: Why do we need to store the GTIN in an unformatted state? Given that GTINs are strictly 8, 12, 13, or 14 characters long, wouldn't padding with zeros be enough? The example you provided shows a 5-character number - can you help me understand the reasoning behind this approach?

@albertm805
Copy link
Contributor Author

Hi @albertm805 , first of all, thanks for the PR

Just did the tech code review

After reviewing the code, I have one question: Why do we need to store the GTIN in an unformatted state? Given that GTINs are strictly 8, 12, 13, or 14 characters long, wouldn't padding with zeros be enough? The example you provided shows a 5-character number - can you help me understand the reasoning behind this approach?

The reason why I am storing the GTIN in a raw state is because it can be any length value. If you consider that it shouldn't be done it can be changed.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

react/Product.js Outdated

let gtin = null

if (rawGTIN != null) {
Copy link

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

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

Use strict equality (!==) instead of loose equality (!=) for null comparison to follow JavaScript best practices and avoid type coercion issues.

Suggested change
if (rawGTIN != null) {
if (rawGTIN !== null && rawGTIN !== undefined) {

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iago1501 is this something also an enhancement that will go with the PR or is there some other alternative code that you would like for me to include?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is an antomatic review made by copilot, it's suggesting an enhancement in your code, it's related to best practices in js:

Use strict equality (!==) instead of loose equality (!=) for null comparison to follow JavaScript best practices and avoid type coercion issues.

It would be great if you could commit this suggestion in the PR since you are using explicit null comparison

@iago1501
Copy link
Contributor

Hi @albertm805 , regarding the GTIN value,

The reason why I am storing the GTIN in a raw state is because it can be any length value. If you consider that it shouldn't be done it can be changed.

Regarding the GTIN value, I understand that it can technically have any length, but perhaps it would be a good idea to follow what’s recommended in the documentation. If it specifies that GTIN should contain only 8, 12, 13, or 14 characters, I think it might be best to stick to those standards, even if that means adding leading zeros where necessary.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@iago1501 iago1501 left a comment

Choose a reason for hiding this comment

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

Just some semantical fixes

@albertm805 albertm805 requested a review from iago1501 December 3, 2025 06:19
@vmourac-vtex vmourac-vtex merged commit 81d3fdd into vtex-apps:master Dec 8, 2025
4 checks passed
@vtex-io-ci-cd
Copy link

vtex-io-ci-cd bot commented Dec 8, 2025

Your PR has been merged! App is being published. 🚀
Version 0.16.0 → 0.16.1

After the publishing process has been completed (check #vtex-io-releases) and doing A/B tests with the new version, you can deploy your release by running:

vtex deploy vtex.structured-data@0.16.1

After that your app will be updated on all accounts.

For more information on the deployment process check the docs. 📖

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.

4 participants