Skip to content

Fix issue #13944. Show Store Views in Terms and Conditions grid. #14546

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 24, 2018
Merged

Fix issue #13944. Show Store Views in Terms and Conditions grid. #14546

merged 1 commit into from
Apr 24, 2018

Conversation

afirlejczyk
Copy link
Contributor

Fix showing Store View in Terms and Conditions grid.

Fixed Issues (if relevant)

  1. Stores -> Terms and Conditions - No Store View shown #13944: Stores -> Terms and Conditions - No Store View shown #

Manual testing scenarios

  1. Disable Single-Store Mode in Magento by going to Stores -> Configuration -> General -> Single-Store Mode
  2. Go to Stores -> Terms and Conditions
  3. Add New Condition
  4. Store View should be displayed in Column "Store View"

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)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Apr 5, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@sidolov
Copy link
Contributor

sidolov commented Apr 5, 2018

Hi @afirlejczyk , please, sign CLA, otherwise, we can't process your pull request. Thanks!

@afirlejczyk
Copy link
Contributor Author

Hi @sidolov I signed CLA.

I have one more question, I noticed Terms and Condition is not build with UI component.
Can I rebuild this view (in new pull request)?

@sidolov
Copy link
Contributor

sidolov commented Apr 6, 2018

Hi @afirlejczyk , yes, it's a good idea!
Looks like your commit was added with another email, GitHub posted warning in the message above, can you add commit with email from current GitHub account or add email address from commit to your account? Thanks!

@sidolov sidolov self-assigned this Apr 6, 2018
@magento-engcom-team magento-engcom-team added this to the April 2018 milestone Apr 6, 2018
sidolov
sidolov requested changes Apr 6, 2018
View reviewed changes
app/code/Magento/CheckoutAgreements/Block/Adminhtml/Agreement/Grid.php Outdated
* @param array $data
* @codeCoverageIgnore
*/
public function __construct(
\Magento\Backend\Block\Template\Context $context,
\Magento\Backend\Helper\Data $backendHelper,
\Magento\CheckoutAgreements\Model\ResourceModel\Agreement\CollectionFactory $collectionFactory,
\Magento\CheckoutAgreements\Model\ResourceModel\Agreement\Grid\CollectionFactory $collectionFactory,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, inject new dependencies according to our Backward Compatible Development policy

@afirlejczyk
Copy link
Contributor Author

Thank you for your review @sidolov
I made changes.

afirlejczyk
afirlejczyk commented Apr 11, 2018
View reviewed changes
app/code/Magento/CheckoutAgreements/Block/Adminhtml/Agreement/Grid.php
* @codeCoverageIgnore
* @SuppressWarnings(PHPMD.UnusedFormalParameter)
*/
public function __construct(
\Magento\Backend\Block\Template\Context $context,
\Magento\Backend\Helper\Data $backendHelper,
\Magento\CheckoutAgreements\Model\ResourceModel\Agreement\CollectionFactory $collectionFactory,

This comment was marked as resolved.

Sign in to view

Copy link
Contributor

Choose a reason for hiding this comment

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

You may leave it as is

sidolov
sidolov requested changes Apr 14, 2018
View reviewed changes
app/code/Magento/CheckoutAgreements/Block/Adminhtml/Agreement/Grid.php Outdated
) {
$this->_collectionFactory = $collectionFactory;

$this->_collectionFactory = $gridColFactory
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better define a new private property for grid collection, assign injected object and change all usages to new object (this step caused by backward compatibility, any extension may be used protected $_collectionFactory and expects concrete object type).
The old property should be marked as deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I added private variable gridCollectionFactory

@magento-engcom-team
Copy link
Contributor

Hi @sidolov, thank you for the review.
ENGCOM-1249 has been created to process this Pull Request

@magento-engcom-team magento-engcom-team mentioned this pull request Apr 24, 2018
Closed
@magento-engcom-team magento-engcom-team merged commit 49432af into magento:2.2-develop Apr 24, 2018
magento-engcom-team pushed a commit that referenced this pull request Apr 24, 2018
ENGCOM-1249: Fix issue #13944. Show Store Views in Terms and Conditio…
9f0af04 
…ns grid. #14546
@magento-engcom-team
Copy link
Contributor

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

This was referenced Apr 26, 2018
Merged
Merged
@afirlejczyk afirlejczyk added Partner: Divante partners-contribution Pull Request is created by Magento Partner labels Jan 9, 2019
@afirlejczyk afirlejczyk deleted the fix/13944-terms-and-conditions-grid branch January 9, 2019 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sidolov sidolov sidolov approved these changes

Assignees

@sidolov sidolov

Labels
Partner: Divante partners-contribution Pull Request is created by Magento Partner Progress: accept Release Line: 2.2
Projects
None yet
Milestone
April 2018
Development

Successfully merging this pull request may close these issues.
4 participants
@afirlejczyk @magento-cicd2 @sidolov @magento-engcom-team