-
Notifications
You must be signed in to change notification settings - Fork 17
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
Implement report notifications #783
Merged
Merged
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
b556e4b
Implement report notifications
BentiGorlich 0457c5b
Merge branch 'main' into new/report-notifications
BentiGorlich 7e1f228
Add the new notifications to the API
BentiGorlich 8fb4ef6
Merge branch 'main' into new/report-notifications
BentiGorlich 22e72fa
Merge branch 'main' into new/report-notifications
BentiGorlich bc5e8b1
Merge branch 'main' into new/report-notifications
BentiGorlich bb160ae
Merge branch 'main' into new/report-notifications
BentiGorlich 98c61a5
Merge branch 'main' into new/report-notifications
BentiGorlich 6d4a55b
Merge branch 'main' into new/report-notifications
melroy89 097d455
Merge branch 'main' into new/report-notifications
BentiGorlich d343d78
Merge branch 'main' into new/report-notifications
BentiGorlich b03874a
Adjust recipients and API objects
BentiGorlich b5984f6
Merge branch 'main' into new/report-notifications
BentiGorlich 986ebf8
Merge branch 'main' into new/report-notifications
BentiGorlich d660daa
Merge branch 'main' into new/report-notifications
1d9a85e
Merge branch 'main' into new/report-notifications
BentiGorlich a794a4c
Merge branch 'main' into new/report-notifications
dbc8528
Merge branch 'main' into new/report-notifications
BentiGorlich 8900450
Only show "open report" for people with permission to view it
BentiGorlich 5ec8723
Merge branch 'main' into new/report-notifications
BentiGorlich File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace DoctrineMigrations; | ||
|
||
use Doctrine\DBAL\Schema\Schema; | ||
use Doctrine\Migrations\AbstractMigration; | ||
|
||
final class Version20240515122858 extends AbstractMigration | ||
{ | ||
public function getDescription(): string | ||
{ | ||
return 'Add the report field for notifications'; | ||
} | ||
|
||
public function up(Schema $schema): void | ||
{ | ||
$this->addSql('ALTER TABLE notification ADD report_id INT DEFAULT NULL'); | ||
$this->addSql('ALTER TABLE notification ADD CONSTRAINT FK_BF5476CA4BD2A4C0 FOREIGN KEY (report_id) REFERENCES report (id) ON DELETE CASCADE NOT DEFERRABLE INITIALLY IMMEDIATE'); | ||
$this->addSql('CREATE INDEX IDX_BF5476CA4BD2A4C0 ON notification (report_id)'); | ||
} | ||
|
||
public function down(Schema $schema): void | ||
{ | ||
$this->addSql('ALTER TABLE notification DROP CONSTRAINT FK_BF5476CA4BD2A4C0'); | ||
$this->addSql('DROP INDEX IDX_BF5476CA4BD2A4C0'); | ||
$this->addSql('ALTER TABLE notification DROP report_id'); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace App\Entity; | ||
|
||
use Doctrine\ORM\Mapping\Entity; | ||
use Doctrine\ORM\Mapping\JoinColumn; | ||
use Doctrine\ORM\Mapping\ManyToOne; | ||
|
||
#[Entity] | ||
class ReportApprovedNotification extends Notification | ||
{ | ||
#[ManyToOne(targetEntity: Report::class)] | ||
#[JoinColumn(nullable: true, onDelete: 'CASCADE')] | ||
public ?Report $report = null; | ||
|
||
public function __construct(User $receiver, Report $report) | ||
{ | ||
parent::__construct($receiver); | ||
|
||
$this->report = $report; | ||
} | ||
|
||
public function getType(): string | ||
{ | ||
return 'report_approved_notification'; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace App\Entity; | ||
|
||
use Doctrine\ORM\Mapping\Entity; | ||
use Doctrine\ORM\Mapping\JoinColumn; | ||
use Doctrine\ORM\Mapping\ManyToOne; | ||
|
||
#[Entity] | ||
class ReportCreatedNotification extends Notification | ||
{ | ||
#[ManyToOne(targetEntity: Report::class)] | ||
#[JoinColumn(nullable: true, onDelete: 'CASCADE')] | ||
public ?Report $report = null; | ||
|
||
public function __construct(User $receiver, Report $report) | ||
{ | ||
parent::__construct($receiver); | ||
|
||
$this->report = $report; | ||
} | ||
|
||
public function getType(): string | ||
{ | ||
return 'report_created_notification'; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace App\Entity; | ||
|
||
use Doctrine\ORM\Mapping\Entity; | ||
use Doctrine\ORM\Mapping\JoinColumn; | ||
use Doctrine\ORM\Mapping\ManyToOne; | ||
|
||
#[Entity] | ||
class ReportRejectedNotification extends Notification | ||
{ | ||
#[ManyToOne(targetEntity: Report::class)] | ||
#[JoinColumn(nullable: true, onDelete: 'CASCADE')] | ||
public ?Report $report = null; | ||
|
||
public function __construct(User $receiver, Report $report) | ||
{ | ||
parent::__construct($receiver); | ||
|
||
$this->report = $report; | ||
} | ||
|
||
public function getType(): string | ||
{ | ||
return 'report_rejected_notification'; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace App\EventSubscriber; | ||
|
||
use App\Event\Report\ReportApprovedEvent; | ||
use App\Service\Notification\ReportNotificationManager; | ||
use Symfony\Component\EventDispatcher\EventSubscriberInterface; | ||
|
||
class ReportApprovedSubscriber implements EventSubscriberInterface | ||
{ | ||
public function __construct( | ||
private readonly ReportNotificationManager $notificationManager, | ||
) { | ||
} | ||
|
||
public function onReportApproved(ReportApprovedEvent $reportedEvent): void | ||
{ | ||
$this->notificationManager->sendReportApprovedNotification($reportedEvent->report); | ||
} | ||
|
||
public static function getSubscribedEvents(): array | ||
{ | ||
return [ | ||
ReportApprovedEvent::class => 'onReportApproved', | ||
]; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace App\EventSubscriber; | ||
|
||
use App\Event\Report\ReportRejectedEvent; | ||
use App\Service\Notification\ReportNotificationManager; | ||
use Symfony\Component\EventDispatcher\EventSubscriberInterface; | ||
|
||
class ReportRejectedSubscriber implements EventSubscriberInterface | ||
{ | ||
public function __construct( | ||
private readonly ReportNotificationManager $notificationManager, | ||
) { | ||
} | ||
|
||
public function onReportRejected(ReportRejectedEvent $reportedEvent): void | ||
{ | ||
$this->notificationManager->sendReportRejectedNotification($reportedEvent->report); | ||
} | ||
|
||
public static function getSubscribedEvents(): array | ||
{ | ||
return [ | ||
ReportRejectedEvent::class => 'onReportRejected', | ||
]; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 notification structure has the actual subject of the notification detailed - using the report id instead breaks this convention, and would also make it impossible for regular users to have any information about the report contained in this notification displayed by the app using the API, since retrieving reports is a moderator and admin only API endpoint.
This solution would not be sufficient if we wanted to display the reason a user's post was deleted in the notification using the API, for example
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.
What I would recommend would be to follow the convention of the other notifications and embed a subset of the report in the notification, at least the
reason
,magazine
,type
, andsubjectId
of the reportsubjectId
would be the post/comment/entry that was reported and removedThere 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'll check that
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.
Ok, here are my thoughts:
ReportCreatedNotification
: only admins and mods can get this, so imo its fine to only send the id of the report, but of course a dto of the report could be added for this.ReportRejectedNotification
: a user should be able to see their own reports, but if not maybe add the subject of the report here, as there is no reason supplied for rejecting a report.ReportApprovedNotification
: since this is going to the author of the reported content, the subject that was reported should be enough.Would you be fine with that?
The reason I do not just want to add a report dto to all of them is simple: you should never be informed who is reporting your content for which reason, unless the report is accepted, but even then the reason the reporting user supplied is not necessarily the one the mod removed your content for. Since we do not have a "moderators_reason" field (or something similar) it would not make much sense to just relay that information to the reported user