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

Allow to tweak default scopes for accounts #31623

Merged
merged 1 commit into from
May 17, 2022

Conversation

tcitworld
Copy link
Member

Closes #6582, Closes #14358, Closes #14959

Replaces #20667

@szaimen
Copy link
Contributor

szaimen commented Apr 28, 2022

I like this PR 👍
But I fear I am not the person to decide... we can push forward with this :)

@szaimen
Copy link
Contributor

szaimen commented Apr 28, 2022

/backport to stable24

@szaimen
Copy link
Contributor

szaimen commented Apr 28, 2022

/backport to stable23

@szaimen
Copy link
Contributor

szaimen commented Apr 28, 2022

@tcitworld can you please resolve the conflicts? Thank you :)

@tcitworld tcitworld force-pushed the tweak-default-scopes-accounts branch from d9c879a to 256237c Compare April 29, 2022 15:03
@tcitworld
Copy link
Member Author

Done

@szaimen
Copy link
Contributor

szaimen commented Apr 29, 2022

Thanks! :)

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Minor comments, otherwise this looks good to me 👍

config/config.sample.php Show resolved Hide resolved
lib/public/Accounts/IAccountManager.php Outdated Show resolved Hide resolved
lib/public/Accounts/IAccountManager.php Outdated Show resolved Hide resolved
@Pytal
Copy link
Member

Pytal commented May 16, 2022

Could we prevent erroneously allowing displayname and email to be set to private in the logic? Unsupported as per

if (
$property->getScope() === self::SCOPE_PRIVATE
&& in_array($property->getName(), [self::PROPERTY_DISPLAYNAME, self::PROPERTY_EMAIL])
) {
if ($throwOnData) {
// v2-private is not available for these fields
throw new InvalidArgumentException('scope');
} else {
// default to local
$property->setScope(self::SCOPE_LOCAL);
}

In these cases we should default to local scope

@jancborchardt
Copy link
Member

@tcitworld can you add before/after screenshots for easier design review? Thanks! :)

@tcitworld tcitworld force-pushed the tweak-default-scopes-accounts branch from fa68b55 to 4d26a9a Compare May 16, 2022 20:55
@tcitworld
Copy link
Member Author

Could we prevent erroneously allowing displayname and email to be set to private in the logic?

Done!

can you add before/after screenshots for easier design review?

I really can't ! 😅
This is a new advanced setting for administrators through config.php only.

@juliusknorr
Copy link
Member

Failures are unrelated.

@juliusknorr juliusknorr merged commit 4c61db4 into master May 17, 2022
@juliusknorr juliusknorr deleted the tweak-default-scopes-accounts branch May 17, 2022 06:23
@szaimen szaimen added the pending documentation This pull request needs an associated documentation update label May 17, 2022
@szaimen
Copy link
Contributor

szaimen commented May 17, 2022

Should we add some documentation on this with a complete example?

@tcitworld
Copy link
Member Author

tcitworld commented May 17, 2022

Yup, needs an entry in https://docs.nextcloud.com/server/latest/admin_manual/configuration_server/config_sample_php_parameters.html

I would suggest a new section with profile.enabled.

@szaimen
Copy link
Contributor

szaimen commented May 17, 2022

There is also this: https://docs.nextcloud.com/server/latest/admin_manual/configuration_user/profile_configuration.html
Do you think we could simply add it to this section? :)

@tcitworld
Copy link
Member Author

@szaimen
Copy link
Contributor

szaimen commented May 17, 2022

But still needs a quick mention just before https://docs.nextcloud.com/server/latest/admin_manual/configuration_server/config_sample_php_parameters.html#app-config-options

If I understood this section in the settings documentation correctly, it is actually auto-generated. So I think we should not need to edit this section manually? Maybe @artonge knows?

@szaimen
Copy link
Contributor

szaimen commented May 17, 2022

But of course the profile_configuration section should then be edited and include this setting, I suppose :)

@tcitworld
Copy link
Member Author

Oh right, I missed that !

@szaimen
Copy link
Contributor

szaimen commented May 17, 2022

@tcitworld can you open a PR with some documentation on this over at https://github.com/nextcloud/documentation/edit/master/admin_manual/configuration_user/profile_configuration.rst or shall I try to add some documentation on this? :)

@tcitworld
Copy link
Member Author

Done in nextcloud/documentation#8320

@szaimen
Copy link
Contributor

szaimen commented May 17, 2022

Thank you! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement feature: users and groups php Pull requests that update Php code
Projects
None yet
6 participants