Skip to content

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

Merged
artonge merged 2 commits into
masterfrom
artonge/fix/make_error_a_info
Apr 1, 2025
Merged

fix(files_reminders): Lower disabled notifications app error to info#51760
artonge merged 2 commits into
masterfrom
artonge/fix/make_error_a_info

Conversation

@artonge

@artongeartonge commented Mar 27, 2025•
edited
Loading

Copy link
Copy Markdown
Collaborator

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 ownerMarch 27, 2025 15:29
@artonge
artonge requested review from Pytal, icewind1991, nfebe and sorbaugh and removed request for a teamMarch 27, 2025 15:29
@artongeartonge self-assigned this Mar 27, 2025
@artongeartonge added 3. to review Waiting for reviews php Pull requests that update Php code tests Related to tests labels Mar 27, 2025
@artongeartonge added this to the Nextcloud 32 milestone Mar 27, 2025
@artonge

artonge commented Mar 27, 2025

Copy link
Copy Markdown
CollaboratorAuthor

/backport to stable31

@artonge

artonge commented Mar 27, 2025

Copy link
Copy Markdown
CollaboratorAuthor

/backport to stable30

@artonge

artonge commented Mar 27, 2025

Copy link
Copy Markdown
CollaboratorAuthor

/backport to stable29

@artonge

artonge commented Mar 27, 2025

Copy link
Copy Markdown
CollaboratorAuthor

/backport to stable28

Pytal
Pytal approved these changes Mar 27, 2025
View reviewed changes
Comment threadapps/files_reminders/lib/Listener/LoadAdditionalScriptsListener.php Outdated
@artonge
artongeforce-pushed the artonge/fix/make_error_a_info branch from d5c79a5 to 9e13e38CompareMarch 31, 2025 17:38
@artonge
artonge requested review from come-nc and susnuxApril 1, 2025 10:10
come-nc
come-nc reviewed Apr 1, 2025
View reviewed changes

@come-nccome-nc left a comment

Copy link
Copy Markdown
Contributor

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

come-nc commented Apr 1, 2025

Copy link
Copy Markdown
Contributor

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

artonge commented Apr 1, 2025•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

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
artongeforce-pushed the artonge/fix/make_error_a_info branch from 9e13e38 to 04d4945CompareApril 1, 2025 12:34
@come-nc

come-nc commented Apr 1, 2025

Copy link
Copy Markdown
Contributor

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

artonge commented Apr 1, 2025

Copy link
Copy Markdown
CollaboratorAuthor

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
artongeforce-pushed the artonge/fix/make_error_a_info branch from 224720f to 67d339cCompareApril 1, 2025 13:18
@artonge
fix(files_reminder): Replace error log by setup check
776c103
Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge
artongeforce-pushed the artonge/fix/make_error_a_info branch from 67d339c to 776c103CompareApril 1, 2025 13:20
@artonge
artonge requested a review from come-ncApril 1, 2025 15:03
come-nc
come-nc approved these changes Apr 1, 2025
View reviewed changes

@come-nccome-nc left a comment

Copy link
Copy Markdown
Contributor

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 masterApr 1, 2025
@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
@susnuxsusnux mentioned this pull request Apr 4, 2025
Merged
4 tasks
susnux
susnux reviewed Apr 4, 2025
View reviewed changes
Comment threadapps/files_reminders/lib/SetupChecks/NeedNotificationsApp.php
}

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

@susnuxsusnuxApr 4, 2025

Copy link
Copy Markdown
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
Comment threadapps/files_reminders/lib/AppInfo/Application.php

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

$context->registerSetupCheck(NeedNotificationsApp::class);

@susnuxsusnuxApr 4, 2025

Copy link
Copy Markdown
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-botnextcloud-bot mentioned this pull request Aug 19, 2025
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@susnuxsusnuxsusnux left review comments
@icewind1991icewind1991icewind1991 approved these changes
@PytalPytalPytal approved these changes
@come-nccome-nccome-nc approved these changes
@nfebenfebeAwaiting requested review from nfebenfebe is a code owner automatically assigned from nextcloud/server-backend
@sorbaughsorbaughAwaiting requested review from sorbaughsorbaugh is a code owner automatically assigned from nextcloud/server-backend

Assignees

@artongeartonge

Labels

3. to reviewWaiting for reviewsphpPull requests that update Php codetestsRelated to tests

Projects

None yet

Milestone

Nextcloud 32

Development

Successfully merging this pull request may close these issues.

6 participants

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