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

Admin panel navigation improvement #33824

Open
wants to merge 19 commits into
base: 2.4-develop
Choose a base branch
from

Conversation

marcin-dykas
Copy link

Hello Magento Team

This is new kind of feature for admin panel. It adds additional links for products in several grids following [Customizable Options] grid.

Description (*)

Functionality for Magento 2 adds missing links in the admin panel for product edit pages. Code follows an idea from the [Customizable Options] section where is [Import options] product grid. [Import options] product grid contains URLs to product's edit page. Unfortunately, in other grids, there is no way to navigate to product edit pages easily. It adds edit URL links to all grids where navigation is needed to open product edit page in new tab.

configurations_original
add_related_original
related_original

Benefits

The administrator saves precious time while working with a large number of products. While adding options to products each time, there is a need to open grid with additional product items. There is no way to go to their edit pages without opening additional browser tabs and searching in other grids. It's complicated and time-consuming. So it adds edit links to the admin panel grids. After clicking in additional link product edit page is opened in a new tab.

add_products_to_group_new
grouped_new
products_to_options_new
options_new
associated_new
add_related_new
related_new

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes magento/magento2#<issue_number>

Manual testing scenarios (*)

  1. Open admin panel and go to Catalog -> Products
  2. Edit simple product
  3. Go to Related Products, Up-Sells, and Cross-Sells section
  4. Click Add Related Products
  5. Grid should contain links. After clicking product edit page is opened in new tab.
  6. Click Up-Sell Products
  7. Grid should contain links. After clicking product edit page is opened in new tab.
  8. Click Cross-Sell Products
  9. Grid should contain links. After clicking product edit page is opened in new tab.
  10. After saving all three grids for Related, Up-Sell and Cross-Sell grids should contain links. After clicking product edit page is opened in new tab.
  11. Go back to Catalog -> Products
  12. Open bundle product
  13. Go to Bundle Items section
  14. Open Add Option -> Add Product to Option
  15. Grid should contain links. After clicking product edit page is opened in new tab.
  16. After saving all options grids should have links. After clicking product edit page is opened in new tab.
  17. Go back to Catalog -> Products
  18. Open grouped product
  19. Go to Grouped Products section
  20. Add Products to Group grid should contain links for products. After clicking product edit page is opened in new tab.
  21. After saving Grouped Products grid should contain links for products. After clicking product edit page is opened in new tab.
  22. Go back to Catalog -> Products
  23. Open Configurable product
  24. Go to Configurations section
  25. Add Products Manually product grid should contain links. After clicking product edit page is opened in new tab.
  26. After saving Configurations grid should contain links. After clicking product edit page is opened in new tab.
  27. Go to Customizable Options section
  28. Import Options grid should contain links. After clicking product edit page is opened in new tab.

Questions or comments

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)
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Aug 17, 2021

Hi @marcin-dykas. Thank you for your contribution
Here are some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE,
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

@bgorski
Copy link
Contributor

bgorski commented Aug 17, 2021

@marcin-dykas thank you for your contribution! However, before we can proceed with reviewing the code and prioritizing the PR, you need to sign the Adobe Contributor License agreement. Can you do that please? You can find it here: https://opensource.adobe.com/cla.html
Please leave a comment when it's done. Thanks!

@marcin-dykas
Copy link
Author

Hello @bgorski

I've just signed CLA.

Thanks.

@bgorski
Copy link
Contributor

bgorski commented Aug 23, 2021

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

Copy link

@BarnyShergold BarnyShergold left a comment

Choose a reason for hiding this comment

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

Good submission!

However there are a lot of failed tests in the areas this PR covers. Please check and fix the failed tests.

There are quite a few new classes and new methods on existing classes yet there are no Unit Tests or MFTF tests. Please look into how your new functionality can be covered by testing.

AddUrlToName.php Outdated
@@ -0,0 +1,105 @@
<?php

Choose a reason for hiding this comment

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

Missing copyright notice

Copy link
Author

Choose a reason for hiding this comment

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

Fixed by adding copyright notice

* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
namespace Magento\ConfigurableProduct\Ui\DataProvider\Product;

Choose a reason for hiding this comment

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

Missing - declare(strict_types=1);

Copy link
Author

Choose a reason for hiding this comment

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

Fixed by adding declare(strict_types=1);

use Magento\Catalog\Model\ResourceModel\Product\CollectionFactory;
use Magento\Catalog\Ui\DataProvider\Product\ProductDataProvider;
use Magento\Ui\DataProvider\Modifier\PoolInterface;

Choose a reason for hiding this comment

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

Missing class DOCBLOCK

Copy link
Author

Choose a reason for hiding this comment

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

Fixed by adding DOCBLOCK

/**
* @var NameHelper
*/
protected $nameHelper;

Choose a reason for hiding this comment

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

should be private

Copy link
Author

Choose a reason for hiding this comment

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

Fixed by changing to private

/**
* @var LocatorInterface
*/
protected $locator;

Choose a reason for hiding this comment

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

class variables should be private

Copy link
Author

Choose a reason for hiding this comment

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

Fixed by changing to private

*
* @return array
*/
public function getData()

Choose a reason for hiding this comment

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

missing return type on prototype

Copy link
Author

Choose a reason for hiding this comment

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

Fixed by adding return type

* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
namespace Magento\ConfigurableProduct\Ui\DataProvider\Product;

Choose a reason for hiding this comment

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

missing declare(strict_types=1);

Copy link
Author

Choose a reason for hiding this comment

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

Fixed by adding declare(strict_types=1);

use Magento\Catalog\Model\ResourceModel\Product\CollectionFactory;
use Magento\Catalog\Ui\DataProvider\Product\ProductDataProvider;
use Magento\Ui\DataProvider\Modifier\PoolInterface;

Choose a reason for hiding this comment

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

Missing class DOCBLOCK

Copy link
Author

Choose a reason for hiding this comment

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

Fixed by adding DOCBLOCK

/**
* @var NameHelper
*/
protected $nameHelper;

Choose a reason for hiding this comment

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

private

Copy link
Author

Choose a reason for hiding this comment

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

Fixed by changing to private

* @param int $sortOrder
* @return array
*/
protected function getHtmlColumn($dataScope, $fit, Phrase $label, $sortOrder)

Choose a reason for hiding this comment

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

missing return type

Copy link
Author

Choose a reason for hiding this comment

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

Fixed by adding return type

@marcin-dykas
Copy link
Author

Hello @BarnyShergold

Thank you for CR

I resolved all issues and now I'll focus on tests.

Thank you

@m2-community-project m2-community-project bot moved this from Changes Requested to Review in Progress in Pull Requests Dashboard Aug 26, 2021
Copy link

@BarnyShergold BarnyShergold left a comment

Choose a reason for hiding this comment

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

Great test coverage from what I can see.
Just a few tidy ups

/**
* @var ObjectManager
*/
protected $objectManager;

Choose a reason for hiding this comment

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

All protected should be private

Copy link
Author

Choose a reason for hiding this comment

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

Fixed by changing all protected to private

/**
* @return object
*/
protected function getModel()

Choose a reason for hiding this comment

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

missing return type
should be private

Copy link
Author

Choose a reason for hiding this comment

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

Fixed by adding return type and changed to private

/**
* Test checks ConfigurableDataProvider type
*/
public function testCheckType()

Choose a reason for hiding this comment

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

missing return type

Copy link
Author

Choose a reason for hiding this comment

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

Fixed by adding return type

/**
* Test checks collection type
*/
public function testGetCollection()

Choose a reason for hiding this comment

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

missing return type

Choose a reason for hiding this comment

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

Usually it's good practice to put all the public methods first and then the private ones

Copy link
Author

Choose a reason for hiding this comment

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

Fixed by moving public functions first. Then protected then private ones at the end ....

@m2-community-project m2-community-project bot moved this from Review in Progress to Changes Requested in Pull Requests Dashboard Sep 6, 2021
@BarnyShergold
Copy link

@marcin-dykas - It's been 2 weeks since changes were requested on this PR for the tests and there is also a conflict to resolve. Can you please give us an update on progress and when you are likely to be able to complete this PR? Otherwise we may have to look at closing it. And we don't want to do that!

@marcin-dykas
Copy link
Author

Hello @BarnyShergold

I've just added last missing unit tests. Now I'll tidy up things to run tests again here.

@marcin-dykas
Copy link
Author

@magento run Unit Tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@marcin-dykas
Copy link
Author

marcin-dykas commented Sep 24, 2021

I've run unit tests here and there are two same errors from Jsunit (B2B) and Jsunit (EE)

Interception cache generation... 6/9 [========>----] 66% 54 secs 400.0 MiBErrors during compilation:
Magento\PricePermissions\Ui\DataProvider\Product\Form\Modifier\Grouped
Missed required argument nameHelper in parent::__construct call. File: /var/www/html/app/code/Magento/PricePermissions/Ui/DataProvider/Product/Form/Modifier/Grouped.php
Magento\PricePermissions\Ui\DataProvider\Product\Form\Modifier\Related
Incompatible argument type: Required type: \Magento\Catalog\Helper\Product\AddUrlToName. Actual type: string; File:
/var/www/html/app/code/Magento/PricePermissions/Ui/DataProvider/Product/Form/Modifier/Related.php

This is because EE version has Magento_PricePermissions which is not present in CE.

In this module there are classes like app/code/Magento/PricePermissions/Ui/DataProvider/Product/Form/Modifier/Grouped.php and app/code/Magento/PricePermissions/Ui/DataProvider/Product/Form/Modifier/Related.php.

They extend \Magento\GroupedProduct\Ui\DataProvider\Product\Form\Modifier\Grouped and \Magento\Catalog\Ui\DataProvider\Product\Form\Modifier\Related where I added one new parameter to constructor.

It should be easy to fix these errors. We just have to add new parameters in parent::_construct calls.

I don't have access to EE repository and I wonder now how to solve it. @BarnyShergold what is procedure in such case ? Is there any ? Will we leave it as is or are we going to create PR to EE repo ? Could you please advise ?

Thank you

@m2-community-project m2-community-project bot moved this from Changes Requested to Review in Progress in Pull Requests Dashboard Sep 29, 2021
@marcin-dykas
Copy link
Author

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@sidolov sidolov added the Priority: P3 May be fixed according to the position in the backlog. label Oct 28, 2021
…rm\Modifier\Data\AssociatedProductsTest::testAddManuallyConfigurationsWithNotFilterableInGridAttribute
app/code/Magento/Catalog/Ui/DataProvider/Product/Related/AbstractDataProvider.php

app/code/Magento/GroupedProduct/Ui/DataProvider/Product/GroupedProductDataProvider.php
@marcin-dykas
Copy link
Author

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@marcin-dykas
Copy link
Author

Still 9 checks are not successful but most of them fails because EE PricePermission module.

Error says:

Interception cache generation... 6/9 [========>----]  66% 1 min 444.0 MiBErrors during compilation:
	Magento\PricePermissions\Ui\DataProvider\Product\Form\Modifier\Grouped
		Missed required argument nameHelper in parent::__construct call. File: /var/www/html/app/code/Magento/PricePermissions/Ui/DataProvider/Product/Form/Modifier/Grouped.php
	Magento\PricePermissions\Ui\DataProvider\Product\Form\Modifier\Related
		Incompatible argument type: Required type: \Magento\Catalog\Helper\Product\AddUrlToName. Actual type: string; File:
/var/www/html/app/code/Magento/PricePermissions/Ui/DataProvider/Product/Form/Modifier/Related.php

Since I don't have access to EE repository I can't fix it. I decided to provided patch to this module which solves this issue.

EE_PricePermissions_module_patch.zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Pull Requests Dashboard
  
Review in Progress
Development

Successfully merging this pull request may close these issues.

None yet

6 participants