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.

Sign up for GitHub

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

Jump to bottom

Allow to tweak default scopes for accounts #20667

Closed
wants to merge 1 commit into from
Closed

Allow to tweak default scopes for accounts #20667

wants to merge 1 commit into from

Conversation

tcitworld
Copy link
Member

@tcitworld tcitworld commented Apr 26, 2020 •
edited by kesselb
Loading

Closes #6582, Closes #14358, Closes #14959

Lacks proper tests

Needs to be documented afterwards.

@tcitworld tcitworld added 3. to review Waiting for reviews feature: users and groups php Pull requests that update Php code labels Apr 26, 2020
@tcitworld tcitworld added this to the Nextcloud 20 milestone Apr 26, 2020
@tcitworld tcitworld force-pushed the tweak-default-scopes-accounts branch from 25228f6 to 6e430b3 Compare April 26, 2020 11:14
@tcitworld tcitworld force-pushed the tweak-default-scopes-accounts branch 2 times, most recently from abca320 to 5da042f Compare April 26, 2020 11:38
kesselb
kesselb reviewed Apr 26, 2020
View reviewed changes
Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

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

Good stuff 👍

lib/private/Accounts/AccountManager.php Outdated Show resolved Hide resolved
config/config.sample.php Outdated Show resolved Hide resolved
@tcitworld tcitworld force-pushed the tweak-default-scopes-accounts branch from 5da042f to f4fe762 Compare April 27, 2020 16:59
@tcitworld tcitworld mentioned this pull request May 27, 2020
Closed
nickvergessen
nickvergessen reviewed Jun 4, 2020
View reviewed changes
lib/private/Accounts/AccountManager.php
private $config;

public const DEFAULT_SCOPE_VALUES = [
self::PROPERTY_DISPLAYNAME => self::VISIBILITY_CONTACTS_ONLY,
Copy link
Member

Choose a reason for hiding this comment

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

We should really make the default on display name and avatar public.
Talk and some other apps are completly unusable at the moment when the settings would be respected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you want to have the change in this PR or shall I open an issue to discuss instead?

ChristophWurst
ChristophWurst reviewed Jun 4, 2020
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Nitpicks on naming

lib/private/Accounts/AccountManager.php
/** @var IConfig */
private $config;

public const DEFAULT_SCOPE_VALUES = [
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
public const DEFAULT_SCOPE_VALUES = [
public const DEFAULT_SCOPES = [

lib/public/Accounts/IAccountManager.php
@@ -52,6 +59,16 @@ interface IAccountManager {
public const PROPERTY_ADDRESS = 'address';
public const PROPERTY_TWITTER = 'twitter';

public const PROPERTIES_LIST = [
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
public const PROPERTIES_LIST = [
public const PROPERTIES = [

@Dubidubiduu Dubidubiduu mentioned this pull request Jul 21, 2020
Closed
@kesselb
Copy link
Contributor

kesselb commented Jul 25, 2020

@tcitworld 👋

@nickvergessen
Copy link
Member

Btw the meaning (and labels) were changed in the meantime and they are now not about visibility but whether they are synced to trusted server and the public addressbook/lookup server.

So maybe visibility is a wrong constant name nowadays

@MorrisJobke MorrisJobke mentioned this pull request Aug 11, 2020
Merged
57 tasks
@kesselb kesselb mentioned this pull request Aug 24, 2020
Closed
@Dubidubiduu

This comment has been minimized.

Sign in to view
This was referenced Dec 14, 2020
Merged
Merged
This was referenced Dec 28, 2020
Merged
Merged
@rullzer rullzer modified the milestones: Nextcloud 21, Nextcloud 22 Jan 6, 2021
@Lumrenion Lumrenion mentioned this pull request Mar 23, 2021
Open
@J0WI
Copy link
Contributor

J0WI commented Apr 28, 2021

ping @tcitworld

@MorrisJobke MorrisJobke mentioned this pull request May 20, 2021
Merged
@MorrisJobke
Copy link
Member

Let's close this for now. We can reopen it later at any time.

@MorrisJobke MorrisJobke deleted the tweak-default-scopes-accounts branch May 20, 2021 10:12
@tcitworld
Copy link
Member Author

My branch though :(

@juliusknorr juliusknorr restored the tweak-default-scopes-accounts branch May 20, 2021 18:37
@MorrisJobke
Copy link
Member

My branch though :(

It can be restored from the github UI at any time :)

@tcitworld tcitworld mentioned this pull request Mar 18, 2022
Merged
@tcitworld
Copy link
Member Author

Follow-up at #31623

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kesselb kesselb kesselb left review comments

@ChristophWurst ChristophWurst ChristophWurst left review comments

@nickvergessen nickvergessen nickvergessen left review comments

@schiessle schiessle Awaiting requested review from schiessle

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

Assignees
No one assigned
Labels
3. to review Waiting for reviews enhancement feature: users and groups php Pull requests that update Php code
Projects
None yet
Milestone
Nextcloud 22
8 participants
@tcitworld @kesselb @nickvergessen @Dubidubiduu @J0WI @MorrisJobke @ChristophWurst @rullzer