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

[User Accounts] Permissions panel fix #7451

Merged

Conversation

AlexandraLivadas
Copy link
Contributor

The permissions panel on the edit user page is not correctly displaying Permission descriptions for user's with superuser permission. This is because of a superuser permissions check which returned a list of permissions without a label parameter specified.

View before:
Screen Shot 2021-05-14 at 2 49 21 PM

View after:
Screen Shot 2021-05-14 at 2 48 43 PM

Testing instructions (if applicable)

  1. Go to edit a user as a superuser and check that the permissions are correctly displayed with their descriptions
  2. Verify that the permissions are also displayed correctly for a user who is not a superuser

@AlexandraLivadas AlexandraLivadas added the Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) label May 14, 2021
@laemtl
Copy link
Contributor

laemtl commented May 17, 2021

@AlexandraLivadas Tests are failing.

1) UserTest::testGetPermissionsVerbose
PDOStatement::execute(): SQLSTATE[HY093]: Invalid parameter number: number of bound variables does not match number of tokens

/app/php/libraries/Database.class.inc:708
/app/php/libraries/Database.class.inc:745
/app/php/libraries/UserPermissions.class.inc:312
/app/test/unittests/UserTest.php:1127

@AlexandraLivadas
Copy link
Contributor Author

@laemtl The tests seem to be passing fine on my sandbox so I will re-run Travis. If it still is failing, I will debug :)

@christinerogers christinerogers added Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix Critical to release PR or issue is key for the release to which it has been assigned labels May 26, 2021
@christinerogers
Copy link
Contributor

Tests failing, @AlexandraLivadas

@ridz1208
Copy link
Collaborator

@AlexandraLivadas any news on this? do you need help with the tests ?

@AlexandraLivadas AlexandraLivadas force-pushed the 2021-05-14_user_accounts_permissions_fix branch from 9f2d9a7 to 726455e Compare July 6, 2021 13:51
@AlexandraLivadas
Copy link
Contributor Author

@laemtl @kongtiaowang This is ready for review

@laemtl laemtl removed the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Jul 26, 2021
FROM permissions p ";

//If not superuser, get the permissions that the user has
if (intval($this->permissions['superuser']) != 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not using hasPermission('superuser')?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will checkout this branch and make sure there was a reason for this decision :)

FROM permissions p ";

//If not superuser, get the permissions that the user has
if ($this->hasPermission('superuser')) {
Copy link
Contributor

@laemtl laemtl Aug 16, 2021

Choose a reason for hiding this comment

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

I think this line should be:
if (!$this->hasPermission('superuser')) {
Then your new test case should start working again. You can try to add it back. Thanks!

@laemtl laemtl added the Passed Manual Tests PR has undergone proper testing by at least one peer label Aug 16, 2021
@driusan driusan merged commit ed21681 into aces:main Aug 26, 2021
@ridz1208 ridz1208 added this to the 24.0.0 milestone Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) Critical to release PR or issue is key for the release to which it has been assigned Passed Manual Tests PR has undergone proper testing by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[User accounts] Permission panel issue
5 participants