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

IA-3024: Not displaying permissions related to modules which are not linked to the account #1328

Conversation

hakifran
Copy link
Contributor

@hakifran hakifran commented May 24, 2024

Explain what problem this PR is resolving

  • Not displaying permissions related to modules which are not linked to the account
    Related JIRA tickets : IA-3024

Self proofreading checklist

  • Did I use eslint and black formatters
  • Is my code clear enough and well documented
  • Are my typescript files well typed
  • New translations have been added or updated if new strings have been introduced in the frontend
  • My migrations file are included
  • Are there enough tests
  • Documentation has been included (for new feature)

Changes

  • Check if grouped permissions is empty and skip if it is

How to test

  • Remove some modules from the current account
  • Check if some permissions groups still appearing

Print screen / video

Screencast.from.2024-05-24.16-02-47.webm

@hakifran hakifran requested a review from mathvdh May 24, 2024 14:04
@hakifran hakifran requested a review from kemar May 28, 2024 07:53
Copy link
Member

@kemar kemar left a comment

Choose a reason for hiding this comment

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

LGTM
I have a few comments on the form that you can fix if you agree with them.

Comment on lines 41 to 47
permissions = perms.filter(codename__in=PERMISSIONS_PRESENTATION[group])
if permissions:
result[group] = []
for permission in permissions:
result[group].append(
{"id": permission.id, "name": _(permission.name), "codename": permission.codename}
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
permissions = perms.filter(codename__in=PERMISSIONS_PRESENTATION[group])
if permissions:
result[group] = []
for permission in permissions:
result[group].append(
{"id": permission.id, "name": _(permission.name), "codename": permission.codename}
)
for permission in perms.filter(codename__in=PERMISSIONS_PRESENTATION[group]):
result[group] = []
result[group].append({"id": permission.id, "name": _(permission.name), "codename": permission.codename})

I think you can keep the previous code and just move the result[group] = [] line inside the for loop.

This could save a bunch of statements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But at each loop it will remove the previous added permission and at the end result[group] will have the last permission

Copy link
Member

Choose a reason for hiding this comment

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

Yes you're right!
But that's strange because I ran the unit tests with this modified code and they were successful 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

self.assertTrue( set([permission["codename"] for permission in response.json()["permissions"]["org_units"]]) <= set(PERMISSIONS_PRESENTATION["org_units"]), ). Maybe I did it wrong here?

iaso/tests/api/test_permissions.py Outdated Show resolved Hide resolved
iaso/tests/api/test_permissions.py Outdated Show resolved Hide resolved
@hakifran hakifran merged commit 09361b2 into main May 28, 2024
3 checks passed
@hakifran hakifran deleted the IA-3024-hide-grouped-permissions-in-modules-not-linked-to-account branch May 28, 2024 17:59
@kemar kemar added release Should be released in production at next deploy Released and removed release Should be released in production at next deploy labels Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants