Skip to content
This repository was archived by the owner on Dec 19, 2019. It is now read-only.

Set Newsletter Config #807

Merged
merged 7 commits into from
Aug 16, 2019
Merged

Set Newsletter Config #807

merged 7 commits into from
Aug 16, 2019

Conversation

@sedonik
Copy link
Contributor

@sedonik sedonik commented Aug 6, 2019

Description (*)

After created Magento\Newsletter\Model\Config it needs to update Magento\Newsletter\Observer\PredispatchNewsletterObserver with new implementation.

Manual testing scenarios (*)

Create Customer from Admin Panel.

Questions or comments

Related to PR.

@sedonik sedonik mentioned this pull request Aug 7, 2019
Merged
4 tasks
lenaorobei
lenaorobei reviewed Aug 8, 2019
View reviewed changes
app/code/Magento/Newsletter/Observer/PredispatchNewsletterObserver.php Outdated
public function __construct(
ScopeConfigInterface $scopeConfig,
UrlInterface $url,
Config $config
Copy link
Contributor

@lenaorobei lenaorobei Aug 8, 2019

Choose a reason for hiding this comment

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

Please follow BC guide here.
https://devdocs.magento.com/guides/v2.3/contributor-guide/backward-compatible-development/
Adding a constructor parameter section.

@lenaorobei
Copy link
Contributor

lenaorobei commented Aug 8, 2019

Please combine those PRs in a way that GraphQl related code is in one PR, core - in the second one.
For example, you need to move app/code/Magento/Newsletter/Model/Config.php here.

@sedonik
Copy link
Contributor Author

sedonik commented Aug 8, 2019

@lenaorobei,
I've updated the branch.
Pls, check.

lenaorobei
lenaorobei reviewed Aug 9, 2019
View reviewed changes
lenaorobei
lenaorobei reviewed Aug 9, 2019
View reviewed changes
app/code/Magento/Newsletter/Model/Config.php Outdated
*
* @return bool
*/
public function isActive($scopeType = ScopeConfigInterface::SCOPE_TYPE_DEFAULT)
Copy link
Contributor

@lenaorobei lenaorobei Aug 9, 2019

Choose a reason for hiding this comment

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

Suggested change
public function isActive($scopeType = ScopeConfigInterface::SCOPE_TYPE_DEFAULT)
public function isActive($scopeType = ScopeConfigInterface::SCOPE_TYPE_DEFAULT): bool

@sedonik
Copy link
Contributor Author

sedonik commented Aug 9, 2019

@lenaorobei ,

Thanks for the update.
I've added commit with suggestions.
Pls, check one more time.

@lenaorobei
Copy link
Contributor

lenaorobei commented Aug 12, 2019

@magento run Functional Tests build, Integration Tests build

@lenaorobei
Copy link
Contributor

lenaorobei commented Aug 12, 2019

@sedonik tests fail with Undefined class constant 'XML_PATH_NEWSLETTER_ACTIVE' message. To keep BC we cannot remove constants but need to mark it as deprecated. All usages of PredispatchNewsletterObserver::XML_PATH_NEWSLETTER_ACTIVE need to be replace with Config ::XML_PATH_NEWSLETTER_ACTIVE.

lenaorobei
lenaorobei reviewed Aug 12, 2019
View reviewed changes
app/code/Magento/Newsletter/Observer/PredispatchNewsletterObserver.php
* Configuration path to newsletter active setting
* @var Newsletter Config
*/
const XML_PATH_NEWSLETTER_ACTIVE = 'newsletter/general/active';
Copy link
Contributor

@lenaorobei lenaorobei Aug 12, 2019

Choose a reason for hiding this comment

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

Please mark as deprecated and add use Magento\Newsletter\Model\Config::XML_PATH_NEWSLETTER_ACTIVE instead comment.

@sedonik
Copy link
Contributor Author

sedonik commented Aug 12, 2019

@lenaorobei,
I've updated.

@lenaorobei
Copy link
Contributor

lenaorobei commented Aug 12, 2019

@sedonik we also need to check usages of deprecated constant and replace with newly introduced one.

@sedonik
Copy link
Contributor Author

sedonik commented Aug 13, 2019

@lenaorobei
Updated.
Please check.

@lenaorobei
Copy link
Contributor

lenaorobei commented Aug 15, 2019

@sedonik please see my fixes I pushed to your branch. Once we deliver this PR to 2.3-develop you will need to pull latest changes to #763 and finish this PR.

@magento-engcom-team
Copy link
Contributor

magento-engcom-team commented Aug 15, 2019

Hi @lenaorobei, thank you for the review.
ENGCOM-5636 has been created to process this Pull Request
✳️ @lenaorobei, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@sedonik
Copy link
Contributor Author

sedonik commented Aug 15, 2019

Ok, @lenaorobei,
I saw the changes. Thank you.

@magento-engcom-team magento-engcom-team merged commit 98dc804 into magento:2.3-develop Aug 16, 2019
@ghost
Copy link

ghost commented Aug 16, 2019

Hi @sedonik, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

magento-engcom-team pushed a commit that referenced this pull request Aug 16, 2019
@lenaorobei
ENGCOM-5636: Set Newsletter Config #807
e8afaa9
sedonik added a commit to sedonik/graphql-ce that referenced this pull request Aug 16, 2019
@sedonik
Added changes from PR magento#807
02654a6
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@lenaorobei lenaorobei lenaorobei approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lenaorobei lenaorobei

Labels

Auto-Tests: Covered Community Insider: Magecom community-insider-contribution Component: Customer Component: Newsletter Release Line: 2.3

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sedonik @lenaorobei @magento-engcom-team