Skip to content

fix(files_reminders): Lower disabled notifications app error to info #51760

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
Merged
merged 2 commits into from
Apr 1, 2025
Merged

fix(files_reminders): Lower disabled notifications app error to info #51760

merged 2 commits into from
Apr 1, 2025

Conversation

@artonge
Copy link
Contributor

@artonge artonge commented Mar 27, 2025 •
edited
Loading

This is spamming the cypress server logs.

Original: #49232

Or should we write a setup check instead?

@artonge artonge requested a review from a team as a code owner March 27, 2025 15:29
@artonge artonge requested review from Pytal, icewind1991, nfebe and sorbaugh and removed request for a team March 27, 2025 15:29
@artonge artonge self-assigned this Mar 27, 2025
@artonge artonge added 3. to review Waiting for reviews php Pull requests that update Php code tests Related to tests labels Mar 27, 2025
@artonge artonge added this to the Nextcloud 32 milestone Mar 27, 2025
@artonge
Copy link
Contributor Author

artonge commented Mar 27, 2025

/backport to stable31

@artonge
Copy link
Contributor Author

artonge commented Mar 27, 2025

/backport to stable30

@artonge
Copy link
Contributor Author

artonge commented Mar 27, 2025

/backport to stable29

@artonge
Copy link
Contributor Author

artonge commented Mar 27, 2025

/backport to stable28

Pytal
Pytal approved these changes Mar 27, 2025
View reviewed changes
@artonge artonge force-pushed the artonge/fix/make_error_a_info branch from d5c79a5 to 9e13e38 Compare March 31, 2025 17:38
@artonge artonge requested review from come-nc and susnux April 1, 2025 10:10
come-nc
come-nc reviewed Apr 1, 2025
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

But does files_reminders app make sense if notifications is disabled?

@come-nc
Copy link
Contributor

come-nc commented Apr 1, 2025

This would re-open #49231 as by default info messages are not shown.

Why does cypress server enabled files_reminder without enabling notifications first?

@artonge
Copy link
Contributor Author

artonge commented Apr 1, 2025 •
edited
Loading

Why does cypress server enabled files_reminder without enabling notifications first?

notifications is bundled, files_reminder is in server.

But does files_reminders app make sense if notifications is disabled?

I am not sure, but probably not, hence the request for an error.

@artonge
fix(files_reminders): Lower disabled notifications app error to info
04d4945 
Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge artonge force-pushed the artonge/fix/make_error_a_info branch from 9e13e38 to 04d4945 Compare April 1, 2025 12:34
@come-nc
Copy link
Contributor

come-nc commented Apr 1, 2025

Why does cypress server enabled files_reminder without enabling notifications first?

notifications is bundled, files_reminder is in server.

Then either clone notifications or disable files_reminder?

@artonge
Copy link
Contributor Author

artonge commented Apr 1, 2025

Then either clone notifications or disable files_reminder?

I think that server should not spam the logs when running without external applications.

I removed the log and added a setup check, it makes more sense.

@artonge artonge force-pushed the artonge/fix/make_error_a_info branch from 224720f to 67d339c Compare April 1, 2025 13:18
@artonge
fix(files_reminder): Replace error log by setup check
776c103 
Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge artonge force-pushed the artonge/fix/make_error_a_info branch from 67d339c to 776c103 Compare April 1, 2025 13:20
@artonge artonge requested a review from come-nc April 1, 2025 15:03
come-nc
come-nc approved these changes Apr 1, 2025
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Ok, why not.

Quite frankly I would be for adding a basic dependency system between apps. Nothing fancy, only preventing enabling if dep is not there, and prevent disabling if app is depended upon.

@artonge artonge merged commit 29a4962 into master Apr 1, 2025
210 of 221 checks passed
@artonge artonge deleted the artonge/fix/make_error_a_info branch April 1, 2025 15:08
This was referenced Apr 1, 2025
Closed
Merged
Merged
Merged
@susnux susnux mentioned this pull request Apr 4, 2025
Merged
4 tasks
susnux
susnux reviewed Apr 4, 2025
View reviewed changes
apps/files_reminders/lib/SetupChecks/NeedNotificationsApp.php
}

public function getName(): string {
return $this->l10n->t('Files reminder');
Copy link
Contributor

Choose a reason for hiding this comment

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

l10n not injected in constructor

susnux
susnux reviewed Apr 4, 2025
View reviewed changes
apps/files_reminders/lib/AppInfo/Application.php

$context->registerEventListener(LoadAdditionalScriptsEvent::class, LoadAdditionalScriptsListener::class);

$context->registerSetupCheck(NeedNotificationsApp::class);
Copy link
Contributor

Choose a reason for hiding this comment

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

class not imported

@nextcloud-bot nextcloud-bot mentioned this pull request Aug 19, 2025
Merged
@skjnldsv skjnldsv modified the milestones: Nextcloud 32, Nextcloud 33 Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@susnux susnux susnux left review comments

@icewind1991 icewind1991 icewind1991 approved these changes

@Pytal Pytal Pytal approved these changes

@come-nc come-nc come-nc approved these changes

@nfebe nfebe Awaiting requested review from nfebe nfebe is a code owner automatically assigned from nextcloud/server-backend

@sorbaugh sorbaugh Awaiting requested review from sorbaugh sorbaugh is a code owner automatically assigned from nextcloud/server-backend

Assignees

@artonge artonge

Labels

3. to review Waiting for reviews php Pull requests that update Php code tests Related to tests

Projects

None yet

Milestone

Nextcloud 32

Development

Successfully merging this pull request may close these issues.

7 participants

@artonge @come-nc @icewind1991 @susnux @Pytal @skjnldsv