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

[GSoC 2019] Configure Web Notification Consumer #384

Merged
merged 2 commits into from Jul 7, 2019

Conversation

@aquibbaig
Copy link
Collaborator

aquibbaig commented Jul 5, 2019

Fixes #374

@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch from c620c8c to f99b96e Jul 7, 2019
Copy link
Contributor

imphil left a comment

Thanks @aquibbaig for this update. I have a couple small comments in the code, nothing major though.

* @ORM\Column(name="recipient", nullable=false)
*
*/
protected $recipient;

This comment has been minimized.

Copy link
@imphil

imphil Jul 7, 2019

Contributor

You don't need that, the recipient is in the notifiable annotation in mgilet-notificationbundle.

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Jul 7, 2019

Author Collaborator

How can we fetch that while persisting the Notification for a specific user ?

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Jul 7, 2019

Author Collaborator

Cause not having a recipient while persisting crashes our consumer

This comment has been minimized.

Copy link
@imphil

imphil Jul 7, 2019

Contributor

You added an annotation to make the User entity a NotifableEntity; that's where the recipient of a message is stored in this case.

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Jul 7, 2019

Author Collaborator

That is correct, what was I even thinking!

public function __construct(
LoggerInterface $logger,
NotificationManager $notificationManager,
UserManagerInterface $userManager

This comment has been minimized.

Copy link
@imphil

imphil Jul 7, 2019

Contributor

this last argument is not documented in the phpdoc.

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Jul 7, 2019

Author Collaborator

userManager is not required in this Consumer, yet. I am removing it completely

$notification->setDate($this->notification->getCreatedAt());
$notification->setRecipient($this->notification->getRecipient());
if ($this->shouldHandle()) {

This comment has been minimized.

Copy link
@imphil

imphil Jul 7, 2019

Contributor

Why do you call that method here? It has already been called once we enter handle().

$notification->setType($this->notification->getType());
$notification->setSubject($this->notification->getSubject());
$notification->setDate($this->notification->getCreatedAt());
$notification->setRecipient($this->notification->getRecipient());

This comment has been minimized.

Copy link
@imphil

imphil Jul 7, 2019

Contributor

The recipient is already given below in the notifiable part (the first parameter of addNotification()), so you don't need that here.

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Jul 7, 2019

Author Collaborator

Agreed, But we are persisting the $notification variable which will not have a Recipient if we omit this line probably.

This comment has been minimized.

Copy link
@imphil

imphil Jul 7, 2019

Contributor

The recipient is the "notifiable entity", you don't need a recipient field in addition to that.

if ($this->shouldHandle()) {
// Persist a Notification
$this->notificationManager->addNotification(array($this->notification->getRecipient()), $notification, true);

This comment has been minimized.

Copy link
@imphil

imphil Jul 7, 2019

Contributor

Please keep this line around 100 characters.

The newer way of writing array() is just using []

@aquibbaig aquibbaig changed the title [GSoC 2019] Create notification entity for web notifications [GSoC 2019] Configure Web Notification Consumer Jul 7, 2019
@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch from 50e35a7 to ad7a9db Jul 7, 2019
@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch from ad7a9db to bde2eb4 Jul 7, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jul 7, 2019

Moving on, @imphil Does the handle() method need more info on the logger and a try->catch ?

@imphil
imphil approved these changes Jul 7, 2019
Copy link
Contributor

imphil left a comment

Looks good, thanks!

@imphil imphil merged commit 0ea1496 into librecores:master Jul 7, 2019
@imphil imphil added this to In progress in GSoC 2019 via automation Jul 7, 2019
@aquibbaig aquibbaig moved this from In progress to Reviewer approved in GSoC 2019 Jul 15, 2019
@aquibbaig aquibbaig moved this from Reviewer approved to Done in GSoC 2019 Jul 15, 2019
@aquibbaig aquibbaig added the gsoc label Aug 17, 2019
@aquibbaig aquibbaig referenced this pull request Sep 9, 2019
6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
GSoC 2019
  
Done
2 participants
You can’t perform that action at this time.