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: rating distribution #2687

Open
wants to merge 1 commit into
base: feat/reviews-and-ratings
Choose a base branch
from

Conversation

Guilera
Copy link
Collaborator

@Guilera Guilera commented Feb 13, 2025

⚠️⚠️ THIS PR DEPENDS ON: ⚠️⚠️

What's the purpose of this pull request?

To retrieves the rating distribution

How it works?

  • increments the api client to retrieves the rating distribution
  • improves graphQL resolvers with rating distribution

Important

The percentage distribution must be in integers.
Here’s the issue: The original percentages might not add up exactly to 100% because, when rounding, small errors can accumulate.
For example, if you have 27.5% and round it to 28%, you’ve added 0.5% more than the actual value. If you repeat this rounding with multiple percentages, you may end up with a total that’s slightly over or under 100%.
If the sum of the percentages is not exactly 100%, the best approach is to adjust the value of the largest percentage

If the sum is 99%: Distribute the remaining 1% to the most frequent score or to the higher scores

If the sum is 101%: Reduce the largest percentage by 1%

This process ensures that the sum of the percentages is always 100%, keeping the distribution as realistic as possible."

References

FEATURE BRANCH
JIRA SFS-2148

@Guilera Guilera requested a review from a team as a code owner February 13, 2025 19:50
@Guilera Guilera requested review from renatamottam and lucasfp13 and removed request for a team February 13, 2025 19:50
@Guilera Guilera changed the base branch from main to feat/reviews-and-ratings February 13, 2025 19:50
@Guilera Guilera mentioned this pull request Feb 13, 2025
2 tasks
Copy link

codesandbox-ci bot commented Feb 13, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@Guilera Guilera mentioned this pull request Feb 13, 2025
@Guilera Guilera requested review from eduardoformiga, lariciamota and pedromtec and removed request for renatamottam February 13, 2025 20:16
@Guilera Guilera force-pushed the feat/rating-distribution branch from e938e1e to 877747a Compare February 14, 2025 13:50
@@ -68,15 +77,31 @@ type Props = PDPContentType & {
// https://www.npmjs.com/package/deepmerge
const overwriteMerge = (_: any[], sourceArray: any[]) => sourceArray

const isClientOfferEnabled = (storeConfig as StoreConfig).experimental
.enableClientOffer

function Page({ data: server, sections, globalSections, offers, meta }: Props) {

Choose a reason for hiding this comment

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

Risk: Affected versions of next are vulnerable to Acceptance of Extraneous Untrusted Data With Trusted Data / Authorization Bypass Through User-Controlled Key.

Fix: Upgrade this library to at least version 13.5.7 at faststore/pnpm-lock.yaml:12940.

Reference(s): GHSA-gp8f-8m3g-qvj9, CVE-2024-46982

💬 To ignore this, reply with:
/fp <comment> for false positive
/ar <comment> for acceptable risk
/other <comment> for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by ssc-adb055b9-fed0-4d70-a57d-eb9825b09449.

@semgrep-code-vtex-org
Copy link

Semgrep found 3 ssc-adb055b9-fed0-4d70-a57d-eb9825b09449 findings:

Risk: Affected versions of next are vulnerable to Acceptance of Extraneous Untrusted Data With Trusted Data / Authorization Bypass Through User-Controlled Key.

Fix: Upgrade this library to at least version 13.5.7 at faststore/pnpm-lock.yaml:12940.

Reference(s): GHSA-gp8f-8m3g-qvj9, CVE-2024-46982

@Guilera Guilera changed the base branch from feat/reviews-and-ratings to main February 14, 2025 13:55
@Guilera Guilera changed the base branch from main to feat/reviews-and-ratings February 14, 2025 13:55
@Guilera Guilera force-pushed the feat/reviews-and-ratings branch from d57303d to 2203c7f Compare February 14, 2025 13:55
Copy link
Contributor

@lariciamota lariciamota left a comment

Choose a reason for hiding this comment

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

question: to get context, why there is this integer limitation on the percentages?

(percentage, index) => (rating.distribution[index + 1] = percentage)
)

console.log('distribution', rating.distribution)
Copy link
Contributor

Choose a reason for hiding this comment

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

todo: remember to delete the console log

Suggested change
console.log('distribution', rating.distribution)

Comment on lines +141 to +149
console.log('search', {
first,
after: maybeAfter,
sort,
term,
selectedFacets,
sponsoredCount,
})

Copy link
Contributor

Choose a reason for hiding this comment

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

todo: remember to delete the console log

Suggested change
console.log('search', {
first,
after: maybeAfter,
sort,
term,
selectedFacets,
sponsoredCount,
})

@@ -1008,6 +1023,13 @@ export type StoreProductGroup = {
skuVariants: Maybe<SkuVariants>
}

export type StoreProductRating = {
Copy link
Contributor

Choose a reason for hiding this comment

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

chore: This file is outdated, could you run the generate command again to update it pls?

Comment on lines +160 to +170
rating: (item) => ({
average: item.rating.average,
totalCount: item.rating.totalCount,
distribution: {
starsOne: item.rating.distribution?.[1] ?? 0,
starsTwo: item.rating.distribution?.[2] ?? 0,
starsThree: item.rating.distribution?.[3] ?? 0,
starsFour: item.rating.distribution?.[4] ?? 0,
starsFive: item.rating.distribution?.[5] ?? 0,
},
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: use destructuring to make it more direct, what do you think?

Suggested change
rating: (item) => ({
average: item.rating.average,
totalCount: item.rating.totalCount,
distribution: {
starsOne: item.rating.distribution?.[1] ?? 0,
starsTwo: item.rating.distribution?.[2] ?? 0,
starsThree: item.rating.distribution?.[3] ?? 0,
starsFour: item.rating.distribution?.[4] ?? 0,
starsFive: item.rating.distribution?.[5] ?? 0,
},
}),
rating: ({ rating: { average, totalCount, distribution } }) => ({
average,
totalCount,
distribution: {
starsOne: distribution?.[1] ?? 0,
starsTwo: distribution?.[2] ?? 0,
starsThree: distribution?.[3] ?? 0,
starsFour: distribution?.[4] ?? 0,
starsFive: distribution?.[5] ?? 0,
},
}),

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