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

[API] Order confirmation email sending #11754

Merged
merged 7 commits into from
Nov 16, 2020
Merged

Conversation

lchrusciel
Copy link
Member

@lchrusciel lchrusciel commented Aug 20, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets
License MIT

This PR introduce mailing refactor, which should be continued in the future. Bigger description and ADR in progress.

@lchrusciel lchrusciel requested a review from a team as a code owner August 20, 2020 12:32
@probot-autolabeler probot-autolabeler bot added Admin AdminBundle related issues and PRs. API APIs related issues and PRs. Shop ShopBundle related issues and PRs. labels Aug 20, 2020
@lchrusciel lchrusciel added the Feature New feature proposals. label Aug 20, 2020
@@ -0,0 +1,28 @@
{% block body %}
{% autoescape false %}
{% set logo = channel.hostname is not null ? 'http://' ~ channel.hostname ~ asset('assets/shop/img/logo.png') : absolute_url(asset('assets/shop/img/logo.png')) %}
Copy link
Member

Choose a reason for hiding this comment

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

Logo is taken from shop namespace, I'm not sure if it is coupling CoreBundle with ShopBundle

Copy link
Member Author

Choose a reason for hiding this comment

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

It is just an asset. Perhaps we should move some image to CoreBundle as well, but it should allow the application to be run without the Shop bundle.

Copy link
Member Author

Choose a reason for hiding this comment

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

It may not render a logo if shop is not enabled.

@@ -25,7 +25,7 @@
"require": {
"php": "^7.3",
"api-platform/core": "^2.5",
"sylius/core-bundle": "^1.7",
"sylius/core-bundle": "dev-master",
Copy link
Member Author

Choose a reason for hiding this comment

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

Let's try composer replace

Copy link
Member Author

Choose a reason for hiding this comment

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

Will be part of separate PR

Copy link
Member Author

Choose a reason for hiding this comment

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

@SirDomin can you try to remove this line?

@lchrusciel lchrusciel force-pushed the api-mailing branch 2 times, most recently from 0f2a51d to 045e812 Compare August 21, 2020 10:44
@lchrusciel
Copy link
Member Author

This PR extracted mailing from Shop to Core. This move allowed us to use the same email code and twig files in both Shop, Admin, and API context. Templates are moved, but in order to provide backward compatibility, files where they were used before, are left and include content from the core. To finish this PR, we should:

  1. Refactor twig includes
  2. Rebase
  3. Fix API Bundle composer.json
  4. Consider refactoring mailing trigger to separate command (and perhaps introduce event bus, for integration layer) <- could be part of separate PR/task
  5. Write ADR about placing emails & sending them from API context

@SirDomin SirDomin force-pushed the api-mailing branch 2 times, most recently from cac900e to 34d7283 Compare November 12, 2020 12:00
/**
* @Given I have proceeded through checkout process in the :localeCode locale with email :email
*/
public function iHaveProceededThroughCheckoutProcessInTheLocaleWithEmail(string $localeCode, string $email)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function iHaveProceededThroughCheckoutProcessInTheLocaleWithEmail(string $localeCode, string $email)
public function iHaveProceededThroughCheckoutProcessInTheLocaleWithEmail(string $localeCode, string $email): void

@GSadee GSadee merged commit 4b1ea10 into Sylius:master Nov 16, 2020
@GSadee
Copy link
Member

GSadee commented Nov 16, 2020

Thank you, Łukasz! 🥇

@lchrusciel lchrusciel deleted the api-mailing branch November 16, 2020 16:29
@SirDomin SirDomin mentioned this pull request Nov 17, 2020
GSadee added a commit that referenced this pull request Nov 19, 2020
This PR was merged into the 1.9-dev branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | master
| Bug fix?        | no
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | not sure
| Related tickets | refactoring the way we handle emails implemented in #11754
| License         | MIT

<!--
 - Bug fixes must be submitted against the 1.7 or 1.8 branch (the lowest possible)
 - Features and deprecations must be submitted against the master branch
 - Make sure that the correct base branch is set

 To be sure you are not breaking any Backward Compatibilities, check the documentation:
 https://docs.sylius.com/en/latest/book/organization/backward-compatibility-promise.html
-->


Commits
-------

c0d8f32 [API] Order confirmation email sending
85b296b [Core] Extract mailer sender from ShopBundle
a73d321 fix composer.json api platform, tests for api mailing added
24ac912 command added
18818a7 mailing is handled with commands
da13fb2 pr fix, behat + spec fixed
b10b272 pr-fix
0def0d8 introduce event bus
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin AdminBundle related issues and PRs. API APIs related issues and PRs. Feature New feature proposals. Shop ShopBundle related issues and PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants