Skip to content

Conversation

@pboguslawski
Copy link
Contributor

This mod allows one to disable talk updates using new global disable_ads boolean setting in config.php.

Related: #3128
Author-Change-Id: IB#1121121

@nickvergessen
Copy link
Member

disable_ads is a very disrespectful config name.

Also if this is only used for the Talk changelog channel, it should be an app config for the app instead (and mostlikely come with an option to toggle it in the admin settings)

@nickvergessen nickvergessen added this to the 💚 Next Major (25) milestone Jul 11, 2022
pboguslawski referenced this pull request in ibpl/spreed Jul 11, 2022
This mod allows one to disable talk updates using new global `disable_ads` boolean setting in `config.php`.

Related: nextcloud#3128
Author-Change-Id: IB#1121121
@nickvergessen nickvergessen added 1. to develop feature: chat 💬 Chat and system messages feature: settings ⚙️ Settings and config related issues labels Jul 11, 2022
@pboguslawski
Copy link
Contributor Author

disable_ads is a very disrespectful config name.

Also if this is only used for the Talk changelog channel, it should be an app config for the app instead (and mostlikely come with an option to toggle it in the admin settings)

We plan to use it for disabling this and other ads we don't want to see in our setup; global disable ads switch is more appropriate for such stuff and may be used in other places also in the future to avoid nc overconfiguration. This PR is because other people find this option valuable:

Feel free to adapt this PR to your requirements if you don't like global switch - we will rather continue to use disable_ads like in this PR.

@pboguslawski pboguslawski removed their assignment Jul 11, 2022
@nickvergessen
Copy link
Member

I discussed it with others and we will accept a dedicated appconfig for this.
A generic "disable_ads" (and again the changelog is not advertisement, it's explaining users what they can do in the newest version?) is not acceptable.

@pboguslawski
Copy link
Contributor Author

I discussed it with others and we will accept a dedicated appconfig for this. A generic "disable_ads" (and again the changelog is not advertisement, it's explaining users what they can do in the newest version?) is not acceptable.

spreed.changelog switch for this function LGTM (PR updated)

@pboguslawski
Copy link
Contributor Author

Let me know if DCO and rebasing to master is required.

@nickvergessen
Copy link
Member

Let me know if DCO and rebasing to master is required.

That would be awesome

This mod introduces spreed.changelog app setting that allows one to disable talk updates.

Related: nextcloud#3128
Related: nextcloud#7569
Author-Change-Id: IB#1121121
Signed-off-by: Pawel Boguslawski <[email protected]>
@pboguslawski
Copy link
Contributor Author

pboguslawski commented Jul 15, 2022

Just force-pushed master compatible version with DCO (2e37b6a). Please change this PR target from stable24 to master.

@nickvergessen nickvergessen changed the base branch from stable24 to master July 15, 2022 07:25
@nickvergessen
Copy link
Member

Seems CI chocked on it. So repushed inside the org at #7613

@nickvergessen nickvergessen merged commit 446dfe3 into nextcloud:master Jul 20, 2022
backportbot-nextcloud bot pushed a commit that referenced this pull request Nov 18, 2022
This mod introduces spreed.changelog app setting that allows one to disable talk updates.

Related: #3128
Related: #7569
Author-Change-Id: IB#1121121
Signed-off-by: Pawel Boguslawski <[email protected]>
backportbot-nextcloud bot pushed a commit that referenced this pull request Nov 18, 2022
This mod introduces spreed.changelog app setting that allows one to disable talk updates.

Related: #3128
Related: #7569
Author-Change-Id: IB#1121121
Signed-off-by: Pawel Boguslawski <[email protected]>
@pboguslawski pboguslawski deleted the master-IB#1121121 branch January 23, 2023 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1. to develop feature: chat 💬 Chat and system messages feature: settings ⚙️ Settings and config related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants