-
Notifications
You must be signed in to change notification settings - Fork 280
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
Conversation
93e492f
to
e73d3b1
Compare
There was a problem hiding this 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.
There was a problem hiding this comment.
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
e73d3b1
to
92ec240
Compare
@@ -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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs a use statement
92ec240
to
1b44b50
Compare
Signed-off-by: Julius Härtl <jus@bitgrid.net>
1b44b50
to
261e6d0
Compare
@@ -21,8 +21,9 @@ | |||
* | |||
*/ | |||
|
|||
namespace OCA\Deck\Activity; | |||
namespace OCA\Deck\Listeners; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ;)
Similar to nextcloud/server#45951 this moves away from the not so lazy registration of comment event handlers.
Checklist