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

Add possibility to update the account.visibility field via the user endpoint #134

Closed
kxkv opened this issue Sep 3, 2023 · 8 comments
Closed
Labels
enhancement New feature or request

Comments

@kxkv
Copy link
Contributor

kxkv commented Sep 3, 2023

It should be possible to update the visibility value via the user endpoint. Today, when you try updating the account.visibility value, nothing happens.

Example code (Python) that should work:

    user_id = 24
    data_object = {
            'account': {
                  'visibility': 3
            }
        }
    response = frhumhub.user.update(user_id, data_object)
@kxkv kxkv changed the title Add possibility to update visibility field in the user endpoint Add possibility to update the account.visibility field via the user endpoint Sep 3, 2023
@luke- luke- added the enhancement New feature or request label Sep 3, 2023
@kxkv
Copy link
Contributor Author

kxkv commented Sep 5, 2023

I have been digging around in the code, trying to see if there was an easy fix to this one. I might very well be wrong, as PHP is not my cup of tea. However, I can not see anywhere in the REST module where this value should be, and is missing. So, the fact that the visibility field is not updated might be due to a bug in HumHub itself.

https://github.com/introspectionism/rest/blob/master/controllers/user/UserController.php
https://github.com/introspectionism/rest/blob/master/models/ApiUser.php

@luke-
Copy link
Contributor

luke- commented Sep 5, 2023

Currently this field is not implemented in the REST API.
https://marketplace.humhub.com/module/rest/docs/html/user.html#tag/User/operation/updateUser

@marc-farre
Copy link
Contributor

marc-farre commented Sep 18, 2023

@luke- PR #140

I had to add $this->user->scenario = User::SCENARIO_EDIT_ADMIN; in ApiUser::load() because the rules in the User model about the visibility property is set only with this scenario: https://github.com/humhub/humhub/blob/master/protected/humhub/modules/user/models/User.php#L163

I hope adding this scenario doesn't prevent other property from loading when updating a user with the REST API, but it don't think it does.

An alternative would be to remove the , 'on' => Profile::SCENARIO_EDIT_ADMIN] in the User rules to have the same behavior as the status property.

Thanks @introspectionism for financing this development.

@yurabakhtin
Copy link
Contributor

@luke- @marc-farre I have done this core PR humhub/humhub#6577 because here https://github.com/humhub/humhub/pull/6577/files#diff-d3a4745ae7c92bb2ed3212a38c146f5f29268cf40a693d007f752f679a1f52abL163 we had the wrong Profile::SCENARIO_EDIT_ADMIN instead of correct User::SCENARIO_EDIT_ADMIN.


I find here https://github.com/humhub/humhub/blob/master/protected/humhub/modules/admin/controllers/UserController.php#L111 we have a condition:

  • $canEditAdminFields = Yii::$app->user->isAdmin() || !$user->isSystemAdmin();

It is used to allow update the columns status and visibilty only by Admin or if the updating user is not admin.
I think this restriction should be applied here in the REST module.
I.e. users with premission "Manage Users" can update these columns only of not admin users.

@luke-
Copy link
Contributor

luke- commented Sep 19, 2023

@yurabakhtin I've merged the core PR. The 1.14.4 should be released soon.

@marc-farre
Copy link
Contributor

Thanks @yurabakhtin
See my comment #140 (comment)

luke- added a commit that referenced this issue Sep 20, 2023
…user

Enh #134: Implementation of the user account `visibility` property in the "Update an existing user" endpoint
@marc-farre
Copy link
Contributor

Thanks @luke- and @yurabakhtin .
@introspectionism You can close the issue now the PR is merged into master.

@kxkv
Copy link
Contributor Author

kxkv commented Sep 20, 2023

Thank you, guys!

@kxkv kxkv closed this as completed Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants