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

Cover Contact CommandHandlers and QueryHandlers by behat tests #16776

Merged

Conversation

@tdavidsonas88
Copy link
Contributor

tdavidsonas88 commented Dec 11, 2019

Questions Answers
Branch? develop
Description? Cover CommandHandlers and QueryHandlers by behat tests #16757
Type? improvement
Category? TE
BC breaks? no
Deprecations? no
Fixed ticket? #14480
How to test? Travis automated tests passes in green

This change is Reviewable

Domain/Contact/CommandHandler/AddContactHandler
Domain/Contact/CommandHandler/EditContactHandler
Domain/Contact/QueryHandler/GetContactForEditingHandler

@tdavidsonas88 tdavidsonas88 requested a review from PrestaShop/prestashop-core-developers as a code owner Dec 11, 2019
@prestonBot

This comment has been minimized.

Copy link
Collaborator

prestonBot commented Dec 11, 2019

Hello @tdavidsonas88!

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

@tdavidsonas88 tdavidsonas88 changed the title Feature/contact=integration tests Cover CommandHandlers and QueryHandlers by behat tests #16757 Dec 12, 2019
@tdavidsonas88 tdavidsonas88 changed the title Cover CommandHandlers and QueryHandlers by behat tests #16757 Cover CommandHandlers and QueryHandlers by behat tests Dec 12, 2019
@matks matks added the migration label Dec 12, 2019
*/
private function mapToEditableContact(int $contactId, array $data): EditableContact
{
$title = $data['title'];

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 13, 2019

Contributor

You don't need this temporary variables you can use them in the constructor right away

This comment has been minimized.

Copy link
@tdavidsonas88

tdavidsonas88 Dec 16, 2019

Author Contributor

@jolelievre @matks @rokaszygmantas In this Behat case, yes, just thought about this style if it is not the Behat test code but production code and the error happens on the live system. Let's imagine the situation, Prestashop shop is running on heavy loaded system and every minute gives profit for the company. Error happens and developer need to fix it ASAP. The case is specific to the LIVE system and in this style of code it won't be obvious which code caused the error - only the long error line will be visible and debugging might be needed.

So not sure what gives more business value to PrestaShop:

  1. Quickly fixing production errors by developers Or
  2. Speed / memory usage of software
  3. My assumptions might be not very good also, because I came from the company with limited knowledge of E-commerce and it was more important to fix errors in production environment ASAP. Successful production environment running time was traded for money and customers value; It might be that I don't understand something about live system maintenance - as this style was forced to me by another more experienced developer.
tests/Integration/Behaviour/behat.yml Show resolved Hide resolved
tests/Integration/Behaviour/behat.yml Show resolved Hide resolved
tests/Unit/PrestaShopBundle/Utils/BoolParserTest.php Outdated Show resolved Hide resolved
@tdavidsonas88

This comment has been minimized.

Copy link
Contributor Author

tdavidsonas88 commented Dec 16, 2019

Thank you for your professional feedback, did my best to make the code better.

@tdavidsonas88 tdavidsonas88 requested review from matks and jolelievre Dec 19, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Dec 20, 2019

@tdavidsonas88 the PR is approved 💚

However I cannot merge it because of a git conflict. Can you please rebase it, then we'll merge it ?

@tdavidsonas88 tdavidsonas88 force-pushed the tdavidsonas88:feature/contact=integration-tests branch from 4027231 to 26475b2 Dec 20, 2019
@tdavidsonas88

This comment has been minimized.

Copy link
Contributor Author

tdavidsonas88 commented Dec 20, 2019

@tdavidsonas88 the PR is approved

However I cannot merge it because of a git conflict. Can you please rebase it, then we'll merge it ?

Thank you @matks for the review. Already rebased!

@tdavidsonas88 tdavidsonas88 requested a review from matks Dec 30, 2019
@matks
matks approved these changes Jan 2, 2020
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 2, 2020

Thank you and congratulations @tdavidsonas88

@matks matks merged commit cd5fbf8 into PrestaShop:develop Jan 2, 2020
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@matks matks added this to the 1.7.7.0 milestone Jan 2, 2020
@matks matks changed the title Cover CommandHandlers and QueryHandlers by behat tests Cover Contact CommandHandlers and QueryHandlers by behat tests Jan 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.