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

Prevent duplicate content scopes in UserPermissionsService.getContentScopes #2929

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fichtnerma
Copy link
Contributor

@fichtnerma fichtnerma commented Dec 13, 2024

Description

The getContentScopes method returns duplicate Scopes

  • duplicate Scopes are now filtered before returning

Acceptance criteria

  • I have verified if my change requires an example: <!-- Unit test | Demo | Development story | No example needed --->
  • I have verified if my change requires a changeset
  • I have verified if my change requires screenshots/screencasts

Further information

@fraxachun
Copy link
Contributor

This is a known bug, but for couriosity, what does this bug affect in real life?

I see some problems here:

  • performance implications due to JSON.parse and JSON.stringify when using many content scopes
  • the mentioned functions do not unify when just the order of the fields is switched ({foo, bar} !== {bar, foo}, but it should be considered the same)
  • it's probably the wrong place because it does not unify the objects when several roundtrips (with possibly multiple invocations of UserPermissionsService.getContentScopes()) are made.
  • This PR already adresses the same problem, however it will target v8.

@johnnyomair
Copy link
Collaborator

This PR already adresses the same problem, however it will target v8.

Could we port the relevant changes to this PR?

@johnnyomair
Copy link
Collaborator

This is a known bug, but for couriosity, what does this bug affect in real life?

The chips in the user permissions admin panel show incorrect numbers.

"@comet/cms-api": patch
---

Fix a Bug that results in duplicate ContentScopes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Fix a Bug that results in duplicate ContentScopes
Prevent duplicate content scopes in `UserPermissionsService.getContentScopes`

Comment on lines +150 to +152
.map((cs) => JSON.stringify(cs))
.filter((value, index, self) => self.indexOf(value) === index)
.map((cs) => JSON.parse(cs))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer using uniqWith and isEqual instead.

@johnnyomair johnnyomair changed the title Fix a Bug resulting in duplicate ContentScopes Prevent duplicate content scopes in UserPermissionsService.getContentScopes Dec 19, 2024
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