Skip to content

Conversation

@Jeroenll
Copy link
Member

@Jeroenll Jeroenll commented Dec 6, 2017

Description

We are using recurring payments in combination with pre-order products. A small amount of the final price is payed and with the shipment, the remaining amount. In order to do this we always need a customer. We therefore automatically convert guest to shadow customers. This isn't an account the customer can use but we need it for the recurring payment.

In Magento v2.2 a plugin of QuoteManagement::submit() is introduced, AccessChangeQuoteControl which now prevents guests from placing orders since $quote->getCustomerId() isn't null anymore.

Moved logic to separate service so we can create a plugin.

Fixed Issues (if relevant)

  1. None that I could find.

Manual testing scenarios

  1. Convert guests to customers by making a plugin of QuoteManagement::submit()
  2. In frontend as guest, add an item to your quote in proceed to checkout.
  3. Complete checkout, make sure plugin to convert guest to shadow customer is triggered.
  4. API call will return following exception: Invalid state change requested

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 unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

2.3 Port: #13776

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no real use for making any additional methods in a plugin public (apart from the plugin itself):

  • Technical Guidelines do not recommend injecting a plugin into any class, it is not supposed to work that way.
  • It is not a really good idea to create a plugin for another plugin.

In this case I suggest to introduce a separate service which would provide this logic, thus, it will be available for any third party code and may be used in this plugin at the same time.

@Jeroenll
Copy link
Member Author

Jeroenll commented Dec 6, 2017

@ishakhsuvarov For my understanding, why is this feature implemented using a plugin? Is this something you solely want to execute in an API environment?

If I create a new Interface/Model which holds only the isAllowed()method in the Quote module, will that be sufficient?

@ishakhsuvarov
Copy link
Contributor

@JeroenVanLeusden Unfortunately I do not have information regarding this plugin and its origin at the moment. I assume this relates to the support of B2B extension.

New model with this method would be sufficient, form my point of view, as it would allow you to customize it with a plugin for your use case and also it would not violate requirements.

@Jeroenll Jeroenll changed the title Make isAllowed public in AccessChangeQuoteControl Move isAllowed method from AccessChangeQuoteControl to separate service Dec 6, 2017
@Jeroenll
Copy link
Member Author

Jeroenll commented Dec 6, 2017

@ishakhsuvarov Logic is moved to separate service. Can you share your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please provide description for the interface in the docblock

Copy link
Contributor

Choose a reason for hiding this comment

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

Please provide description for the class or inherit it from the interface

@ishakhsuvarov
Copy link
Contributor

@JeroenVanLeusden Updated version looks good to me.
Please check the Travis CI test results, I assume unit tests have failed because plugin we covered with one. So it has to be adjusted.
Let us know if you need any help with that.

Thank you!

@ishakhsuvarov
Copy link
Contributor

Also, Functional Tests job on Travis is failing on the Checkout, I've restarted it to make sure, but it is still red, so I think it may have something to do with the changes introduced.

@Jeroenll
Copy link
Member Author

Jeroenll commented Dec 7, 2017

@ishakhsuvarov Will look at the tests somewhere this week and let you know.

@ishakhsuvarov
Copy link
Contributor

@JeroenVanLeusden Thanks

@Jeroenll
Copy link
Member Author

Jeroenll commented Dec 7, 2017

@ishakhsuvarov Unit tests are now passing. Are you able to check the functional tests? I haven't had time yet to figure out how they work.

@ishakhsuvarov
Copy link
Contributor

@JeroenVanLeusden Sure. I will check it and let you know what is wrong

@okorshenko okorshenko modified the milestones: December 2017, January 2018 Jan 8, 2018
@Jeroenll
Copy link
Member Author

Jeroenll commented Jan 9, 2018

@ishakhsuvarov Any update on this PR? If you need something from me please let me know.

@Jeroenll
Copy link
Member Author

@ishakhsuvarov I've updated the code and all tests are now green.

@magento-engcom-team magento-engcom-team added Partner: Reach Digital Pull Request is created by partner Reach Digital partners-contribution Pull Request is created by Magento Partner labels Mar 29, 2018
@okorshenko okorshenko modified the milestones: March 2018, April 2018 Apr 16, 2018
@magento-engcom-team magento-engcom-team merged commit 01c54c8 into magento:2.2-develop Apr 27, 2018
@magento-engcom-team
Copy link
Contributor

Hi @JeroenVanLeusden. Thank you for your contribution.
Changes from your Pull Request will be available with the upcoming 2.2.5 release.

@Jeroenll Jeroenll deleted the patch-2 branch April 27, 2018 15:26
@paales paales added Partner: Reach Digital Pull Request is created by partner Reach Digital and removed Partner: Reach Digital Pull Request is created by partner Reach Digital labels May 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Award: complex Partner: Reach Digital Pull Request is created by partner Reach Digital partners-contribution Pull Request is created by Magento Partner Progress: accept Release Line: 2.2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants