Skip to content

Conversation

anzin
Copy link
Contributor

@anzin anzin commented Feb 17, 2021

Description (*)
I've implemented:

  1. Generator for the "Delete Entity By Id Command."
  2. Added test for "Delete Entity By Id Command'.

Screenshot 2021-02-17 at 15 40 33

Screenshot 2021-02-17 at 15 40 19

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with integration/functional tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@@ -238,6 +238,7 @@
<internalFileTemplate name="Magento PHP Form Generic Button Block Class"/>
<internalFileTemplate name="Magento Entity New Action Controller Class"/>
<internalFileTemplate name="Magento New Entity Layout XML"/>
<internalFileTemplate name="Magento Delete Entity By Id Command Model"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why we need the word Model here. It's just a command, isn’t it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eduard13 @coderimus what do you think guys?

Copy link
Contributor

@eduard13 eduard13 Feb 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 agree, Model word is extra. But for me this "Command" is a bit confusing. For me it looks like a Service Contract, that handles the Entity removal, which should have an API interface and a Service that implements it.
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VitaliyBoyko agree that the Model is an extra word.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks 🙏

Copy link
Contributor

@VitaliyBoyko VitaliyBoyko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the Model from the command template

@anzin
Copy link
Contributor Author

anzin commented Feb 18, 2021

Hello, @VitaliyBoyko I've fixed the code by recommendation.

Thank you

Copy link
Contributor

@VitaliyBoyko VitaliyBoyko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants