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

chore: Move comments event handler to use proper event dispatcher #6008

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Jun 18, 2024

Similar to nextcloud/server#45951 this moves away from the not so lazy registration of comment event handlers.

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Tests (unit, integration, api and/or acceptance) are included
  • Documentation (manuals or wiki) has been updated or is not required

Copy link
Contributor

@luka-nextcloud luka-nextcloud left a comment

Choose a reason for hiding this comment

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

The failed cypress tests don't seem to be related to the code change.
The failed integration test is related to the code change. Seems like comment event was not triggered.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this file should be moved to tests/unit/Listeners/CommentEventHandlerTest.php

@juliusknorr juliusknorr force-pushed the chore/comments-event-legacy branch from e73d3b1 to 92ec240 Compare June 27, 2024 13:16
@@ -49,7 +49,7 @@ public function setUp(): void {
$this->notificationHelper = $this->createMock(NotificationHelper::class);
$this->cardMapper = $this->createMock(CardMapper::class);
$this->changeHelper = $this->createMock(ChangeHelper::class);
$this->commentEventHandler = new CommentEventHandler(
Copy link
Member

Choose a reason for hiding this comment

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

needs a use statement

@juliusknorr juliusknorr force-pushed the chore/comments-event-legacy branch from 92ec240 to 1b44b50 Compare June 28, 2024 11:30
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr force-pushed the chore/comments-event-legacy branch from 1b44b50 to 261e6d0 Compare June 28, 2024 12:20
@@ -21,8 +21,9 @@
*
*/

namespace OCA\Deck\Activity;
namespace OCA\Deck\Listeners;
Copy link
Member

Choose a reason for hiding this comment

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

😬 okay i guess you can do it that way

Copy link
Member Author

Choose a reason for hiding this comment

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

I also addressed the review comment from Luka with it as the namespace of the actual class was changed, it made sense to adapt the text namespace as well ;)

@juliusknorr juliusknorr merged commit 2f5d39f into main Jun 28, 2024
32 checks passed
@juliusknorr juliusknorr deleted the chore/comments-event-legacy branch June 28, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants