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

Command to create Notification #389

Merged
merged 1 commit into from Jul 21, 2019

Conversation

@aquibbaig
Copy link
Collaborator

aquibbaig commented Jul 16, 2019

Initial PR which is aimed at fixing #388

@aquibbaig aquibbaig added the gsoc label Jul 16, 2019
@aquibbaig aquibbaig force-pushed the aquibbaig:notif-command branch from f35a2d9 to f09dc90 Jul 16, 2019
protected function configure()
{
$this->setDescription('Adds a Notification to the database')
->setHelp('This command allows you to send a web notification to Rabbitmq.');

This comment has been minimized.

Copy link
@imphil

imphil Jul 17, 2019

Contributor

This tool is neither limited to rabbitmq nor to web notifications. How about "Send a notification to a user" as description?

->setHelp('This command allows you to send a web notification to Rabbitmq.');
$this->addArgument('subject', InputArgument::REQUIRED, 'Add a Notification subject...');
$this->addArgument('message', InputArgument::REQUIRED, 'Add a Notification message...');
$this->addArgument('type', InputArgument::REQUIRED, 'Specify the Notification type...');

This comment has been minimized.

Copy link
@imphil

imphil Jul 17, 2019

Contributor
  • Please remove all those "...".
  • "Notification" isn't capitalized.
  • You need to add a "username" argument.
{
$users = $this->userManager->findUsers();
foreach ($users as $user) {

This comment has been minimized.

Copy link
@imphil

imphil Jul 17, 2019

Contributor

Don't send a notification to all users, add a "username" argument, use the UserManager to actually get the User for a given username, and send the notification to this one only.

{
parent::__construct();
$this->producer = $notificationProducer;
$this->notification = $notification;

This comment has been minimized.

Copy link
@imphil

imphil Jul 17, 2019

Contributor

Why is notification an object member variable? It should be a local variable in the execute() method.

}
/**
* Publish notifications for test users

This comment has been minimized.

Copy link
@imphil

imphil Jul 17, 2019

Contributor

Adjust this comment as well when adding the username input argument.

use Symfony\Component\Console\Input\InputArgument;
/**
* Creates a Notification and publishes it to RabbitMQ

This comment has been minimized.

Copy link
@imphil

imphil Jul 17, 2019

Contributor

Create a notification for a given user.

/**
* @var string
*/
protected static $defaultName = 'create:notification';

This comment has been minimized.

Copy link
@imphil

imphil Jul 17, 2019

Contributor

Should be librecores:send-notification

* @param Notification $notification
* @param UserManagerInterface $userManager
*/
public function __construct(ProducerInterface $notificationProducer, Notification $notification, UserManagerInterface $userManager)

This comment has been minimized.

Copy link
@imphil

imphil Jul 17, 2019

Contributor

Why do you inject the Notification data class here?

@aquibbaig aquibbaig force-pushed the aquibbaig:notif-command branch from f09dc90 to 38ea100 Jul 17, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jul 17, 2019

@imphil Yep. Thanks for the review. I just made some changes

@aquibbaig aquibbaig force-pushed the aquibbaig:notif-command branch from 38ea100 to cfe4084 Jul 17, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jul 17, 2019

@imphil user -> username

@aquibbaig aquibbaig force-pushed the aquibbaig:notif-command branch from cfe4084 to 6465b61 Jul 17, 2019
@imphil imphil merged commit 35eb5a3 into librecores:master Jul 21, 2019
@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Jul 21, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.