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

Implements CQRS on Notifications #15965

Merged
merged 1 commit into from Oct 31, 2019

Conversation

@atomiix
Copy link
Contributor

atomiix commented Oct 15, 2019

Questions Answers
Branch? develop
Description? Use CQRS to get and update notifications
Type? refacto
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #12870
How to test? Add a new customer, see if you get a notification then open it and see if it disappears. This PR changes no behavior, it's a refactoring, notifications should work as before. Examples of notifications: successful deletion of a product, or successful update of an element.

This change is Reviewable

@atomiix atomiix requested a review from PrestaShop/prestashop-core-developers as a code owner Oct 15, 2019
@prestonBot

This comment has been minimized.

Copy link
Collaborator

prestonBot commented Oct 15, 2019

Hello @atomiix!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

*/
public function handle(GetNotificationLastElements $query)
{
return (new \Notification())->getLastElements();

This comment has been minimized.

Copy link
@sarjon

sarjon Oct 15, 2019

Contributor

I'm afraid you should not blindly return whatever legacy gives you. 😕

This comment has been minimized.

Copy link
@eternoendless

eternoendless Oct 15, 2019

Member

Queries must return a Data Transfer Object.

See also Query and QueryHandler principles:

  1. A QueryHandler SHOULD return data objects that make sense to the domain, and SHOULD NOT leak internal objects.
*/
public function handle(GetNotificationLastElements $query)
{
return (new \Notification())->getLastElements();

This comment has been minimized.

Copy link
@eternoendless

eternoendless Oct 15, 2019

Member

Queries must return a Data Transfer Object.

See also Query and QueryHandler principles:

  1. A QueryHandler SHOULD return data objects that make sense to the domain, and SHOULD NOT leak internal objects.
namespace PrestaShop\PrestaShop\Core\Domain\Notification\Command;
/**
* ACK employee notification last element

This comment has been minimized.

Copy link
@eternoendless

eternoendless Oct 15, 2019

Member
Suggested change
* ACK employee notification last element
* Updates the last notification element from a given type seen by the employee
@atomiix atomiix force-pushed the atomiix:notification-CQRS branch 2 times, most recently from 9a441ad to a722ab5 Oct 16, 2019
@atomiix atomiix force-pushed the atomiix:notification-CQRS branch from a722ab5 to f1c7a0d Oct 18, 2019
@atomiix atomiix force-pushed the atomiix:notification-CQRS branch from f1c7a0d to a87cbf6 Oct 21, 2019
@matks
matks approved these changes Oct 29, 2019
@matks matks dismissed eternoendless’s stale review Oct 29, 2019

Requested changes have been applied

@matks matks added this to the 1.7.7.0 milestone Oct 29, 2019
@sarahdib sarahdib self-assigned this Oct 31, 2019
@sarahdib sarahdib added QA ✔️ and removed waiting for QA labels Oct 31, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 31, 2019

Thank you @atomiix

@matks matks merged commit 6e60f22 into PrestaShop:develop Oct 31, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@atomiix atomiix deleted the atomiix:notification-CQRS branch Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.