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

Fix order customer comment and product out of stock #7

Merged
merged 4 commits into from
Jul 29, 2021

Conversation

jolelievre
Copy link
Contributor

@jolelievre jolelievre commented Oct 5, 2020

Questions Answers
Description? Some templates had been fixed directly in the core but not in the sources from this repo
Also update composer dependencies so that PHP 7.3 can be used to generate the mail theme
Type? improvement
BC breaks? yes
Deprecations? no
Fixed ticket? Fixes PrestaShop/PrestaShop#22028
How to test? You can regenerate the modern theme with the appropriate wordings (already tested by @jolelievre )

@eternoendless
Copy link
Member

@jolelievre Would you mind pushing your last commit in a separate PR?

Copy link
Contributor

@PierreRambaud PierreRambaud left a comment

Choose a reason for hiding this comment

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

Could you please separate commits?

@jolelievre
Copy link
Contributor Author

You mean update the dependency in a separate PR?

@PierreRambaud
Copy link
Contributor

You mean update the dependency in a separate PR?

Yep :)

@jolelievre
Copy link
Contributor Author

Ok @PierreRambaud I split the update into this PR #10

And @Julievrz I made the modification to use the existing wording, so I added the Product line so that the merchant can still identify which product is concerned To answer your question two templates exist one is used by the core and another one by the module ps_emailalert I guess this one has been integrated in the core directly later 🤷‍♂️

@ghost
Copy link

ghost commented Nov 21, 2020

@jolelievre my last PR #8 deleted

{{ 'Hi {firstname} {lastname},'|trans({}, 'Emails.Body', locale) }}

Because it's an email for the shop not for the customer.

@ghost
Copy link

ghost commented Nov 21, 2020

atomiix
atomiix previously approved these changes Nov 23, 2020
@atomiix atomiix dismissed their stale review November 23, 2020 10:13

Clicked too fast 😅

@Julievrz
Copy link

The wording looks good to me, but I could not find the label. ✔️

@@ -10,7 +10,7 @@
<!-- TITLE BEGINING -->
</mj-raw>
<mj-text padding-top="0" padding-bottom="20px" font-weight="600" font-size="20px">
{{ 'Hi,'|trans({}, 'Emails.Body', locale) }}
{{ 'Hi {firstname} {lastname},'|trans({}, 'Emails.Body', locale) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

As @okom3pom said, this email is for the shop, not the customer.

Suggested change
{{ 'Hi {firstname} {lastname},'|trans({}, 'Emails.Body', locale) }}
{{ 'Hi,'|trans({}, 'Emails.Body', locale) }}

Copy link

@sowbiba sowbiba left a comment

Choose a reason for hiding this comment

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

LGTM
Waiting to resolve @atomiix and @okom3pom feedback to validate

Copy link
Contributor

@matthieu-rolland matthieu-rolland left a comment

Choose a reason for hiding this comment

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

LGTM for me too once @atomiix 's feedback has been dealt with.

.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
Copy link
Member

@Progi1984 Progi1984 left a comment

Choose a reason for hiding this comment

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

Some feedbacks

@jolelievre
Copy link
Contributor Author

@Progi1984 @atomiix You're both right, I was just lazy updating my global config 😅
It's done now and I removed the commit from the PR

@sarahdib
Copy link

@jolelievre this one can be tested by a dev :)

@atomiix
Copy link
Contributor

atomiix commented Jul 29, 2021

Thanks @jolelievre

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.

10 participants